Anim: store BoneCollections in a flat array #115354

Merged
Nathan Vegdahl merged 2 commits from nathanvegdahl/blender:hierarchical_bone_collections into main 2023-12-05 10:39:58 +01:00
Member

This is in preparation for eventual hierarchical bone collections.

The motivation here is that this will allow us to efficiently specify
children as an index range, which would be inefficient with a listbase
due to the list traversal overhead incurred for index-based look ups.

We're still saving to blend files as a list base for forwards compatibility
with Blender 4.0, but storing as an array at runtime for efficient indexing.

This should not result in any user-visible changes.

This is in preparation for eventual hierarchical bone collections. The motivation here is that this will allow us to efficiently specify children as an index range, which would be inefficient with a listbase due to the list traversal overhead incurred for index-based look ups. We're still saving to blend files as a list base for forwards compatibility with Blender 4.0, but storing as an array at runtime for efficient indexing. This should not result in any user-visible changes.
Nathan Vegdahl force-pushed hierarchical_bone_collections from 5c6531c94d to 6459ff3517 2023-11-24 16:35:51 +01:00 Compare
Nathan Vegdahl force-pushed hierarchical_bone_collections from 6459ff3517 to 2979ae155a 2023-11-27 17:58:15 +01:00 Compare
Nathan Vegdahl changed title from WIP: Anim: make Bone Collections hierarchical/nested to WIP: Anim: store BoneCollections in a flat array 2023-11-27 17:58:55 +01:00
Nathan Vegdahl added the
Module
Animation & Rigging
label 2023-11-27 18:00:12 +01:00
Nathan Vegdahl changed title from WIP: Anim: store BoneCollections in a flat array to Anim: store BoneCollections in a flat array 2023-11-27 18:00:36 +01:00
Nathan Vegdahl requested review from Christoph Lendenfeld 2023-11-27 18:00:47 +01:00
Author
Member

@blender-bot build

@blender-bot build
Nathan Vegdahl force-pushed hierarchical_bone_collections from 2979ae155a to 5aa0e367a5 2023-11-28 11:20:51 +01:00 Compare
Author
Member

@blender-bot build

@blender-bot build
Christoph Lendenfeld approved these changes 2023-11-28 12:15:27 +01:00
Christoph Lendenfeld left a comment
Member

can see nothing wrong with that
good job

can see nothing wrong with that good job
Author
Member

Looking over the code again myself, I noticed some things that could be nicer. So I'll hold off on merging until I've had a chance to address those.

