Fix T72667: Crash when collections unlink an edit-mesh
AbandonedPublic
Actions

Authored by Campbell Barton (campbellbarton) on Jan 5 2020, 4:41 AM.

Details

Reviewers

Bastien Montagne (mont29)
Dalai Felinto (dfelinto)

Maniphest Tasks

T72667: Outliner "delete hierarchy" crash with a mesh in edit-mode

Summary

This patch fixes T72667: Outliner "delete hierarchy" crash with a mesh in edit-mode, however it's more a design decision, which has further implications.

Note that this issue could be fixed differently - however I looked into some alternatives such as...

Supporting keeping the mesh in edit-mode without an object. While this could be made to work, there are quite a few complications with the draw manager - making this a larger change then I would have expected... there are also a bug outside the scope of this patch - see T72848.
Removing BMEditMesh.ob pointer which resolves the initial crash, leaving other non-trivial issues to resolve.

If we go with this fix, there are further changes that should be made for completeness, although I don't consider them blockers.

Add the ability to exit edit-mode for all other object types (not just edit-mesh).
Add the ability to flush sculpt data (currently un-linking an object in sculpt mode wont flush it's data to the mesh).

Diff Detail

Repository

rB Blender

Branch

TEMP-T72667-FIX (branched from master)

Build Status

Buildable 6163
Build 6163: arc lint + arc unit

Event Timeline

Campbell Barton (campbellbarton) created this revision.Jan 5 2020, 4:41 AM

Harbormaster completed remote builds in B6163: Diff 20626.Jan 5 2020, 4:41 AM

Campbell Barton (campbellbarton) added a task: T72667: Outliner "delete hierarchy" crash with a mesh in edit-mode.Jan 5 2020, 4:45 AM

Campbell Barton (campbellbarton) retitled this revision from Fix T72667: Crash when unlinking edit-mesh to Fix T72667: Crash when collections unlink an edit-mesh.Jan 5 2020, 4:53 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Campbell Barton (campbellbarton) added a reviewer: Dalai Felinto (dfelinto).

Campbell Barton (campbellbarton) added a project: BF Blender.

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Jan 5 2020, 4:57 AM

Patch looks good to me from a quick check…

I have another problem with that issue though: it reveals that BMEditMesh.ob is not handled at all in library_query code! I guess it should be added there with the IDWALK_CB_LOOPBACK flag? Wondering whether that would be enough to fix original issue (although this might fall under the 'edit mesh without object' case you said was tricky to get working then…).

But in any case all ID pointers within blender data (including in temp sub-structs) should be handled here, so that’s another (trivial) bug to fix.

Main concern with that change otherwise is that it only seems to address the case from hierarchy deletion? Because objects can be deleted from many other places/ways, having to track them all like that is not really nice… :/

Shouldn't that indeed be called from BKE_object_free? (which should be called from BKE_id_free_us). I think this is what @Bastien Montagne (mont29) is suggesting as well, no.

An issue with flushing data in free functions means it would end up performing expensive data flushing between modes on exit - for example.

Calling in BKE_id_free_us makes more sense since this isn't called from BKE_main_free, so this could work.

Even so, this means BKE_id_free_us may end up calling high-level code when flushing mode data.

Updating shape-keys, multi-res, possibly clearing & re-initializing point-cache. Doing this from low level code could cause some complicated bugs.

This is why I preferred to only perform the flushing when manually unlinking a collection, is more of an explicit action, as @Bastien Montagne (mont29) notes it leaves the issue for other cases where the user unlinks the object.

I'll look into breaking this up into steps,

Fix the BMEditMesh.ob dangling pointer issue by clearing edit-mode data or removing this struct member entirely.
Support multiple objects in edit-mode (fixing T72848).
Flush edits when unlinking, but don't clear Object-Data editing structure since this may be used in another window.

I'm not sure exactly where this should be done, as mentioned BKE_id_free_us could back-fire, this could even be handled by operators, calling ED_editors_flush_edits or adding a similar function that takes a collection argument.