Fix T96344: edit mode GPU subdivision crashes with X-ray mode and modifiers
ClosedPublic
Actions

Authored by Kévin Dietrich (kevindietrich) on Mar 29 2022, 9:35 AM.

Details

Reviewers

Clément Foucault (fclem)
Jeroen Bakker (jbakker)
Brecht Van Lommel (brecht)

Maniphest Tasks

T96344: GPU subdivision: Crash on select in Edit Mode with X-Ray on and Mirror Modifier

Commits

rBf88646a1edcc: Fix T96344: edit mode GPU subdivision crashes with X-ray mode and modifiers
rBba28c10199b3: Fix T96344: edit mode GPU subdivision crashes with X-ray mode and modifiers

Summary

The crash happens because the origindex layers created as part of
the modifier stack evaluation are not set in the MeshRenderData when
they should have been.

This is because when selecting in X-ray mode, a subdivision wrapper
is created to ensure that selection happens with a subdivided
geometry, and this replaces the MDATA wrapper which is also used to
setup the MeshRenderData.

As we do not seemingly have an MDATA wrapper, the draw code decides
that we can extract draw buffers directly from the BMesh, instead of
the mapped Mesh with origin indices layers.

To fix this, we should also consider to use mapped extraction if a
subdivision wrapper exists on the mesh.

NOTE: the program flow is the following: - GPU subdivision is initially done in edit mode, the Mesh has an MDATA wrapper - When selection happens with X-ray mode, a subdivision wrapper is created - Draw code considers to have a BMesh since we are in edit mode and there is no MDATA wrapper - Crash due to out of bounds access of BMesh faces when updating the edit_data VBO, which is the only one needing an update

CC @Brecht Van Lommel (brecht), @Campbell Barton (campbellbarton), @Sergey Sharybin (sergey), as this seems to be the first
case of two different wrappers colliding and may have design implications.

Diff Detail

Repository

rB Blender

Branch

fix_T96344_gpu_subd_xray_crash (branched from master)

Build Status

Buildable 21304
Build 21304: arc lint + arc unit

Event Timeline

Kévin Dietrich (kevindietrich) requested review of this revision.Mar 29 2022, 9:35 AM

Kévin Dietrich (kevindietrich) created this revision.

Harbormaster completed remote builds in B21304: Diff 49816.Mar 29 2022, 9:35 AM

Kévin Dietrich (kevindietrich) added a task: T96344: GPU subdivision: Crash on select in Edit Mode with X-Ray on and Mirror Modifier.Mar 29 2022, 3:31 PM

Just to be sure, is this fix "safe"? in a way that it does not alter other cases. To me it seems to be (safe).

I am not knowledgeable on the new Mesh wrappers so I cannot judge on the design implications.

The fix seems to work ok. I tried with and without x-ray enabled, and in different files than the one from the bug report. But then I don't really know if it might cause other issues in other configurations than edit mode + on cage GPU subdiv + x-ray.

Brecht Van Lommel (brecht) accepted this revision.Mar 29 2022, 7:22 PM

Brecht Van Lommel (brecht) added inline comments.

source/blender/draw/intern/draw_cache_extract_mesh_render_data.c
472–473	The use of the modified `has_mdata` logic here makes sense to me. It's effectively enabling `use_mapped` whenever the wrapper is not bmesh, which means there is a topology modifying modifier and a need for mapping.
505	The use of `has_mdata` here I don't understand, though I already do not understand it without this patch. I would expect it to test `use_mapped` instead, that is you want to change `MR_EXTRACT_MAPPED` -> `MR_EXTRACT_MESH` in case there is no good mapping. However, as far as this fix is concerned I don't think it's introducing additional issues.