Paths

Table of Contentst

Fix T94144: Duplicate edges in dual mesh node
ClosedPublic
Actions

Authored by Wannes Malfait (Wannes) on Dec 1 2021, 11:23 PM.

Details

Reviewers

Hans Goudey (HooglyBoogly)
Jacques Lucke (JacquesLucke)

Maniphest Tasks

T94144: Geometry Nodes: Dual Mesh creates duplicate edges

Commits

rBe9092110ff1c: Fix T94144: Duplicate edges in dual mesh node

Summary

The duplicated edges were caused by 'oversubdivided' edges, i.e. edges
where some of the vertices on them are only connected to two polygons.
For most 'normal' meshes this shouldn't occurr, or only very little, so
the performance impact of this change should be neglegible. In practice
this is also avoidable by triangulating the mesh first.

The fix finds these vertices and 'dissolves' them so that only one edge
is created.

Blend file with bad mesh (tested with BKE_mesh_is_valid):

dual_mesh_2poly_verts.blend845 KBDownload

Diff Detail

Repository: rB Blender

Event Timeline

Wannes Malfait (Wannes) requested review of this revision.Dec 1 2021, 11:23 PM

Wannes Malfait (Wannes) created this revision.

Harbormaster completed remote builds in B19137: Diff 45546.Dec 1 2021, 11:23 PM

Wannes Malfait (Wannes) edited the summary of this revision. (Show Details)Dec 1 2021, 11:29 PM

Wannes Malfait (Wannes) added reviewers: Hans Goudey (HooglyBoogly), Jacques Lucke (JacquesLucke).

Wannes Malfait (Wannes) added a project: Geometry Nodes.

Jacques Lucke (JacquesLucke) added inline comments.Dec 6 2021, 5:52 PM

source/blender/nodes/geometry/nodes/node_geo_dual_mesh.cc
711	The comment should mention that the mesh is not dissolved in the input mesh, but only in the data structures you derived from it.
712	This entire loop should be moved to a separate function. It shouldn't clutter this "main" function because it rarely has an impact on the result.
720	It seems like it dissolves an edge, but that edge might not actually include `vert_i`, maybe I'm missing something.
728	Is there some guarantee above the order of indices in `vertex_poly_indices`? Would be good to add comments to `vertex_poly_indices` similar variables.

Split code into new function and clean up comments

Harbormaster completed remote builds in B19240: Diff 45711.Dec 6 2021, 6:36 PM

Hans Goudey (HooglyBoogly) requested changes to this revision.Dec 15 2021, 9:23 PM

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/nodes/geometry/nodes/node_geo_dual_mesh.cc
542	`derived datastructures from the mesh` -> `data structures derived from the mesh`
545	Sometimes when there's a rather confusing situation like this, I create a bug report with some images if there isn't one already, then you can write something like (see TXXXX) here
570–583	These checks could be moved to a separate function to avoid the duplication (only v1 vs v2 is different between them. And maybe the function name could give some hint about the meaning of the check
574	Setting the type to "Loose" is a way to make it ignored later on, right? Do you think it would make more sense to have a separate `Ignored` type? I'm not sure myself.
587–588	I don't really understand how putting poly indices in the vert fields of MEdge makes sense. Is there something I'm missing?
638–639	+1 for the comment. I'd rather see something more specific than "the way this is created". What about the way it's created?

This revision now requires changes to proceed.Dec 15 2021, 9:23 PM

Split code into new function and clean up comments
Cleanup: comments and move check to separate function

Harbormaster completed remote builds in B19478: Diff 46111.Dec 16 2021, 10:55 AM

Wannes Malfait (Wannes) added a task: T94144: Geometry Nodes: Dual Mesh creates duplicate edges.Dec 16 2021, 10:56 AM

Wannes Malfait (Wannes) retitled this revision from Fix duplicate edges in dual mesh node to Fix T94144: Duplicate edges in dual mesh node.

Wannes Malfait (Wannes) added inline comments.

source/blender/nodes/geometry/nodes/node_geo_dual_mesh.cc
574	I think setting it to 'loose' is fine, as it corresponds to what happens. I think of it like the when you hold 'alt' in the node editor to 'detach' a node from a link. In the same way the vertex gets detached in this case from the edge (not actually but that's how it's treated afterwards), and is therefore 'loose'.
587–588	Polygon indices in the input mesh become vertex indices in the dual mesh. I also added a comment to clarify this.

The code looks good to me now. Thanks!

This revision is now accepted and ready to land.Dec 16 2021, 4:34 PM

Closed by commit rBe9092110ff1c: Fix T94144: Duplicate edges in dual mesh node (authored by Wannes Malfait (Wannes), committed by Hans Goudey (HooglyBoogly)). · Explain WhyDec 20 2021, 8:08 PM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rBe9092110ff1c: Fix T94144: Duplicate edges in dual mesh node.

Revision Contents
Changeset List

Path

Size

source/

blender/

nodes/

geometry/

nodes/

node_geo_dual_mesh.cc

84 lines

Diff 46240

View Options

source/blender/nodes/geometry/nodes/node_geo_dual_mesh.cc

Fix T94144: Duplicate edges in dual mesh nodeClosedPublicActions