Page MenuHome

Fix T98813: crash with GPU subdiv in edit mode and instance objects
ClosedPublic

Authored by Brecht Van Lommel (brecht) on Jun 13 2022, 6:38 PM.

Details

Summary

Instancing with geometry nodes uses just the evaluated Mesh, and ignores the
Object that it came from. That meant that it would try to look up the subsurf
modifier on the instancer object which does not have the subsurf modifier.

Instead of storing a session UUID and looking up the modifier data, store a
point to the subsurf modifier runtime data. Unlike the modifier data, this
runtime data is preserved across depsgraph CoW. It must be for the subdiv
descriptor contained in it to stay valid along with the draw cache.

As a bonus, this moves various Mesh_Runtime variables into the subsurf runtime
data, reducing memory usage for meshes not using subdivision surfaces.

This change is bigger than I would hope for 3.2.1, but I tried to carefully
keep existing behavior:

  • mesh_wrapper_ensure_subdivision would call BKE_subsurf_modifier_ensure_runtime instead of just accessing the pointer. However we never need to allocate runtime data at this point, the subsurf mesh wrapper only gets setup after we have already allocated the runtime data in the subsurf modifier.
  • calc_loop_normals and use_loop_normals are separate variables because they already were in the existing code. The former indicates if there is data mask asking for loop normals, and the latter if the subsurf modifier will interpolate them. Potentially we do not need to calc loop normals if they will not be interpolated, but leaving that unchanged for now.
  • use_optimal_display now is affected by MOD_APPLY_TO_BASE_MESH, but this should make no difference as that's for applying modifiers and meshes generated for that purpose will not go to the draw code.

Diff Detail

Repository
rB Blender

Event Timeline

Brecht Van Lommel (brecht) requested review of this revision.Jun 13 2022, 6:38 PM
Brecht Van Lommel (brecht) created this revision.
Hans Goudey (HooglyBoogly) added inline comments.
source/blender/makesdna/DNA_mesh_types.h
126–128

This comment should probably be explicit about ownership: the struct is owned by the subdivision surface modifier.

It seems like it could be owned by the mesh potentially, which would allow using the same system for the subdivision surface geometry node. Maybe that would make the change too big for 3.2 though, not sure.

I only have a minor inline comment, everything else seems to be in order.

source/blender/draw/intern/draw_cache_impl_subdivision.cc
2067–2068

The last sentence about mesh can be removed.

This revision is now accepted and ready to land.Jun 14 2022, 7:41 AM
Sergey Sharybin (sergey) requested changes to this revision.Jun 14 2022, 10:03 AM

From a quick glance at a code overall seems fine. But there are some aspects which I wouldn't mind testing. The problem is: the patch does not apply cleanly for me.
Marking as requested changes so that the patch is not seen as ready for land. Once you're online or at the studio I'm sure we can quickly go over those little things I wanted to test!

This revision now requires changes to proceed.Jun 14 2022, 10:03 AM
This revision is now accepted and ready to land.Jun 14 2022, 1:40 PM
Brecht Van Lommel (brecht) marked 2 inline comments as done.

Improve comments