Page MenuHome

Edit Mesh: multi-thread auto-smooth sharp-edge calculation
ClosedPublic

Authored by Campbell Barton (campbellbarton) on Jul 20 2021, 9:56 AM.
Tags
None
Tokens
"Love" token, awarded by Zino."Love" token, awarded by chironamo."Love" token, awarded by gilberto_rodrigues.

Details

Summary

Merge the sharp edge tagging into bm_mesh_loops_calc_normals,
this has the advantage that edge tagging can be performed as part of
walking over each vertices edges - instead of tagging in a separate loop.

Even though this will tag edges twice (once for each vertex),
the computation isn't heavy as it's only calculating a dot-product
between the two face users to compare the angle.

This change combined with D11928 makes BM_loops_calc_normal_vcos
around 5.68x faster,
with an overall speedup over 2.6x when transforming a high poly mesh.
(tested on a system with 32 cores).

---

This must be applied on top of D11928.

Diff Detail

Repository
rB Blender
Branch
TEMP-BMESH-AUTOSMOOTH-MT-MERGE-SPLIT-EDGES (branched from master)
Build Status
Buildable 15910
Build 15910: arc lint + arc unit

Event Timeline

Campbell Barton (campbellbarton) requested review of this revision.Jul 20 2021, 9:56 AM
Campbell Barton (campbellbarton) created this revision.
Campbell Barton (campbellbarton) retitled this revision from Optimize auto-smooth sharp-edge calculation to Edit Mesh: multi-thread auto-smooth sharp-edge calculation.Jul 20 2021, 10:07 AM
Bastien Montagne (mont29) requested changes to this revision.Jul 22 2021, 4:21 PM

We get the same potential issues caused by introducing atomic ops used at a very high rate here as in D11993: Mesh: optimize normal calculation... think we can avoid using them or any thread safety measure here however, see comments below?

Besides that point, patch LGTM.

source/blender/bmesh/intern/bmesh_mesh_normals.c
876–881

Not sure we actually need atomics here?

  • Assigning to uint8_t can be considered as atomic in itself (in the sense that concurrent assignment should never generate corrupted data).
  • Result we want to assign to e->head.hflag should always be the same in both potentially concurrent assignments, and does not depend on the initial value of e->head.hflag.

That would need a proper detailed comment about why we do not need atomics/thread safety here of course, but think we can get rid of them in that very specific case.

This revision now requires changes to proceed.Jul 22 2021, 4:21 PM
  • Cast to a char so the atomic operation can't become un-safe if this type ever changes.

Attempt to update based on branch

Updated diff manually, arc is including changes it shouldn't

Think assigning to any native data type that fits into current arch wordsize is always atomic?

Note that the whole operation (read, bitwise bool op, and assign) is not atomic, but as said in my comment this is not a problem in our very specific case.

This revision is now accepted and ready to land.Jul 22 2021, 5:18 PM
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)