Fix T67190 .
- New Walker added ( bmw_NonManifoldedgeWalker_type )
- New walker used when edge connected with 3 or more faces .
- logic of bmw_EdgeLoopWalker_step used , see line number 1651 .
Details
Details
- Reviewers
Campbell Barton (campbellbarton) - Maniphest Tasks
- T67190: Edge Loop Select doesn't support non-manifold edges
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- D10497-update (branched from master)
- Build Status
Buildable 13399 Build 13399: arc lint + arc unit
Event Timeline
Comment Actions
Attached example that fails, select face at thr 3D cursor.
| source/blender/bmesh/intern/bmesh_walkers_impl.c | ||
|---|---|---|
| 1648 | This isn't used. | |
Comment Actions
In case edge loop is not continuous , some edges remains unselected .Reverse traversal is done to overcome the problem (from line 1658) . Begins from edge selected by user .
Changes
- BMwEdgeLoopWalker.start used as BMwEdgeLoopWalker.cur
- BMwEdgeLoopWalker.start tracks previous edge in reverse traversal ( Separate struct with two struct members last1, last2 can be created . Prevents use of start to track the previous edge eg. BMwNonManifoldWalker .)
- do-while loop .
- Loop until face count at edge will match with count(face) of the edge of edge loop .
Comment Actions
Changes with respect to previous diff :
- (BM_elem_flag_test(temp->e, BM_ELEM_SELECT) check removed .
- temp variable pointer removed .
| source/blender/bmesh/intern/bmesh_walkers_impl.c | ||
|---|---|---|
| 1647 | Walkers shouldn't depend on selection (notice none of the other walkers here do). | |
| 1648–1652 | This can be extracted into a function, since it's called again below. | |
| 1650 | This assumes loop is part of an edge with two face users. That should be checked for with BM_loop_is_manifold, if that fails. bail out of the loopl. | |
| 1659 | This is starting out from one of the loops only, it could start out from any of 3+ loops. Shouldn't all of them be considered as candidates for stepping over? | |
Comment Actions
This is an example that doesn't work properly:
This shows why you need to walk over all radial loops of each edge.
| source/blender/bmesh/intern/bmesh_walkers_impl.c | ||
|---|---|---|
| 1661 | This still seems to be | |
Comment Actions
- loop added to walk through all faces connected to edge
- for loop introduced to avoid separate loop code for reverse traversing
Comment Actions
- This walker should define it's own struct there is no need to reuse BMwEdgeLoopWalker.
| source/blender/bmesh/intern/bmesh_walkers_impl.c | ||
|---|---|---|
| 1617–1618 | This doesn't make sense to set, it can be removed from a new struct. Instead, the face_count should be stored, currently this is counted on every edge step which isn't needed. | |
| 1663 | For a predictable outcome this should check all radial loops connect to the same edge (or can't find an edge that matches). This way there can never be a branch in the path which follows only one of the edges. | |
Comment Actions
Changes
- struct BMwNonManifoldEdgeLoopWalker created
- l_cur introduced : to store valid non manifold edge .
Comment Actions
Generally looks good, the code misses some comments, it should include an explanation of some of the less obvious logic.
For example
- Reasoning for requiring all connected edges match.
- Short description of bmw_NonManifoldLoop_edgeloop
- Some general comments on how the stepping works.
| source/blender/bmesh/intern/bmesh_walkers_impl.c | ||
|---|---|---|
| 1663 | bmw_NonManifoldLoop_edgeloop will never return an BM_edge_is_boundary(l->e) edge, this check is redundant. | |
Comment Actions
Changes :
- Comments added .
- Description of bmw_NonManifoldLoop_edgeloop added .
- Check BM_edge_is_boundary(l->e) removed .
Comment Actions
- Fix bmw_NonManifoldLoop_edgeloop call assigning to 'l' which is used for radial iteration around 'e'.