Anim: move ANIM_... bone collection functions into C++ style, such as the blender::animrig namespace #116938

New Issue

Sybren A. Stüvel · 2024-01-09T12:39:26+01:00

Sybren A. Stüvel commented

2024-01-09 12:39:26 +01:00

When bone collections were introduced, they replaced armature layers. The latter had an older C-style API, which translated into the bone collections following a similar approach. Now that much more of Blender's code has been moved to C++, a few refactors are in order:

Some of these are marked as 'OPTIONAL', as they need more discussion & better view of where we are going with the C++ style code.

OPTIONAL: Create a separate C++ wrapper for bone collections. This would follow the approach we have for the BoneColor struct, where there is an explicit distinction between the C DNA struct and its C++ wrapper class. Currently the DNA BoneCollection struct has some C++ methods on it, directly. Because of that, any alteration to its C++ interface requires recompiling everything that (indirectly) includes DNA_armature_types.h, which can get cumbersome during development. The downside is that there's then a difference between ::BoneCollection (DNA) and blender::animrig::BoneCollection (C++ wrapper).
Clean up some of the ANIM_… functions that were created during the transition from animation layers to bone collections. Now that the latter are more mature, we may not need all of those, or they could use some renaming to clarify their functionality.
Move some more querying functions into the C++ BoneCollection class. I think currently only bonecoll_has_children() could move to BoneCollection::has_children(), as the other functions all require access to the armature as well.
Wherever possible, move the ANIM_… functions to the blender::animrig namespace and drop their ANIM_ prefix.
Double-check that all functions that require the armature as well are named armature_bonecoll_….
OPTIONAL: Create a separate C++ wrapper for bArmature, which could then receive all the armature_bonecoll_… functions as bArmature::bonecoll_… methods.

When bone collections were introduced, they replaced armature layers. The latter had an older C-style API, which translated into the bone collections following a similar approach. Now that much more of Blender's code has been moved to C++, a few refactors are in order: Some of these are marked as 'OPTIONAL', as they need more discussion & better view of where we are going with the C++ style code. - [ ] OPTIONAL: Create a separate C++ wrapper for bone collections. This would follow the approach we have for the `BoneColor` struct, where there is an explicit distinction between the C DNA struct and its C++ wrapper class. Currently the DNA `BoneCollection` struct has some C++ methods on it, directly. Because of that, any alteration to its C++ interface requires recompiling everything that (indirectly) includes `DNA_armature_types.h`, which can get cumbersome during development. The downside is that there's then a difference between `::BoneCollection` (DNA) and `blender::animrig::BoneCollection` (C++ wrapper). - [ ] Clean up some of the `ANIM_…` functions that were created during the transition from animation layers to bone collections. Now that the latter are more mature, we may not need all of those, or they could use some renaming to clarify their functionality. - [ ] Move some more querying functions into the C++ `BoneCollection` class. I think currently only `bonecoll_has_children()` could move to `BoneCollection::has_children()`, as the other functions all require access to the armature as well. - [ ] Wherever possible, move the ` ANIM_…` functions to the `blender::animrig` namespace and drop their `ANIM_` prefix. - [ ] Double-check that all functions that require the armature as well are named `armature_bonecoll_…`. - [ ] OPTIONAL: Create a separate C++ wrapper for `bArmature`, which could then receive all the `armature_bonecoll_…` functions as `bArmature::bonecoll_…` methods.

👍 1

Sybren A. Stüvel added the

labels 2024-01-09 12:39:26 +01:00

Sybren A. Stüvel self-assigned this 2024-01-09 12:39:26 +01:00

Nathan Vegdahl commented

2024-01-11 12:53:18 +01:00

This all looks pretty sensible to me.

OPTIONAL: Create a separate C++ wrapper for bone collections.
[...]
OPTIONAL: Create a separate C++ wrapper for bArmature

I'm on the fence about the benefits of this. If a type has a single clear representation, that makes things simpler and easier to think about. But I do agree that the compile time hit of putting methods directly on the DNA types is obnoxious.

If C++ wrappers do end up being how we deal with this kind of thing in Blender, I think we should settle on one canonical way to do it across the whole code base with uniform naming schemes, and document that pattern so that it's clear to anyone diving into the code base.

(Side note that's beyond the scope of this issue: I doubt there's much disagreement here, but ideally I'd like to see some level of C++-ification of DNA itself. For example, being limited to C in DNA is why we had to essentially write our own duplicate Vector code for bone collections, rather than just using the existing BLI Vector implementation.)

This all looks pretty sensible to me. > OPTIONAL: Create a separate C++ wrapper for bone collections. > [...] > OPTIONAL: Create a separate C++ wrapper for bArmature I'm on the fence about the benefits of this. If a type has a single clear representation, that makes things simpler and easier to think about. But I do agree that the compile time hit of putting methods directly on the DNA types is obnoxious. If C++ wrappers do end up being how we deal with this kind of thing in Blender, I think we should settle on one canonical way to do it across the whole code base with uniform naming schemes, and document that pattern so that it's clear to anyone diving into the code base. (Side note that's beyond the scope of this issue: I doubt there's much disagreement here, but ideally I'd like to see some level of [C++-ification of DNA itself](https://devtalk.blender.org/t/dna-decentralization-and-c/30391). For example, being limited to C in DNA is why we had to essentially write our own duplicate Vector code for bone collections, rather than just using the existing BLI `Vector` implementation.)

Sybren A. Stüvel commented

2024-01-12 11:55:24 +01:00

If C++ wrappers do end up being how we deal with this kind of thing in Blender, I think we should settle on one canonical way to do it across the whole code base with uniform naming schemes, and document that pattern so that it's clear to anyone diving into the code base.

Of course. This discussion here is a direct result of that not having happened yet.

I discussed this face-to-face with Sergey, and he is in favour of having explicit C++ wrappers. They give a single, clear distinction between "the C/DNA world" and "the C++ world". So as far as I'm concerned that's settled, and we just go for these C++ wrappers (like already in place for BoneColor).

> If C++ wrappers do end up being how we deal with this kind of thing in Blender, I think we should settle on one canonical way to do it across the whole code base with uniform naming schemes, and document that pattern so that it's clear to anyone diving into the code base. Of course. This discussion here is a direct result of that not having happened yet. I discussed this face-to-face with Sergey, and he is in favour of having explicit C++ wrappers. They give a single, clear distinction between "the C/DNA world" and "the C++ world". So as far as I'm concerned that's settled, and we just go for these C++ wrappers (like already in place for `BoneColor`).

👍 1

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Anim: move ANIM_... bone collection functions into C++ style, such as the blender::animrig namespace #116938

Anim: move `ANIM_...` bone collection functions into C++ style, such as the `blender::animrig` namespace #116938