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

Merged
Nathan Vegdahl merged 12 commits from nathanvegdahl/blender:111780_armature_undo_with_collections into main 2023-09-11 18:28:24 +02:00
Member

The issue was that Armatures have their own edit-mode undo system, but it
didn't account for bone collections yet.

The solution implemented here is to also copy the list of BoneCollections
in each edit-mode undo step, just like EditBones themselves already are.
Additionally, the undo EditBones now have their bone collection membership
remapped to point at those undo copies, so that the entire undo step is
self-contained.

When restoring from an undo step, we simply do the reverse: copy all of
the EditBones and BoneCollections back to the actual armature, and remap
the EditBone collection membership appropriately.

This patch also includes a temporary workaround for a final issue, where
BoneCollection membership information could be lost when undoing pops you
out of edit mode. The correct solution for this is a bit involved, and
will be part of a future PR. But the workaround ensures that things
aren't broken in the mean time.

The issue was that Armatures have their own edit-mode undo system, but it didn't account for bone collections yet. The solution implemented here is to also copy the list of BoneCollections in each edit-mode undo step, just like EditBones themselves already are. Additionally, the undo EditBones now have their bone collection membership remapped to point at those undo copies, so that the entire undo step is self-contained. When restoring from an undo step, we simply do the reverse: copy all of the EditBones and BoneCollections back to the actual armature, and remap the EditBone collection membership appropriately. This patch also includes a temporary workaround for a final issue, where BoneCollection membership information could be lost when undoing pops you out of edit mode. The correct solution for this is a bit involved, and will be part of a future PR. But the workaround ensures that things aren't broken in the mean time.
Nathan Vegdahl added the
Module
Animation & Rigging
label 2023-09-05 11:29:55 +02:00
Nathan Vegdahl added this to the Animation & Rigging project 2023-09-05 11:30:07 +02:00
Nathan Vegdahl force-pushed 111780_armature_undo_with_collections from 9af9549519 to 0253bada97 2023-09-05 17:44:32 +02:00 Compare
Author
Member

The fix is working now, except for one weird corner case: immediately upon loading a file with an armature already in it, if you enter edit mode on that armature, make any change to the armature, and then undo until it pops you out of edit mode again, all collection membership is lost when you pop out.

I've begun investigating this, but haven't tracked it down yet.

The fix is working now, except for one weird corner case: immediately upon loading a file with an armature already in it, if you enter edit mode on that armature, make any change to the armature, and then undo until it pops you out of edit mode again, all collection membership is lost when you pop out. I've begun investigating this, but haven't tracked it down yet.
Nathan Vegdahl changed title from WIP: Fix #111780: bone collections and undo don't work together to Fix #111780: bone collections and undo don't work together 2023-09-07 17:28:04 +02:00
Nathan Vegdahl requested review from Sybren A. Stüvel 2023-09-07 17:28:29 +02:00
Nathan Vegdahl requested review from Bastien Montagne 2023-09-08 10:10:39 +02:00
Bastien Montagne approved these changes 2023-09-08 10:41:32 +02:00
Bastien Montagne left a comment
Owner

Essentially just signing of on the hack regarding forcing an extra memfile undo step storage when entering Armature Edit mode. As discussed in person, this is not ideal, but an acceptable workaround until proper separation of all edit mode data is implemented.

