Show Action Slots (Bindings) in the Action editor #122672

Merged
Sybren A. Stüvel merged 1 commits from dr.sybren/blender:anim/baklava-action-editor-bindings into main 2024-07-04 14:44:37 +02:00

Note on 'Binding' vs 'Slot': this PR was created before the decision to change the name from 'Bindings' to 'Slots'. To avoid confusion, and to keep this PR in sync with the code changes, it still uses the term 'binding'. Renames of 'Binding' to 'Slot' in the code will happen after this PR lands.


In the Action mode of the Dope Sheet, show the Bindings, optionally showing all Bindings in that Action.

image

This also works when different data-block types are animated from the same Action:

image

Note that this requires that each Binding is actually used by a data-block. When this is not the case, only simple properties will be resolved, so in the above example, "Focal Length" would be fine (because it's known that it's an ID_CA type and thus we can look up the property). Any RNA path that contains a period is shown as-is, so 'F-Stop (Depth of Field)' would just be shown as dof.aperture_fstop[0]. How to represent these situations is for another PR, as I first want to land this "basic" functionality.

Once #122500 has landed too, there's more control in the user interface over Action & Binding assignments of non-object data-blocks, and then we can look more at how to handle unassigned bindings, Action duplication, etc.

Also the visual indentation in the channels list is not to my liking, but IMO something we should look at once we implement F-Curve groups. For now, the layout is acceptable.

Finally, the 'link' icon is just a placeholder for actually having a proper icon for the bindings.

