Details

Reviewers

Sergey Sharybin (sergey)
Campbell Barton (campbellbarton)
Jacques Lucke (JacquesLucke)

Maniphest Tasks

T94495: Geometry nodes: Split edges node leads to a crash in edit mode

Commits

rBedb15b84583e: Mesh: Avoid creating incorrect original index layers
rB1850a0b2ab12: Mesh: Avoid creating incorrect original index layers

Summary

Currently, whenever any BMesh is converted to a Mesh (except for edit
mode switching), original index (CD_ORIGINDEX) layers are added.
This is incorrect, because many operations just convert some Mesh into
a BMesh and then back. Those operations shouldn't make any assumption
about where their input mesh came from. It might even come from a
primitive in geometry nodes, where there are no original indices at all.

Conceptually, mesh original indices should be filled by the modifier
stack when first creating the evaluated mesh. So that's where they're
moved in this patch. A separate function now fills the indices with their
default (0,1,2,3...) values. The way the mesh wrapper system defers
the BMesh to Mesh conversion makes this a bit less obvious though.

The old behavior is incorrect, but it's also slower, because three
arrays the size of the mesh's vertices, edges, and faces had to be
allocated and filled during the BMesh to Mesh conversion, which just
ended up putting more pressure on the cache. In the many cases where
original indices aren't used, I measured an 8% speedup for the
conversion (from 76.5ms to 70.7ms).

Generally there is an assumption that BMesh is "original" and Mesh is
"evaluated". After this patch, that assumption isn't quite as strong,
but it still exists. First, original indices are added whenever converting a
BMesh "wrapper" to a Mesh. Second, original indices are not added to
the BMesh at the beginning of evaluation, which assumes that every
BMesh in the viewport is original and doesn't need the mapping.

Fixes T94495

Diff Detail

Repository: rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Feb 5 2022, 1:48 AM

Hans Goudey (HooglyBoogly) created this revision.

Hans Goudey (HooglyBoogly) added inline comments.Feb 5 2022, 1:51 AM

source/blender/nodes/geometry/nodes/node_geo_edge_split.cc
42–44	This could be committed separately, since it's a problem on the other side of the conversion (mesh to BMesh). I wanted to keep it in the patch to add some context though. Here, the process is totally correct if the BMesh is created with any existing orig index layers, which will still be transferred to the result. What was not correct is that later on, `BKE_mesh_from_bmesh_for_eval_nomain` would make up its own orig indices.

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Feb 5 2022, 1:56 AM

Hans Goudey (HooglyBoogly) mentioned this in D13715: Fix T94495: 'Split Edges' geometry node leads to a crash in edit mode.Feb 5 2022, 2:02 AM

Do we guarantee that CD_ORIGINDEX never exists in the BMesh? Or, at least is always up-to-date? Depending on this the "ensure" behavior might need to be changed to the origindex re-initialization on the input of the modifier stack.

source/blender/blenkernel/intern/DerivedMesh.cc
1453–1454	Why there is no need to ensure origindex here?
1536	Same as above
1581	Why this is needed? As in, why the origindex was not ensured when the `mesh_final` was created from bmesh in the evaluation loop above?
source/blender/blenkernel/intern/mesh.cc
47	Not really sure where this is used.
79	Same as above.
1231	Suggest to split it into 2 lines to make it easier to follow what's going on.

Do we guarantee that CD_ORIGINDEX never exists in the BMesh?

It depends which BMesh you're talking about. In general no, but I think you're talking about the original edit mode BMesh. In that case there should be no original index layers. In the latest version of the patch I added an assert to verify that.

source/blender/blenkernel/intern/DerivedMesh.cc
1453–1454	Well, original index layers weren't added here before this patch, since this is just creating a wrapper of the BMesh. Forgetting the previous behavior for a moment though, I think this makes sense because we don't do orig index mapping when drawing a `BMesh`, for better or worse. That's what I described in the last paragraph of the patch description.
1581	You're right. It will be done by the mesh wrapper when necessary anyway.
source/blender/blenkernel/intern/mesh.cc
47	Right, sorry, I forgot to remove that.

Add an assert about no original index layers on original mesh
Split into two lines for clarity
Remove unnecessary ensuring of original indices
Cleanup: Slightly improve comments
Merge branch 'master' into fix-incorrect-orig-indices

Thanks for the clarifications!

