Bone collections and bone collection membership are not handled properly when undoing in armature edit mode #111780

New Issue

Nathan Vegdahl · 2023-09-01T10:58:07+02:00

Nathan Vegdahl commented

2023-09-01 10:58:07 +02:00

Blender Version
Broken: 998136f7a7

Short description of error

When you undo in armature edit mode, all bones lose their bone collection memberships, and adding/removing bone collections is not undone.

Exact steps for others to reproduce the error

Create an armature.
Enter edit mode.
Add some bones and collections, and assign some bones to some collections.
Repeatedly undo, and see that bone collection membership is lost on all bones, and the operations adding bone collections don't get undone.

This is due to bone collections not being handled at all in the armature undo code.

In order to initially land bone collections, I added the following code as a stop-gap to avoid crashes due to accessing invalid memory:

		source/blender/editors/armature/armature_utils.cc
		Lines 871 to 881 in 619f38514e
	
				    /* TODO: WORKAROUND: this is a temporary hack to avoid segfaults when

				     * undoing, because bone collections are not handled properly by the

				     * armature undo code yet.  This just discards all collection membership

				     * data to avoid dangling references.  This MUST be addressed properly

				     * before release.

				     * See: https://projects.blender.org/blender/blender/pulls/109976#issuecomment-1008429

				     */

				    BoneCollectionReference *bcoll_ref = (BoneCollectionReference *)(&ebone_dst->bone_collections);

				    bcoll_ref->next = nullptr;

				    bcoll_ref->prev = nullptr;

				    bcoll_ref->bcoll = nullptr;

It does indeed eliminate the crashes, but also causes undo to work incorrectly. To fix this issue, that code must be removed, and all crashes and misbehavior resulting from that need to be addressed.

**Blender Version** Broken: 998136f7a7b **Short description of error** When you undo in armature edit mode, all bones lose their bone collection memberships, and adding/removing bone collections is not undone. **Exact steps for others to reproduce the error** 1. Create an armature. 2. Enter edit mode. 3. Add some bones and collections, and assign some bones to some collections. 4. Repeatedly undo, and see that bone collection membership is lost on all bones, and the operations adding bone collections don't get undone. This is due to bone collections not being handled at all in the armature undo code. ---- In order to initially land bone collections, I added the following code as a stop-gap to avoid crashes due to accessing invalid memory: https://projects.blender.org/blender/blender/src/commit/619f38514e9908456936b0be64ea9cc565c3c8fb/source/blender/editors/armature/armature_utils.cc#L871-L881 It does indeed eliminate the crashes, but also causes undo to work incorrectly. To fix this issue, that code must be removed, and all crashes and misbehavior resulting from that need to be addressed.

Nathan Vegdahl added the

labels 2023-09-01 10:59:01 +02:00

Nathan Vegdahl self-assigned this 2023-09-01 10:59:09 +02:00

Nathan Vegdahl commented

2023-09-01 17:53:51 +02:00

Further investigation into the crashes revealed that there are multiple underlying issues, any of which can cause crashes:

The armature undo code in editors/armature/editarmature_undo.cc copies the edit bones to the undo stack, for later restoration. However, it doesn't copy the collection membership listbase, so the undo copy of an edit bone points to the same listbase memory as the edit bone still in the armature. If the latter gets deleted, the collection membership listbase also gets deleted, and the undo edit bone is then pointing at invalid memory. This issue is relatively straightforward to address, by simply doing proper copy of that membership listbase... except that will still interact badly with the remaining issues.
The armature undo code in editors/armature/editarmature_undo.cc doesn't handle bone collections at all. This means that adding/removing collections while in armature edit mode doesn't get undone. That on its own needs to get fixed just because it's faulty undo behavior. But it also can result in invalid memory access, due to the collection membership listbase stored in the undo edit bones. If a collection is removed, and then you undo, one of the edit bones may still be pointing to the removed collection. But even after fixing things to restore removed collections on undo, they presumably will not have the same memory address, so the undo edit bones will still point to invalid memory.
Outside of edit mode, undo is handled by copying data from a memory file. So even if we fix both of the above issues to work properly within edit mode, undoing outside of edit mode frees and recreates the bone collections, so the edit-mode undo stack will again have edit bones pointing at invalid memory via their collection membership listbase. And this also leads to crashes when object-mode undo and edit-mode undo interact, which is fairly easy to trigger.

Further investigation into the crashes revealed that there are multiple underlying issues, any of which can cause crashes: 1. The armature undo code in `editors/armature/editarmature_undo.cc` copies the edit bones to the undo stack, for later restoration. However, it doesn't copy the collection membership listbase, so the undo copy of an edit bone points to the same listbase memory as the edit bone still in the armature. If the latter gets deleted, the collection membership listbase also gets deleted, and the undo edit bone is then pointing at invalid memory. This issue is relatively straightforward to address, by simply doing proper copy of that membership listbase... except that will still interact badly with the remaining issues. 2. The armature undo code in `editors/armature/editarmature_undo.cc` doesn't handle bone collections *at all*. This means that adding/removing collections while in armature edit mode doesn't get undone. That on its own needs to get fixed just because it's faulty undo behavior. But it *also* can result in invalid memory access, due to the collection membership listbase stored in the undo edit bones. If a collection is removed, and then you undo, one of the edit bones may still be pointing to the removed collection. But even after fixing things to restore removed collections on undo, they presumably will not have the same memory address, so the undo edit bones will *still* point to invalid memory. 3. Outside of edit mode, undo is handled by copying data from a memory file. So even if we fix both of the above issues to work properly *within* edit mode, undoing outside of edit mode frees and recreates the bone collections, so the edit-mode undo stack will again have edit bones pointing at invalid memory via their collection membership listbase. And this also leads to crashes when object-mode undo and edit-mode undo interact, which is fairly easy to trigger.

Nathan Vegdahl referenced a pull request that will close this issue

2023-09-05 11:29:27 +02:00

Fix #111780: bone collections and undo don't work together #111965

Sybren A. Stüvel added this to the 4.0 milestone 2023-09-05 15:07:32 +02:00

Pratik Borhade referenced this issue

2023-09-08 08:50:13 +02:00

When rotating the rigify bone and then pressing CTRL+Z, the bone group is lost #112120

Nathan Vegdahl referenced this issue from a commit

2023-09-11 18:28:26 +02:00

Fix #111780: bone collections and undo don't work together

Nathan Vegdahl closed this issue

2023-09-11 18:28:27 +02:00