**Note on 'Binding' vs 'Slot':** this PR was created before [the decision to change the name from 'Bindings' to 'Slots'](https://devtalk.blender.org/t/2024-06-27-animation-rigging-module-meeting/35328). To avoid confusion, and to keep this PR in sync with the code changes, it still uses the term 'binding'. Renames of 'Binding' to 'Slot' in the code will happen after this PR lands. ---------- In the Action mode of the Dope Sheet, show the Bindings, optionally showing all Bindings in that Action. ![image](/attachments/8fd007a0-2dc8-4534-a26f-83f8f303f8a6) This also works when different data-block types are animated from the same Action: ![image](/attachments/e4300164-0078-42d1-a729-fd3b23a79bfc) Note that this requires that each Binding is actually used by a data-block. When this is not the case, only simple properties will be resolved, so in the above example, "Focal Length" would be fine (because it's known that it's an `ID_CA` type and thus we can look up the property). Any RNA path that contains a period is shown as-is, so 'F-Stop (Depth of Field)' would just be shown as `dof.aperture_fstop[0]`. How to represent these situations is for another PR, as I first want to land this "basic" functionality. Once #122500 has landed too, there's more control in the user interface over Action & Binding assignments of non-object data-blocks, and then we can look more at how to handle unassigned bindings, Action duplication, etc. Also the visual indentation in the channels list is not to my liking, but IMO something we should look at once we implement F-Curve groups. For now, the layout is acceptable. Finally, the 'link' icon is just a placeholder for actually having a proper icon for the bindings.
Sybren A. Stüvel added 5 commits 2024-06-03 17:58:04 +02:00
Sybren A. Stüvel added 1 commit 2024-06-04 16:04:27 +02:00
Sybren A. Stüvel added 3 commits 2024-06-06 17:01:58 +02:00
Use the binding's display name in the dope sheet channel list, rather than
its internal name.
The anim filtering code for bindings was not respecting the
`ANIMFILTER_FCURVESONLY` flag, and thus always returning an
`bAnimListElem` for the binding itself.

Now, if the flag is there, it ignores the binding's expansion state, and
then always produces elements for the FCurves for that binding.
Sybren A. Stüvel added 1 commit 2024-06-07 12:49:57 +02:00
Sybren A. Stüvel added 4 commits 2024-06-07 13:00:35 +02:00
Move the `bAnimListElem` struct down in its header file, so that it sits
underneath the `enum` types that it uses. This move is necessary for an
upcoming refactor where the struct actually declares its fields to be of
those enum types.

Separating the move from the actual change in type will help with
clarity in the diffs.

No functional changes.
Refactor: Anim: make some bAnimListElem fields enum-typed
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
9428456ac7
Change the `bAnimListElem` fields `type`, `update`, and `datatype` from
respectively `int`, `char`, and `short` to their actual `enum` types,
respectively `eAnim_ChannelType`, `eAnim_Update_Flags`, and `eAnim_KeyType`.

To prevent compiler warnings, all `switch` statements that try to handle
these fields have been expanded with the missing `case`s. This should help
in the future when new channel types are added, as that'll cause more
compiler warnings in those places that need updating.

One `if`/`else if`/`else` chain was converted into a `switch` for clarity.
This was actually the place where my (upcoming) code is hitting the
`BLI_assert_unreachable()`, which was the final straw for this refactor.

No functional changes.
# Conflicts:
#	source/blender/editors/include/ED_anim_api.hh
No functional changes, just avoiding compiler warnings and adding TODO
comments.
Sybren A. Stüvel added 3 commits 2024-06-07 15:25:31 +02:00
Sybren A. Stüvel added 1 commit 2024-06-07 15:39:48 +02:00
Sybren A. Stüvel added 3 commits 2024-06-07 16:01:13 +02:00
Sybren A. Stüvel added the
Module
Animation & Rigging
label 2024-06-10 10:48:52 +02:00
Sybren A. Stüvel added 2 commits 2024-06-11 12:08:12 +02:00
Sybren A. Stüvel added 1 commit 2024-06-11 14:29:24 +02:00
Sybren A. Stüvel added 5 commits 2024-06-13 16:30:39 +02:00
Anim: add ID* cache of users to Action Bindings
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
4ead1a001b
Keep track of which IDs are animated by which Action Binding. This will be
necessary for display in the Action editor, where animation data that is
unrelated to the active object can be shown (when "show all bindings" is
on).

Note: animation evaluation will not be using this cache, at least not in
the near future. Potentially when we introduce animation-level constraints
this will change, but that's for the future.

--------------

This also adds an RNA function `action.bindings["name"].debug_log_users()`,
which can be used to print the list of users of that binding to stdout.
This is purely for debugging/testing this PR, and will be removed before
landing on `main`.

The user cache isn't actually used in this PR. It will be used soon in
#122672. That in itself is a big enough PR that I thought this aspect would
be nice to have as a separate one.
Use the users cache introduced in #123187 to look up the F-Curve names.
Sybren A. Stüvel added 2 commits 2024-06-14 09:17:34 +02:00
Sybren A. Stüvel added 1 commit 2024-06-14 09:29:45 +02:00
Action Editor: use name_display RNA property to edit a binding name
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
8831db2452
Sybren A. Stüvel requested review from Nathan Vegdahl 2024-06-14 10:31:52 +02:00
Sybren A. Stüvel requested review from Christoph Lendenfeld 2024-06-14 10:32:02 +02:00
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel changed title from WIP: show Bindings in the Action editor to show Bindings in the Action editor 2024-06-14 10:35:23 +02:00

@dr.sybren There is an issue in the "Dope Sheet" mode. Bindings are shown even though the parent entry is collapsed. That leads to really confusing behavior, because the binding will show as empty as long as the parent isn't expanded

@dr.sybren There is an issue in the "Dope Sheet" mode. Bindings are shown even though the parent entry is collapsed. That leads to really confusing behavior, because the binding will show as empty as long as the parent isn't expanded
Nathan Vegdahl requested changes 2024-06-14 14:31:29 +02:00
Dismissed
Nathan Vegdahl left a comment
Member

Looking good so far. Just some suggestions that I think could help people when reading the code.

Looking good so far. Just some suggestions that I think could help people when reading the code.
@ -975,7 +971,34 @@ static bAnimChannelType ACF_GROUP = {
/* name for fcurve entries */
static void acf_fcurve_name(bAnimListElem *ale, char *name)
Member

Definitely not for this PR, but just noting since I thought of it now: at some point it would be good to put these functions in a namespace like channel_filter so we can drop the cryptic acf_ prefix.

Definitely not for this PR, but just noting since I thought of it now: at some point it would be good to put these functions in a namespace like `channel_filter` so we can drop the cryptic `acf_` prefix.
Author
Member

I think we'd better just drop the whole thing and implement something better ;-)

I think we'd better just drop the whole thing and implement something better ;-)
dr.sybren marked this conversation as resolved
@ -1336,3 +1359,3 @@
};
#endif
static void acf_action_binding_name(bAnimListElem *ale, char *name)
Member

Should the name parameter be r_name?

Should the name parameter be `r_name`?
Author
Member

It's a step forward. The rest of the code just uses name, and I agree it's yet another layer of confusion in this area of the code.

It's a step forward. The rest of the code just uses `name`, and I agree it's yet another layer of confusion in this area of the code.
dr.sybren marked this conversation as resolved
@ -1301,6 +1311,7 @@ static size_t animfilter_fcurves(ListBase *anim_data,
static size_t animfilter_fcurves_span(ListBase * /*bAnimListElem*/ anim_data,
Member

I realize this is a preexisting function, but I have no idea what this does from the function name, parameters, and return type. Might be worth adding even just a one-liner doc comment.

(I will now proceed to read the code to figure out what it does.)

I realize this is a preexisting function, but I have no idea what this does from the function name, parameters, and return type. Might be worth adding even just a one-liner doc comment. (I will now proceed to read the code to figure out what it does.)
dr.sybren marked this conversation as resolved
@ -432,0 +433,4 @@
animrig::Action *action = static_cast<animrig::Action *>(ale->key_data);
BLI_assert(action);
animrig::Binding *binding = static_cast<animrig::Binding *>(ale->data);
return anim_keyframes_loop(ked, *action, binding, key_ok, key_cb, fcu_cb);
Member

Would now be a good time to rename this to something like act_layered_keyframes_loop(), as well as the corresponding function for legacy actions?

Would now be a good time to rename this to something like `act_layered_keyframes_loop()`, as well as the corresponding function for legacy actions?
Author
Member

Good point. I don't like the shortening of action to act, so I'll go for action_layered_keyframes_loop.

Good point. I don't like the shortening of `action` to `act`, so I'll go for `action_layered_keyframes_loop`.
Member

Looks good. But I should have been clearer: I also meant for act_keyframes_loop() below to get renamed accordingly. E.g. action_legacy_keyframes_loop().

Looks good. But I should have been clearer: I also meant for `act_keyframes_loop()` below to get renamed accordingly. E.g. `action_legacy_keyframes_loop()`.
dr.sybren marked this conversation as resolved
Sybren A. Stüvel added 6 commits 2024-06-14 15:02:35 +02:00
Add a debug print for ease of reviewing, will remove before landing
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
51f59529d3
Fix test failures by setting G.main in unit tests
Some checks reported errors
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
a3e796e17a
Rename Binding::runtime_allocate() to Binding::blend_read_post()
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
9d7f34636c
Sybren A. Stüvel added 3 commits 2024-06-14 15:54:26 +02:00
Rename anim_keyframes_loopaction_layered_keyframes_loop
Some checks failed
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
a7e46ecb1e
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel requested review from Nathan Vegdahl 2024-06-14 15:54:46 +02:00
Nathan Vegdahl requested changes 2024-06-14 16:24:45 +02:00
Dismissed
Nathan Vegdahl left a comment
Member

I wasn't clear enough in one of my comments, and thus you didn't fully address it. More in the comment thread itself.

I wasn't clear enough in one of my comments, and thus you didn't fully address it. More in the comment thread itself.
Sybren A. Stüvel added 23 commits 2024-06-18 17:49:26 +02:00
Refactor: Anim filtering, avoid passing bDopesheet *ads parameter
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
736d391818
In the animation filtering code, avoid passing `bDopesheet *ads` as a
separate parameter. Instead, pass `bAnimContext *ac`. This already
contains the same pointer. Some carefully placed asserts help to verify
that these pointers were indeed the same.

Not only do I think this approach is cleaner, it is a necessary step for
upcoming PRs #123187 and #122672. Those will need a `Main *bmain`
pointer, which is already available in the `bAnimContext` struct.

No functional changes.
The only use of `G_MAIN` is in `BKE_animdata_free()`. I couldn't find a
clean place to otherwise clear this flag.
entires → entries
Some checks failed
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
af7c4bbfa3
Merge remote-tracking branch 'origin/main' into anim/baklava-bindings-to-id-cache
Some checks failed
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
589b701eac
Merge branch 'main' into anim/baklava-bindings-to-id-cache
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
6e80fb0fe8
Hide more behind #ifdef WITH_ANIM_BAKLAVA
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
63e484da3c
Add missing assignment to G_TEST in a unit test
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
d017821be1
Refactor: simplify make_new_animlistelem() by returning early
All checks were successful
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
21db70d5dc
No functional changes. Just avoiding having the entire function in a
condition.
Sybren A. Stüvel added 1 commit 2024-06-18 17:59:16 +02:00
Merge remote-tracking branch 'origin/main' into anim/baklava-action-editor-bindings
Some checks failed
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
d23c847a80
Author
Member

@dr.sybren There is an issue in the "Dope Sheet" mode. Bindings are shown even though the parent entry is collapsed. That leads to really confusing behavior, because the binding will show as empty as long as the parent isn't expanded

@ChrisLend With all the recent changes to this PR, can you still reproduce this?

> @dr.sybren There is an issue in the "Dope Sheet" mode. Bindings are shown even though the parent entry is collapsed. That leads to really confusing behavior, because the binding will show as empty as long as the parent isn't expanded @ChrisLend With all the recent changes to this PR, can you still reproduce this?
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel added 1 commit 2024-06-18 18:05:05 +02:00
Sybren A. Stüvel added 1 commit 2024-06-19 09:56:41 +02:00
Fix unittests
All checks were successful
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
0bf06f6180
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel force-pushed anim/baklava-action-editor-bindings from ed3c4581b9 to 0bf06f6180 2024-06-19 11:20:22 +02:00 Compare
Sybren A. Stüvel requested review from Nathan Vegdahl 2024-06-19 12:34:15 +02:00

@dr.sybren I can, yes.
image

@dr.sybren I can, yes. ![image](/attachments/8b423685-41e2-40ab-a964-32e482e81e19)
7.5 KiB
Sybren A. Stüvel added 4 commits 2024-06-20 11:58:46 +02:00
Remove unintended formatting change
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
95f4ca407b
Sybren A. Stüvel added 2 commits 2024-06-20 14:32:05 +02:00
Author
Member

There is an issue in the "Dope Sheet" mode. Bindings are shown even though the parent entry is collapsed. That leads to really confusing behavior, because the binding will show as empty as long as the parent isn't expanded

Solved, by not showing Bindings in Dope Sheet mode at all. The F-Curves in the Dope Sheet are shown in context of their animated ID anyway, so I think showing Bindings there is a bit of a waste of vertical space.

So now Bindings are only shown in the Action editor, because that's the only place where F-Curves for those Bindings are shown without their animated ID.

> There is an issue in the "Dope Sheet" mode. Bindings are shown even though the parent entry is collapsed. That leads to really confusing behavior, because the binding will show as empty as long as the parent isn't expanded Solved, by not showing Bindings in Dope Sheet mode at all. The F-Curves in the Dope Sheet are shown in context of their animated ID anyway, so I think showing Bindings there is a bit of a waste of vertical space. So now Bindings are only shown in the Action editor, because that's the only place where F-Curves for those Bindings are shown without their animated ID.
Sybren A. Stüvel changed title from show Bindings in the Action editor to Show Action Slots in the Action editor 2024-07-01 12:33:46 +02:00
Sybren A. Stüvel changed title from Show Action Slots in the Action editor to Show Action Slots (Bindings) in the Action editor 2024-07-01 12:36:12 +02:00
Sybren A. Stüvel force-pushed anim/baklava-action-editor-bindings from 2a7557e770 to dc8cac5f1a 2024-07-01 12:46:17 +02:00 Compare
Sybren A. Stüvel force-pushed anim/baklava-action-editor-bindings from dc8cac5f1a to 9c5c856b80 2024-07-01 14:01:43 +02:00 Compare
Sybren A. Stüvel added 2 commits 2024-07-01 14:11:09 +02:00
It's not a nice macro, but still better to use it where possible to be
more uniform with the rest of the code.
Sybren A. Stüvel added 1 commit 2024-07-01 14:39:01 +02:00
Sybren A. Stüvel changed title from Show Action Slots (Bindings) in the Action editor to WIP: Show Action Slots (Bindings) in the Action editor 2024-07-01 14:50:26 +02:00
Sybren A. Stüvel added 2 commits 2024-07-01 15:28:58 +02:00
Sybren A. Stüvel added 1 commit 2024-07-01 15:52:12 +02:00
Sybren A. Stüvel added 1 commit 2024-07-01 17:36:16 +02:00
Sybren A. Stüvel added 1 commit 2024-07-01 17:40:48 +02:00
Sybren A. Stüvel changed title from WIP: Show Action Slots (Bindings) in the Action editor to Show Action Slots (Bindings) in the Action editor 2024-07-01 17:42:07 +02:00
Christoph Lendenfeld requested changes 2024-07-02 10:31:33 +02:00
Dismissed
Christoph Lendenfeld left a comment
Member

I think the FCurve channels should keep a bit of left indent.
This looks like a mistake and is also different to the legacy action.
image

I just noticed that FCurve groups are not drawn with layered actions. I am not against that as I think we could find a better solution, I am just wondering if that was intentional

I think the FCurve channels should keep a bit of left indent. This looks like a mistake and is also different to the legacy action. ![image](/attachments/cd53b323-5784-4479-8171-d4f0dd62dd91) I just noticed that FCurve groups are not drawn with layered actions. I am not against that as I think we could find a better solution, I am just wondering if that was intentional
@ -1339,0 +1369,4 @@
BLI_strncpy(r_name, binding->name_without_prefix().c_str(), ANIM_CHAN_NAME_SIZE);
}
static bool acf_action_binding_name_prop(bAnimListElem *ale,

I think you need to run the formatter again

I think you need to run the formatter again
@ -1139,1 +1153,4 @@
/** \see #blender::animrig::Binding::flags() */
int8_t binding_flags;
uint8_t _pad1[3];

is that the new standard to do padding instead of char ?

Edit: that was an old comment that I couldn't remove because the code was no longer there

is that the new standard to do padding instead of `char` ? Edit: that was an old comment that I couldn't remove because the code was no longer there
ChrisLend marked this conversation as resolved
Nathan Vegdahl requested changes 2024-07-02 16:16:04 +02:00
Dismissed
Nathan Vegdahl left a comment
Member

Mostly looks good to me. Just some nits and questions.

Mostly looks good to me. Just some nits and questions.
@ -487,0 +488,4 @@
enum class Flags : uint8_t {
Expanded = (1 << 0), /** Expanded/collapsed in animation editors. */
Selected = (1 << 1), /** Selected in animation editors. */
/* When adding/removing a flag, also update the ENUM_OPERATORS() invocation below. */
Member

Nit: maybe "further below", or something else to help indicate that it's quite a ways away. My initial gut reaction was that it would be just below along with the flag methods, and it took me a bit to reorient and find it a page or so down.

Nit: maybe "further below", or something else to help indicate that it's quite a ways away. My initial gut reaction was that it would be just below along with the flag methods, and it took me a bit to reorient and find it a page or so down.
  • Move comments on separate lines.
+ Move comments on separate lines.
dr.sybren marked this conversation as resolved
@ -250,6 +250,10 @@ Binding *Action::binding_for_handle(const binding_handle_t handle)
const Binding *Action::binding_for_handle(const binding_handle_t handle) const
{
if (handle == Binding::unassigned) {
Member

Just double-checking: this is just an optimization, right? If so, looks good. If not, a comment would be good.

Just double-checking: this is just an optimization, right? If so, looks good. If not, a comment would be good.
Author
Member

It is, indeed. It's not necessary at all for the functionality, it's more of a mental optimisation to remind people (mostly myself) that it's ok to call this function with Binding::unassigned as parameter. I'll also write that in the function's documentation.

It is, indeed. It's not necessary at all for the functionality, it's more of a mental optimisation to remind people (mostly myself) that it's ok to call this function with `Binding::unassigned` as parameter. I'll also write that in the function's documentation.
dr.sybren marked this conversation as resolved
@ -1339,0 +1363,4 @@
{
const animrig::Binding *binding = static_cast<animrig::Binding *>(ale->data);
if (!binding) {
BLI_strncpy(r_name, "-nil-", ANIM_CHAN_NAME_SIZE);
Member

When does this happen? Should this have an assert?

When does this happen? Should this have an assert?
dr.sybren marked this conversation as resolved
@ -40,2 +46,4 @@
/* Handle some nullptr cases. */
if (name == nullptr) {
/* A 'get name' function should be able to get the name, otherwise it's a bug. */
BLI_assert_unreachable();
Member

👍👍👍👍👍👍👍👍👍👍

👍👍👍👍👍👍👍👍👍👍
dr.sybren marked this conversation as resolved
@ -1189,0 +1201,4 @@
action_bindinghandle_to_keylist(adt, action, adt->binding_handle, keylist, saction_flag, range);
}
void action_binding_to_keylist(AnimData *adt,
Member

Is this function useful/necessary? It seems redundant with action_bindinghandle_to_keylist() since the parameters are identical except for taking a Binding handle rather than a Binding. But that one difference is trivial to handle at the call sites by just passing binding.handle as seen in the implementation here.

Is this function useful/necessary? It seems redundant with `action_bindinghandle_to_keylist()` since the parameters are identical except for taking a Binding handle rather than a Binding. But that one difference is trivial to handle at the call sites by just passing `binding.handle` as seen in the implementation here.
dr.sybren marked this conversation as resolved
@ -881,6 +898,10 @@ bool ANIM_fmodifiers_paste_from_buf(ListBase *modifiers, bool replace, FCurve *c
*/
int getname_anim_fcurve(char *name, ID *id, FCurve *fcu);
std::string getname_anim_fcurve_bound(Main &bmain,
Member

A doc comment would be good (just a short-and-sweet one is fine). From just the name/signature and without the context of the larger PR I don't think its purpose/behavior would be clear to me, at least.

A doc comment would be good (just a short-and-sweet one is fine). From just the name/signature and without the context of the larger PR I don't think its purpose/behavior would be clear to me, at least.
dr.sybren marked this conversation as resolved
Author
Member

@ChrisLend wrote:
I think the FCurve channels should keep a bit of left indent.
This looks like a mistake and is also different to the legacy action.

Yup, that's why the PR description includes:

Also the visual indentation in the channels list is not to my liking, but IMO something we should look at once we implement F-Curve groups. For now, the layout is acceptable.

@ChrisLend wrote:
I just noticed that FCurve groups are not drawn with layered actions. I am not against that as I think we could find a better solution, I am just wondering if that was intentional

Yup, that's for later. The data model doesn't even have F-Curve groups. The way they were implemented before was hackish, and I don't want to repeat that. Basically because of the structure of ListBase, any struct can only be in a single list (because it embeds its own prev and next pointers). This means that you cannot have an Action-global list of F-Curves and a separate list for the F-Curves in the Action Group. So the Action Group basically has its first and last pointers point to the F-Curves in the global list, and you have to make sure that when iterating you stop at group.last instead of stopping at fcurve.next == nullptr. My plan is to go for a similar structure as the hierarchical Bone Collections, but that's for a future PR.

> @ChrisLend wrote: > I think the FCurve channels should keep a bit of left indent. > This looks like a mistake and is also different to the legacy action. Yup, that's why the PR description includes: >Also the visual indentation in the channels list is not to my liking, but IMO something we should look at once we implement F-Curve groups. For now, the layout is acceptable. > @ChrisLend wrote: > I just noticed that FCurve groups are not drawn with layered actions. I am not against that as I think we could find a better solution, I am just wondering if that was intentional Yup, that's for later. The data model doesn't even have F-Curve groups. The way they were implemented before was hackish, and I don't want to repeat that. Basically because of the structure of `ListBase`, any struct can only be in a single list (because it embeds its own `prev` and `next` pointers). This means that you cannot have an Action-global list of F-Curves _and_ a separate list for the F-Curves in the Action Group. So the Action Group basically has its `first` and `last` pointers point to the F-Curves in the global list, and you have to make sure that when iterating you stop at `group.last` instead of stopping at `fcurve.next == nullptr`. My plan is to go for a similar structure as the hierarchical Bone Collections, but that's for a future PR.
Christoph Lendenfeld approved these changes 2024-07-04 11:01:16 +02:00
Christoph Lendenfeld left a comment
Member

@dr.sybren in that case the PR is ready to land from my point of view
sorry missed the section in the PR description that mentions the indent

@dr.sybren in that case the PR is ready to land from my point of view sorry missed the section in the PR description that mentions the indent
Sybren A. Stüvel added 6 commits 2024-07-04 11:19:07 +02:00
Sybren A. Stüvel requested review from Nathan Vegdahl 2024-07-04 11:21:09 +02:00
Nathan Vegdahl approved these changes 2024-07-04 11:24:02 +02:00
Nathan Vegdahl left a comment
Member

Looks good to me! Let's get this thing landed! :-)

Looks good to me! Let's get this thing landed! :-)
Sybren A. Stüvel force-pushed anim/baklava-action-editor-bindings from 0fd4ff1582 to e641d01cd4 2024-07-04 11:51:30 +02:00 Compare
Author
Member

I just force-pushed the squashed commit, just so that I can give it one last test on the buildbot.

I just force-pushed the squashed commit, just so that I can give it one last test on the buildbot.
Sybren A. Stüvel force-pushed anim/baklava-action-editor-bindings from e641d01cd4 to 82c89fe77a 2024-07-04 11:52:27 +02:00 Compare
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel merged commit 1d2cea1e3e into main 2024-07-04 14:44:37 +02:00
Sybren A. Stüvel deleted branch anim/baklava-action-editor-bindings 2024-07-04 14:44:45 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
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
Asset Browser Project
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#122672
No description provided.