(On a longer term, it's likely best to investigate getting entirely rid of the edit data and special edit mode undo for armatures, it makes really little sense to have it in the first place).

Essentially just signing of on the hack regarding forcing an extra memfile undo step storage when entering Armature Edit mode. As discussed in person, this is not ideal, but an acceptable workaround until proper separation of all edit mode data is implemented. (On a longer term, it's likely best to investigate getting entirely rid of the edit data and special edit mode undo for armatures, it makes really little sense to have it in the first place).
@ -845,0 +849,4 @@
* object-mode armature data until exiting edit mode. But that
* change is a bit of a project, and will be done later. This line
* should be removed when that is done. */
bmain->is_memfile_undo_written = false;

Think it's important to address this asap (4.0 is too late for sure, but would make it an important target for 4.1).

Think it's important to address this asap (4.0 is too late for sure, but would make it an important target for 4.1).

@nathanvegdahl could you make a task for this & assign it to the 4.1 milestone?

@nathanvegdahl could you make a task for this & assign it to the 4.1 milestone?
Author
Member

Will do!

Edit: #112134

Will do! Edit: #112134
nathanvegdahl marked this conversation as resolved
Sybren A. Stüvel requested changes 2023-09-08 12:44:03 +02:00
@ -194,0 +196,4 @@
/* -------------------------------------------------------------------- */
/* Only used by edit-mode Armature undo: */
void ANIM_bonecoll_listbase_free(ListBase *bcolls, const bool do_id_user);

This should document well what it's for, especially since it's so specific to a single use case.

Also include what do_id_user means.

Also remove the const here, it has no meaning in the function declaration.

This should document well what it's for, especially since it's so specific to a single use case. Also include what `do_id_user` means. Also remove the `const` here, it has no meaning in the function declaration.
nathanvegdahl marked this conversation as resolved
@ -194,0 +198,4 @@
void ANIM_bonecoll_listbase_free(ListBase *bcolls, const bool do_id_user);
/**
* Duplicates a list of BoneCollections.

I find the naming of this function a bit confusing. It claims to just duplicate, but then it also discards certain information, and builds other info. It doesn't seem like it's doing what's on the tin.

I find the naming of this function a bit confusing. It claims to just duplicate, but then it also discards certain information, and builds other info. It doesn't seem like it's doing what's on the tin.
nathanvegdahl marked this conversation as resolved
@ -194,0 +211,4 @@
*/
void ANIM_bonecoll_listbase_copy(ListBase *bone_colls_dst,
ListBase *bone_colls_src,
blender::Map<BoneCollection *, BoneCollection *> *r_bcoll_map,

Return parameters should always be the last one. In this case I would just make the function return the map, though.

Return parameters should always be the last one. In this case I would just make the function return the map, though.
nathanvegdahl marked this conversation as resolved
@ -194,0 +212,4 @@
void ANIM_bonecoll_listbase_copy(ListBase *bone_colls_dst,
ListBase *bone_colls_src,
blender::Map<BoneCollection *, BoneCollection *> *r_bcoll_map,
const bool do_id_user);

As above, drop the const.

As above, drop the `const`.
nathanvegdahl marked this conversation as resolved
@ -437,0 +440,4 @@
/* ********************************************** */
/* Utility functions for Armature edit-mode undo. */
void ANIM_bonecoll_listbase_free(ListBase *bcolls, const bool do_id_user)

Swap the ANIM_bonecoll_listbase_free and ANIM_bonecoll_listbase_copy functions, both in the .cc and .hh files. By having the 'copy' function first, it's clearer what the 'free' function is for.

Swap the `ANIM_bonecoll_listbase_free` and `ANIM_bonecoll_listbase_copy` functions, both in the `.cc` and `.hh` files. By having the 'copy' function first, it's clearer what the 'free' function is for.
nathanvegdahl marked this conversation as resolved
@ -832,3 +830,1 @@
for (ebone = static_cast<EditBone *>(lb->first); ebone; ebone = ebone_next) {
ebone_next = ebone->next;
LISTBASE_FOREACH_MUTABLE (EditBone *, ebone, lb) {

Keep non-functional and functional changes separate. Is changing this function really necessary for this PR?

Keep non-functional and functional changes separate. Is changing this function really necessary for this PR?
nathanvegdahl marked this conversation as resolved
@ -46,2 +69,3 @@
EditBone *act_edbone;
ListBase lb;
BoneCollection *active_collection;
ListBase ebones;

Document the type that's stored in the ListBases, for example ListBase /*EditBone*/ ebones.
It might be trivial, but it will reduce a level of guesswork.

Document the type that's stored in the ListBases, for example `ListBase /*EditBone*/ ebones`. It might be trivial, but it will reduce a level of guesswork.
nathanvegdahl marked this conversation as resolved
@ -68,0 +97,4 @@
ANIM_bonecoll_listbase_copy(&arm->collections, &uarm->bone_collections, &bcoll_map, true);
arm->active_collection = bcoll_map.lookup_default(uarm->active_collection, nullptr);
/* Point the new edit bones at the new collections. */

IMO this comment can be removed, as it just repeats what the function does. If there is the need, the function needs a better name ;-)

IMO this comment can be removed, as it just repeats what the function does. If there is the need, the function needs a better name ;-)
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl requested review from Sybren A. Stüvel 2023-09-08 15:25:29 +02:00

(On a longer term, it's likely best to investigate getting entirely rid of the edit data and special edit mode undo for armatures, it makes really little sense to have it in the first place).

@mont29 I think the main point of armature edit data is that it is not hierarchical, i.e. bone coordinates are stored relative to the armature rather than the parent bone, and thus bones can be easily moved without affecting their children, or reparented without recomputing the coordinates.

> (On a longer term, it's likely best to investigate getting entirely rid of the edit data and special edit mode undo for armatures, it makes really little sense to have it in the first place). @mont29 I think the main point of armature edit data is that it is not hierarchical, i.e. bone coordinates are stored relative to the armature rather than the parent bone, and thus bones can be easily moved without affecting their children, or reparented without recomputing the coordinates.
Author
Member

I think the main point of armature edit data is that it is not hierarchical, i.e. bone coordinates are stored relative to the armature rather than the parent bone, and thus bones can be easily moved without affecting their children, or reparented without recomputing the coordinates.

It also allows selection and manipulation of the head/tail of the bone, rather than the bone itself as a whole.

But I nevertheless agree with @mont29. We should still be able to provide convenience APIs and present UI manipulation that works with bones essentially the same way that EditBones do now, without actually having a completely separate data structure that needs to be created/destroyed/synced and all the headaches that come with that.

Having separate edit-mode data structures can make sense for e.g. mesh sculpt mode, where there are significant trade offs that you're making that are needed while sculpting but not appropriate otherwise. But for armatures, even with thousands of bones I have a hard time imagining it being a performance/memory bottleneck to just compute this kind of stuff on the fly, so that everything is always synced and users (both via UI and via Python API) don't have to worry about the Bone/EditBone distinction.

> I think the main point of armature edit data is that it is not hierarchical, i.e. bone coordinates are stored relative to the armature rather than the parent bone, and thus bones can be easily moved without affecting their children, or reparented without recomputing the coordinates. It also allows selection and manipulation of the head/tail of the bone, rather than the bone itself as a whole. But I nevertheless agree with @mont29. We should still be able to provide convenience APIs and present UI manipulation that works with bones essentially the same way that EditBones do now, without *actually* having a completely separate data structure that needs to be created/destroyed/synced and all the headaches that come with that. Having separate edit-mode data structures can make sense for e.g. mesh sculpt mode, where there are significant trade offs that you're making that are needed while sculpting but not appropriate otherwise. But for armatures, even with thousands of bones I have a hard time imagining it being a performance/memory bottleneck to just compute this kind of stuff on the fly, so that everything is always synced and users (both via UI and via Python API) don't have to worry about the Bone/EditBone distinction.
Sybren A. Stüvel requested changes 2023-09-11 11:57:22 +02:00
Sybren A. Stüvel left a comment
Member

Nice work! Just some minor notes.

Nice work! Just some minor notes.
@ -437,0 +446,4 @@
BLI_assert(BLI_listbase_is_empty(bone_colls_dst));
blender::Map<BoneCollection *, BoneCollection *> bcoll_map =
blender::Map<BoneCollection *, BoneCollection *>();

Is this repetition really necessary? Wouldn't this do the trick?

blender::Map<BoneCollection *, BoneCollection *> bcoll_map();
Is this repetition really necessary? Wouldn't this do the trick? ```cpp blender::Map<BoneCollection *, BoneCollection *> bcoll_map(); ```
Author
Member

Ah, that's my rusty (pun intended) C++ showing through. I forgot you could even initialize things that way in C++. Thanks!

Ah, that's my rusty (pun intended) C++ showing through. I forgot you could even initialize things that way in C++. Thanks!
Author
Member

Apparently that doesn't work, because it's treated as a function prototype. The correct initialization is apparently just:

blender::Map<BoneCollection *, BoneCollection *> bcoll_map;

Which for classes will automatically call the default constructor. Stylistically that rubs me the wrong way, though, because it's visually the same as an uninitialized variable. So there's another way in C++11 and above, using uniform initialization syntax, which is what I've now changed the code to:

blender::Map<BoneCollection *, BoneCollection *> bcoll_map{};

This keeps it both short and explicit. Might be worth adding this to the style guide at some point (there doesn't currently seem to be anything about object initialization there).

Apparently that doesn't work, because it's treated as a function prototype. The correct initialization is apparently just: ``` blender::Map<BoneCollection *, BoneCollection *> bcoll_map; ``` Which for classes will automatically call the default constructor. Stylistically that rubs me the wrong way, though, because it's visually the same as an uninitialized variable. So there's another way in C++11 and above, using uniform initialization syntax, which is what I've now changed the code to: ``` blender::Map<BoneCollection *, BoneCollection *> bcoll_map{}; ``` This keeps it both short and explicit. Might be worth adding this to the style guide at some point (there doesn't currently seem to be anything about object initialization there).
nathanvegdahl marked this conversation as resolved
@ -437,0 +450,4 @@
LISTBASE_FOREACH (BoneCollection *, bcoll_src, bone_colls_src) {
BoneCollection *bcoll_dst = static_cast<BoneCollection *>(MEM_dupallocN(bcoll_src));
/* This is rebuilt from the edit bones, so we don't need to copy it. */

is → will be

is → will be
nathanvegdahl marked this conversation as resolved
@ -437,0 +472,4 @@
IDP_FreeProperty_ex(bcoll->prop, do_id_user);
}
/* Bone references. */

Instead of documenting the 'what', I think it's better to document the 'why' here. After all, this list should be empty (it is explicitly cleared in the duplication function above).

Instead of documenting the 'what', I think it's better to document the 'why' here. After all, this list should be empty (it is explicitly cleared in the duplication function above).
Author
Member

Ah, very good point. Indeed, it's not obvious that the list won't always be empty. I'll add that in the comment.

Ah, very good point. Indeed, it's not obvious that the list won't always be empty. I'll add that in the comment.
nathanvegdahl marked this conversation as resolved
@ -827,6 +827,7 @@ void ED_armature_to_edit(bArmature *arm)
void ED_armature_ebone_listbase_free(ListBase *lb, const bool do_id_user)
{

This newline can be removed from the PR.

This newline can be removed from the PR.
Author
Member

Oops. Thanks for catching that!

Oops. Thanks for catching that!
nathanvegdahl marked this conversation as resolved
@ -836,6 +837,9 @@ void ED_armature_ebone_listbase_free(ListBase *lb, const bool do_id_user)
IDP_FreeProperty_ex(ebone->prop, do_id_user);
}
/* Bone collection references. */

I don't think this comment and the next are necessary. They only rephrase the code, and the surrounding code is so simple there is no need for a "this section does X" style of comments either.

I don't think this comment and the next are necessary. They only rephrase the code, and the surrounding code is so simple there is no need for a "this section does X" style of comments either.
nathanvegdahl marked this conversation as resolved
@ -41,0 +52,4 @@
* bones and collections together.
*/
static void remap_ebone_bone_collection_references(
ListBase *edit_bones, blender::Map<BoneCollection *, BoneCollection *> *bcoll_map)
  • Document what's in the ListBase
  • Since bcoll_map is not being modified here, pass it as const reference instead of pointer.
- Document what's in the `ListBase` - Since `bcoll_map` is not being modified here, pass it as `const` reference instead of pointer.
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl force-pushed 111780_armature_undo_with_collections from 6deb74ad47 to ffbf980f5c 2023-09-11 12:51:01 +02:00 Compare
Sybren A. Stüvel requested changes 2023-09-11 12:59:08 +02:00
Sybren A. Stüvel left a comment
Member

I'm getting a compiler warning:

In file included from /home/sybren/workspace/blender-git/build_linux/source/blender/makesrna/intern/rna_armature_gen.cc:33:
In file included from /home/sybren/workspace/blender-git/blender/source/blender/makesrna/intern/rna_armature.cc:73:
/home/sybren/workspace/blender-git/blender/source/blender/animrig/ANIM_bone_collections.h:219:50: warning: 'ANIM_bonecoll_listbase_copy_no_membership' has C-linkage specified, but returns incomplete type 'blender::Map<BoneCollection *, BoneCollection *>' (aka 'Map<BoneCollection *, BoneCollection *>') which could be incompatible with C [-Wreturn-type-c-linkage]
blender::Map<BoneCollection *, BoneCollection *> ANIM_bonecoll_listbase_copy_no_membership(
                                                 ^
1 warning generated.

Basically you're writing C++ in a C-only header.
I think it's fine to wrap these two function declarations in a #ifdef __cplusplus block.

I'm getting a compiler warning: ``` In file included from /home/sybren/workspace/blender-git/build_linux/source/blender/makesrna/intern/rna_armature_gen.cc:33: In file included from /home/sybren/workspace/blender-git/blender/source/blender/makesrna/intern/rna_armature.cc:73: /home/sybren/workspace/blender-git/blender/source/blender/animrig/ANIM_bone_collections.h:219:50: warning: 'ANIM_bonecoll_listbase_copy_no_membership' has C-linkage specified, but returns incomplete type 'blender::Map<BoneCollection *, BoneCollection *>' (aka 'Map<BoneCollection *, BoneCollection *>') which could be incompatible with C [-Wreturn-type-c-linkage] blender::Map<BoneCollection *, BoneCollection *> ANIM_bonecoll_listbase_copy_no_membership( ^ 1 warning generated. ``` Basically you're writing C++ in a C-only header. I think it's fine to wrap these two function declarations in a `#ifdef __cplusplus` block.
Author
Member

I think it's fine to wrap these two function declarations in a #ifdef __cplusplus block.

Additionally had to move it out of the extern C block, which makes sense.

> I think it's fine to wrap these two function declarations in a #ifdef __cplusplus block. Additionally had to move it out of the `extern C` block, which makes sense.
Nathan Vegdahl force-pushed 111780_armature_undo_with_collections from ce0df94a45 to 0fbc3621a7 2023-09-11 17:56:43 +02:00 Compare
Nathan Vegdahl force-pushed 111780_armature_undo_with_collections from 0fbc3621a7 to cbd1407dd2 2023-09-11 18:01:36 +02:00 Compare
Nathan Vegdahl added 1 commit 2023-09-11 18:22:52 +02:00
Sybren A. Stüvel approved these changes 2023-09-11 18:24:51 +02:00
Nathan Vegdahl merged commit d64e4a387a into main 2023-09-11 18:28:24 +02:00
Nathan Vegdahl deleted branch 111780_armature_undo_with_collections 2023-09-11 18:28:26 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#111965
No description provided.