From the modifiers evaluation side think the remaining question from my side is: shall there be BKE_mesh_ensure_default_orig_index_layers call in the create_orco_mesh?

The rest of the change seems reasonable to me, although would be nice to have Campbell's view here as well.

From the modifiers evaluation side think the remaining question from my side is: shall there be BKE_mesh_ensure_default_orig_index_layers call in the create_orco_mesh?

I wasn't sure about that. My conceptual understanding in this area is a bit weaker. I see some explicit handling of the orig index layer masks, I'm not totally sure how this relates though:

mask.vmask |= CD_MASK_ORIGINDEX;
mask.emask |= CD_MASK_ORIGINDEX;
mask.pmask |= CD_MASK_ORIGINDEX;

I agree Campbell's opinion would be very valuable here.

One way to view it is this: if we don't add it and it ends up being necessary, we can just add it then. But if we add it and it's not necessary, we'll probably forget about it and end up adding unnecessary layers for the next few years ;)

Merge master

By the way, when committing this I would commit the edge split node change to 3.1, but the rest of the patch only to the 3.2 branch.

Jacques Lucke (JacquesLucke) accepted this revision.Feb 14 2022, 10:14 AM

Jacques Lucke (JacquesLucke) added inline comments.

source/blender/blenkernel/intern/mesh_wrapper.c
135	I wonder what you mean with "assume" here: Does it mean that something would break if the assumption is wrong, or would that just add a performance penalty?

Improve comment about assumptions

Hans Goudey (HooglyBoogly) marked 3 inline comments as done.Feb 14 2022, 5:00 PM

Gilberto Rodrigues (gilberto_rodrigues) added a subscriber: Gilberto Rodrigues (gilberto_rodrigues).Feb 18 2022, 1:15 AM

This looks good, although there are some cases original indices are later expected to to used still.

Although I didn't check for bugs it's safest to keep original indices in cases they might still be needed.

As has been noted create_orco_mesh returns a mesh that can be passed into BKE_modifier_modify_mesh so this would be good to keep.
construct_param_handle_subsurfed passes the result of BKE_mesh_from_bmesh_for_eval_nomain to subsurf_make_derived_from_derived(..) which checks for original indices. However currently CDDM_from_mesh is removing those original indices so this can be handled separately from this patch.

Accepting as an extra iteration from me isn't needed.

source/blender/blenkernel/BKE_mesh.h
93	Similar functions refer to `customdata` instead of layers, eg: `BKE_mesh_ensure_skin_customdata` `BKE_mesh_ensure_facemap_customdata` This could be named using a similar convention.

Oops, didn't realize I'm a blocking reviewer.
No stoppers from my side!

This revision is now accepted and ready to land.Feb 18 2022, 11:29 AM

Hans Goudey (HooglyBoogly) mentioned this in rB82c3bef7655b: Fix T94495: Split edges node leads to a crash in edit mode.Feb 18 2022, 5:34 PM

Closed by commit rB1850a0b2ab12: Mesh: Avoid creating incorrect original index layers (authored by Hans Goudey (HooglyBoogly)). · Explain WhyFeb 18 2022, 5:57 PM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rB1850a0b2ab12: Mesh: Avoid creating incorrect original index layers.

Philipp Oeser (lichtwerk) added a commit: rBedb15b84583e: Mesh: Avoid creating incorrect original index layers.Mar 24 2022, 9:07 AM

Mesh: Avoid creating incorrect original index layers
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 47992

source/blender/blenkernel/BKE_mesh.h

source/blender/blenkernel/intern/DerivedMesh.cc

source/blender/blenkernel/intern/mesh.cc

source/blender/blenkernel/intern/mesh_wrapper.c

source/blender/bmesh/intern/bmesh_mesh_convert.cc

source/blender/nodes/geometry/nodes/node_geo_edge_split.cc

Mesh: Avoid creating incorrect original index layersClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 47992

source/blender/blenkernel/BKE_mesh.h

source/blender/blenkernel/intern/DerivedMesh.cc

source/blender/blenkernel/intern/mesh.cc

source/blender/blenkernel/intern/mesh_wrapper.c

source/blender/bmesh/intern/bmesh_mesh_convert.cc

source/blender/nodes/geometry/nodes/node_geo_edge_split.cc

Mesh: Avoid creating incorrect original index layers
ClosedPublic
Actions

Revision Contents
Changeset List