Looking over the code again myself, I noticed some things that could be nicer. So I'll hold off on merging until I've had a chance to address those.
Nathan Vegdahl requested review from Sybren A. Stüvel 2023-12-04 11:39:18 +01:00
Sybren A. Stüvel requested changes 2023-12-04 14:32:44 +01:00
@ -118,3 +115,1 @@
'.',
offsetof(BoneCollection, name),
sizeof(bcoll->name));
struct DupNameCheckData {

This struct can be removed, as bonecoll_name_is_duplicate can just use armature and bcoll by reference from the outer scope. Just make sure they're captured by changing [] to [&].

This struct can be removed, as `bonecoll_name_is_duplicate` can just use `armature` and `bcoll` by reference from the outer scope. Just make sure they're captured by changing `[]` to `[&]`.
Author
Member

Unfortunately we can't do this, because BLI_uniquename_cb expects a function pointer of a specific type, and making bonecoll_name_is_duplicate capture anything makes it not match that type.

Unfortunately we can't do this, because `BLI_uniquename_cb` expects a function pointer of a specific type, and making `bonecoll_name_is_duplicate` capture anything makes it not match that type.
nathanvegdahl marked this conversation as resolved
@ -121,0 +143,4 @@
* This means, for example, that for a collection array of length N, you can
* pass N as the index to append to the end.
*/
static void bonecoll_insert_at_index(bArmature *armature, BoneCollection *bcoll, int index)

const int

`const int`
nathanvegdahl marked this conversation as resolved
@ -121,0 +185,4 @@
*/
static BoneCollection *bonecoll_remove(bArmature *armature, int index)
{
BLI_assert(index < armature->collection_array_num);

Not 0 <= index?

Not `0 <= index`?
nathanvegdahl marked this conversation as resolved
@ -121,0 +202,4 @@
armature->collection_array[armature->collection_array_num] = nullptr;
/* Update the active BoneCollection. */
int new_active_index = armature->runtime.active_collection_index;

As discussed, replace with:

  /* Update the active BoneCollection. */
  if (index >= armature->runtime.active_collection_index) {
    int active_index = armature->runtime.active_collection_index;

    if (index == armature->collection_array_num - 1) {
      /* Removing the last element: activate the now-last element. */
      active_index--;
    }
    else if (index < active_index) {
      /* The active collection shifted, because a collection before it was removed. */
      active_index--;
    }
    ANIM_armature_bonecoll_active_index_set(armature, active_index);
  }

This makes sure that there is a clear separation between "deciding what the active index should be" and "using the active index to ensure pointers are correct".

As discussed, replace with: ```cpp /* Update the active BoneCollection. */ if (index >= armature->runtime.active_collection_index) { int active_index = armature->runtime.active_collection_index; if (index == armature->collection_array_num - 1) { /* Removing the last element: activate the now-last element. */ active_index--; } else if (index < active_index) { /* The active collection shifted, because a collection before it was removed. */ active_index--; } ANIM_armature_bonecoll_active_index_set(armature, active_index); } ``` This makes sure that there is a clear separation between "deciding what the active index should be" and "using the active index to ensure pointers are correct".
nathanvegdahl marked this conversation as resolved
@ -121,0 +219,4 @@
static int bonecoll_find_index(const bArmature *armature, const BoneCollection *bcoll)
{
int index = 0;
while (index < armature->collection_array_num && armature->collection_array[index] != bcoll) {

I'd write something like this:

  for (BoneCollection *arm_bcoll : armature->collection_span()) {
    if (arm_bcoll == bcoll) {
      return index;
    }
    index++;
  }
  return -1;
I'd write something like this: ```cpp for (BoneCollection *arm_bcoll : armature->collection_span()) { if (arm_bcoll == bcoll) { return index; } index++; } return -1; ```
nathanvegdahl marked this conversation as resolved
@ -155,2 +266,3 @@
BLI_insertlinkafter(&armature->collections, anchor, bcoll);
const int anchor_index = bonecoll_find_index(armature, anchor);
BLI_assert(anchor_index >= 0);

anchor can be nullptr to insert at the front of the list. This means that anchor_index can be -1. Which is fine, because then below anchor_index + 1 is actually 0 and points to the right place.

`anchor` can be `nullptr` to insert at the front of the list. This means that `anchor_index` can be `-1`. Which is fine, because then below `anchor_index + 1` is actually `0` and points to the right place.
nathanvegdahl marked this conversation as resolved
@ -224,19 +332,63 @@ bool ANIM_armature_bonecoll_is_editable(const bArmature *armature, const BoneCol
return true;
}
bool ANIM_armature_bonecoll_move_to_index(bArmature *armature, int from_index, int to_index)

const int

`const int`
nathanvegdahl marked this conversation as resolved
@ -227,0 +345,4 @@
/* Shift collections over to fill the gap at from_index and make room at to_index. */
{
BoneCollection **start = armature->collection_array + std::min(from_index, to_index);
const size_t count = std::max(from_index, to_index) - std::min(from_index, to_index);

For this entire function, it might be clearer to have:

const int start_index = std::min(...);
const int end_index = std::max(...);
const int direction = from_index < to_index ? -1 : 1;

and then use those in the rest of the code. That should make some comparisons easier, and allow for some more natural grouping of the different cases.

For this entire function, it might be clearer to have: ```cpp const int start_index = std::min(...); const int end_index = std::max(...); const int direction = from_index < to_index ? -1 : 1; ``` and then use those in the rest of the code. That should make some comparisons easier, and allow for some more natural grouping of the different cases.
nathanvegdahl marked this conversation as resolved
@ -272,3 +424,3 @@
}
BLI_remlink_safe(&armature->collections, bcoll);
bonecoll_remove(armature, bonecoll_find_index(armature, bcoll));

This is a good illustration of bonecoll_remove being a bit of a confusing name. ANIM_armature_bonecoll_remove removes from the list and frees, whereas bonecoll_remove only removes from the list.

This is a good illustration of `bonecoll_remove` being a bit of a confusing name. `ANIM_armature_bonecoll_remove` removes from the list **and** frees, whereas `bonecoll_remove` only removes from the list.
nathanvegdahl marked this conversation as resolved
@ -535,2 +682,2 @@
blender::Map<BoneCollection *, BoneCollection *> ANIM_bonecoll_listbase_copy_no_membership(
ListBase *bone_colls_dst, ListBase *bone_colls_src, const bool do_id_user)
blender::Map<BoneCollection *, BoneCollection *> ANIM_bonecoll_array_copy_no_membership(
BoneCollection ***bcoll_array_dst,

I think it might be clearer to have the function return the destination Span<BoneCollection *>(), and have the caller assign it to the approriate fields. This would introduce a struct BoneCollCopyResult or something like that, which includes the Map and that Span, so I'm not 100% convinced it'll actually improve things. I think it's worth experimenting with, as it would remove the triple pointer.

I think it might be clearer to have the function return the destination `Span<BoneCollection *>()`, and have the caller assign it to the approriate fields. This would introduce a struct `BoneCollCopyResult` or something like that, which includes the `Map` and that `Span`, so I'm not 100% convinced it'll actually improve things. I think it's worth experimenting with, as it would remove the triple pointer.
Author
Member

We discussed in another part of the review that spans aren't supposed to own data, so in retrospect I don't think it's appropriate to return an owned heap-allocated array that way. But since this approach requires a struct anyway, I can just put the array pointer and length directly in the struct.

We discussed in another part of the review that spans aren't supposed to own data, so in retrospect I don't think it's appropriate to return an owned heap-allocated array that way. But since this approach requires a struct anyway, I can just put the array pointer and length directly in the struct.
nathanvegdahl marked this conversation as resolved
@ -537,0 +683,4 @@
BoneCollection ***bcoll_array_dst,
int *bcoll_array_dst_num,
BoneCollection **bcoll_array_src,
int bcoll_array_src_num,

const int

`const int`
nathanvegdahl marked this conversation as resolved
@ -539,3 +695,3 @@
blender::Map<BoneCollection *, BoneCollection *> bcoll_map{};
LISTBASE_FOREACH (BoneCollection *, bcoll_src, bone_colls_src) {
for (int i = 0; i < *bcoll_array_dst_num; i++) {

They're the same value, so I'd choose to loop until bcoll_array_src_num here. It's part of the input paramters.

They're the same value, so I'd choose to loop until `bcoll_array_src_num` here. It's part of the input paramters.
nathanvegdahl marked this conversation as resolved
@ -63,2 +65,2 @@
LISTBASE_FOREACH_BACKWARD_MUTABLE (BoneCollection *, bcoll, &arm.collections) {
ANIM_armature_bonecoll_remove(&arm, bcoll);
while (arm.collection_array_num > 0) {
ANIM_armature_bonecoll_remove(&arm, arm.collection_array[arm.collection_array_num - 1]);

When you turn bonecoll_remove() into ANIM_armature_bonecoll_remove_from_index() (as discussed face-to-face), this can become a call to ANIM_armature_bonecoll_remove_from_index() and not be accidentally quadratic.

When you turn `bonecoll_remove()` into `ANIM_armature_bonecoll_remove_from_index()` (as discussed face-to-face), this can become a call to `ANIM_armature_bonecoll_remove_from_index()` and not be accidentally quadratic.
nathanvegdahl marked this conversation as resolved
@ -146,0 +142,4 @@
armature_dst->collection_array = static_cast<BoneCollection **>(
MEM_dupallocN(armature_src->collection_array));
armature_dst->collection_array_num = armature_src->collection_array_num;
for (int i = 0; i < armature_dst->collection_array_num; i++) {

Here I'd also use the source armature as "source of truth", and thus loop until armature_src->collection_array_num

Here I'd also use the source armature as "source of truth", and thus loop until `armature_src->collection_array_num`
nathanvegdahl marked this conversation as resolved
@ -146,0 +143,4 @@
MEM_dupallocN(armature_src->collection_array));
armature_dst->collection_array_num = armature_src->collection_array_num;
for (int i = 0; i < armature_dst->collection_array_num; i++) {
armature_dst->collection_array[i] = static_cast<BoneCollection *>(

I think the for body should be moved into a separate "duplicate bone collection" function. The "copy armature data" function is already doing too much.

I think the `for` body should be moved into a separate "duplicate bone collection" function. The "copy armature data" function is already doing too much.
nathanvegdahl marked this conversation as resolved
@ -171,1 +179,3 @@
}
if (armature->collection_array) {
for (BoneCollection *bcoll : armature->collections_span()) {
/* ID properties. */

This could be something like:

BLI_freelistN(&bcoll->bones);
ANIM_bonecoll_free(bcoll);
This could be something like: ```cpp BLI_freelistN(&bcoll->bones); ANIM_bonecoll_free(bcoll); ```
nathanvegdahl marked this conversation as resolved
@ -303,0 +333,4 @@
/* Restore the BoneCollection array and clear the listbase. */
arm->collection_array = collection_array_backup;
if (arm->collection_array_num > 0) {

The if can be removed.

The `if` can be removed.
nathanvegdahl marked this conversation as resolved
@ -336,0 +384,4 @@
arm->collection_array_num, sizeof(BoneCollection *), __func__);
{
int i = 0;
LISTBASE_FOREACH (BoneCollection *, bcoll, &arm->collections) {

Use LISTBASE_FOREACH_INDEX instead.

Use `LISTBASE_FOREACH_INDEX` instead.
nathanvegdahl marked this conversation as resolved
@ -3043,0 +3111,4 @@
blender::Span<const BoneCollection *> bArmature::collections_span() const
{
if (collection_array == nullptr || collection_array_num <= 0) {

No need for the if.

No need for the `if`.
nathanvegdahl marked this conversation as resolved
@ -74,3 +74,3 @@
char active_collection_name[MAX_NAME];
ListBase /* EditBone */ ebones;
ListBase /* BoneCollection */ bone_collections;
BoneCollection **collection_array;

Let's make this a Vector<BoneCollection> instead. That way ANIM_bonecoll_array_copy_no_membership can just return its map + that vector?

Let's make this a `Vector<BoneCollection>` instead. That way `ANIM_bonecoll_array_copy_no_membership` can just return its map + that vector?
Author
Member

So, I thought about this more, and since we also use ANIM_bonecoll_array_copy_no_membership to copy from the undo stack back to the actual armature (which doesn't use a Vector), I think we'd just be trading gnarly code for gnarly code if we do it that way. Because we'd then need to manually extract the pointer and length from the Vector to store it in the armature DNA struct, and also make sure it doesn't run its destructor and free the memory.

So, I thought about this more, and since we also use `ANIM_bonecoll_array_copy_no_membership` to copy from the undo stack back to the actual armature (which doesn't use a `Vector`), I think we'd just be trading gnarly code for gnarly code if we do it that way. Because we'd then need to manually extract the pointer and length from the `Vector` to store it in the armature DNA struct, and also make sure it doesn't run its destructor and free the memory.

Bah, you make a good point. Let's keep the code as-is, then.

Bah, you make a good point. Let's keep the code as-is, then.
nathanvegdahl marked this conversation as resolved
@ -143,3 +150,3 @@
BLI_listbase_count(&ebone->bone_collections);
}
uarm->undo_size += sizeof(BoneCollection) * BLI_listbase_count(&uarm->bone_collections);
uarm->undo_size += (sizeof(BoneCollection) + sizeof(BoneCollection *)) *

This should be commented.

This should be commented.
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl force-pushed hierarchical_bone_collections from 5aa0e367a5 to ddb6024183 2023-12-04 17:19:59 +01:00 Compare
Nathan Vegdahl force-pushed hierarchical_bone_collections from 21e93734a5 to 4cbfa1fc37 2023-12-04 18:38:35 +01:00 Compare
Nathan Vegdahl force-pushed hierarchical_bone_collections from 4cbfa1fc37 to 0774a9aa11 2023-12-04 18:40:34 +01:00 Compare
Nathan Vegdahl force-pushed hierarchical_bone_collections from 0774a9aa11 to 8244b3a093 2023-12-04 18:45:38 +01:00 Compare
Sybren A. Stüvel approved these changes 2023-12-05 10:32:24 +01:00
Sybren A. Stüvel left a comment
Member

LGTM!

For a future commit: there are a blocks of code where a subset of the array is shifted in a certain direction. It might be interesting to extract this into a function of its own, as I think the hierarchical bone collections will make use of this as well.

LGTM! For a future commit: there are a blocks of code where a subset of the array is shifted in a certain direction. It might be interesting to extract this into a function of its own, as I think the hierarchical bone collections will make use of this as well.
Nathan Vegdahl merged commit 9e7a70d6c6 into main 2023-12-05 10:39:58 +01:00
Nathan Vegdahl deleted branch hierarchical_bone_collections 2023-12-05 10:39:59 +01:00
Alexander Gavrilov reviewed 2023-12-28 21:01:04 +01:00
@ -344,3 +341,1 @@
bone_collection_by_name.add(bcoll->name, bcoll);
mapped = bcoll;
BoneCollection *new_bcoll = ANIM_armature_bonecoll_new(arm, bcoll->name);

Well, this does result in user-visible changes: it breaks Rigify because join now loses custom properties.

Well, this does result in user-visible changes: it breaks Rigify because join now loses custom properties.

This was resolved in 34b04b625c.

This was resolved in 34b04b625ce.
dr.sybren marked this conversation as resolved
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 project
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#115354
No description provided.