Fix T96327: data transfer crash with GPU subdivision
ClosedPublic
Actions

Authored by Kévin Dietrich (kevindietrich) on Mar 29 2022, 3:40 PM.

Details

Reviewers

Sergey Sharybin (sergey)
Campbell Barton (campbellbarton)
Brecht Van Lommel (brecht)

Maniphest Tasks

T96327: GPU Subdividion: DataTransfer modifier with custom normal option crash blender

Commits

rB08731d70bf66: Fix T96327: data transfer crash with GPU subdivision

Summary

The crash is caused as the subdivision wrapper does not have loop
normals, which are generally computed at the end of the modifier stack
evaluation via mesh_calc_modifier_final_normals. (Note that they are
initially computed, but deleted by the subdivision wrapper creation.)

This records in the mesh runtime whether loop normals should have been
computed and computes them alongside the subdivision wrapper.

Diff Detail

Repository: rB Blender

Event Timeline

Kévin Dietrich (kevindietrich) requested review of this revision.Mar 29 2022, 3:40 PM

Kévin Dietrich (kevindietrich) created this revision.

Harbormaster completed remote builds in B21322: Diff 49841.Mar 29 2022, 3:40 PM

Not really my area or expertise, but seems reasonable.

Kévin Dietrich (kevindietrich) added a task: T96327: GPU Subdividion: DataTransfer modifier with custom normal option crash blender.Apr 12 2022, 4:52 PM

This isn't just related to this patch, but I think there is some clarification missing about the meaning of CD_NORMAL.
To me anyway, it's not clear whether it refers to derived/runtime data that can always be calculated from mesh data, or whether it actually means custom normals that can be changed by users.

I had thought that CD_NORMAL meant the former, and that CD_CUSTOMLOOPNORMAL was used for the latter. But this patch seems to treat CD_NORMAL equivalently to custom normals. Or maybe the mesh is just in an "invalid" state WRT this information briefly?

Part of why I'm asking is because I was planning to do a similar refactor for face corner normals as for face and vertex normals.

In D14489#393069, @Hans Goudey (HooglyBoogly) wrote:

This isn't just related to this patch, but I think there is some clarification missing about the meaning of CD_NORMAL.
To me anyway, it's not clear whether it refers to derived/runtime data that can always be calculated from mesh data, or whether it actually means custom normals that can be changed by users.

I had thought that CD_NORMAL meant the former, and that CD_CUSTOMLOOPNORMAL was used for the latter. But this patch seems to treat CD_NORMAL equivalently to custom normals. Or maybe the mesh is just in an "invalid" state WRT this information briefly?

Part of why I'm asking is because I was planning to do a similar refactor for face corner normals as for face and vertex normals.

One thing to note is this patch is mostly copying the logic from MOD_subsurf.c, it is not really doing anything special.

I think part of the confusion on the meaning may come from the fact that CD_NORMAL is used as a temporary layer whenever we need to do some (nondestructive) work with custom normals? Here (and in the subdiv code in general) we need to interpolate normals, which uses the interpolation callback from LayerTypeInfo, but CD_CUSTOMLOOPNORMAL does not have one. It seems like a similar thing happens for data transfer. That's just my two cents, someone with better knowledge could explain better.

Brecht Van Lommel (brecht) requested changes to this revision.Apr 26 2022, 4:57 PM

Brecht Van Lommel (brecht) added a subscriber: Brecht Van Lommel (brecht).

Brecht Van Lommel (brecht) added inline comments.

source/blender/blenkernel/intern/DerivedMesh.cc
632	I think we should we do the same in `editbmesh_calc_modifier_final_normals`?
634	Could we skip computing loop normals here for the subdiv wrapper? It seems like a waste of time if they are going to be recomputed later, and also is not consistent with the modifier being evaluated as part of the stack, since loop normals would then always be computed after the subdiv modifier.
source/blender/blenkernel/intern/mesh_wrapper.c
366 ↗	(On Diff #49841)	`BKE_mesh_tessface_clear` is unnecessary and can be removed, I removed it from the place you copied it from in rB51a7e4b488a4: Cleanup: remove unused mface tesselation code from modifier stack.