WIP: Anim: Baklava, add Animation data-block to anim filtering code #119875

Closed
Sybren A. Stüvel wants to merge 4 commits from dr.sybren:pr/baklava-animation-filtering into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

Add support for the Animation data-block to the anim filtering code. This adds support to the Dope Sheet and Graph Editor.

Marked as WIP because #118677 needs to land in main first. This PR contains the not-yet-landed commits from that PR as well.

Add support for the `Animation` data-block to the anim filtering code. This adds support to the Dope Sheet and Graph Editor. Marked as WIP because #118677 needs to land in `main` first. This PR contains the not-yet-landed commits from that PR as well.
Sybren A. Stüvel added the
Module
Animation & Rigging
label 2024-03-25 12:45:04 +01:00
Sybren A. Stüvel force-pushed pr/baklava-animation-filtering from be4eb400ff to 80c8778632 2024-03-25 15:37:49 +01:00 Compare
Sybren A. Stüvel force-pushed pr/baklava-animation-filtering from 4424a4bee4 to 9399e1158c 2024-03-28 16:55:47 +01:00 Compare
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR119875) when ready.
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR119875) when ready.
Sybren A. Stüvel force-pushed pr/baklava-animation-filtering from dfef9e8f99 to 20df792ab3 2024-03-29 16:36:15 +01:00 Compare
Nathan Vegdahl requested changes 2024-04-04 17:51:29 +02:00
Nathan Vegdahl left a comment
Member

Not done reviewing, but here are the comments I have so far.

Not done reviewing, but here are the comments I have so far.
@ -198,0 +336,4 @@
/**
* Return the name prefix for the Binding's type.
*
* This is the ID name prefix, so "OB" for objects, "CA" for cameras, etc.
Member

Is the "??" prefix for "unknown/not-yet-decided" a common pattern elsewhere in Blender? If not, I think it would be good to explicitly document here.

Is the "??" prefix for "unknown/not-yet-decided" a common pattern elsewhere in Blender? If not, I think it would be good to explicitly document here.
@ -135,0 +598,4 @@
return "??";
}
static char name[3] = {0};
Member

This makes the Rust programmer in me very nervous:

  • Since name is used for the return value, if this function is called multiple times then later calls will silently change the value returned from earlier calls.
  • This is not thread safe. If this is called from multiple threads then there is a race condition writing to name.

These are both kind of nasty foot guns, because on the surface this function looks like a simple read-only query. It would be very easy to accidentally misuse. So at the very least both of these things should be documented. But even better would be to eliminate them entirely, and make this function actually a read-only query like it appears.

Can we make it an invariant that the first two chars of this->name are always (correctly) initialized? Then we can just trivially return that, and then both of these foot guns disappear.

This makes the Rust programmer in me very nervous: - Since `name` is used for the return value, if this function is called multiple times then later calls will silently change the value returned from earlier calls. - This is not thread safe. If this is called from multiple threads then there is a race condition writing to `name`. These are both kind of nasty foot guns, because on the surface this function looks like a simple read-only query. It would be very easy to accidentally misuse. So at the very least both of these things should be documented. But even better would be to eliminate them entirely, and make this function actually a read-only query like it appears. Can we make it an invariant that the first two chars of `this->name` are always (correctly) initialized? Then we can just trivially return that, and then both of these foot guns disappear.
Sybren A. Stüvel force-pushed pr/baklava-animation-filtering from 20df792ab3 to 19ba07341f 2024-04-08 15:15:03 +02:00 Compare
Nathan Vegdahl requested changes 2024-04-08 17:30:27 +02:00
@ -449,2 +469,2 @@
/* quick macro to test if AnimData is usable */
#define ANIMDATA_HAS_KEYS(id) ((id)->adt && (id)->adt->action)
/* quick macro to test if AnimData has usable Action */
#define ANIMDATA_HAS_KEYS(id) ((id)->adt && !(id)->adt->animation && (id)->adt->action)
Member

I think the semantics of this macro no longer match its name. Maybe rename it to ANIMDATA_HAS_ACTION.

I think the semantics of this macro no longer match its name. Maybe rename it to `ANIMDATA_HAS_ACTION`.
Author
Member

That's outside the scope of this PR though. The mismatch already existed prior to my changes. Sure I could clean it up, but that goes for so much of that area of the code that I'm limiting my "cleanup before changing" work.

That's outside the scope of this PR though. The mismatch already existed prior to my changes. Sure I could clean it up, but that goes for so much of that area of the code that I'm limiting my "cleanup before changing" work.
nathanvegdahl marked this conversation as resolved
@ -457,3 +476,4 @@
#define ANIMDATA_HAS_NLA(id) ((id)->adt && !(id)->adt->animation && (id)->adt->nla_tracks.first)
/**
* Quick macro to test for all three above usability tests, performing the appropriate provided
Member

This whole doc comment could use a clean up pass.

Then again, maybe it's not worth it if we're going to be abandoning the current filtering code soon-ish anyway? But wanted to flag it anyway.

This whole doc comment could use a clean up pass. Then again, maybe it's not worth it if we're going to be abandoning the current filtering code soon-ish anyway? But wanted to flag it anyway.
dr.sybren marked this conversation as resolved
@ -663,3 +735,1 @@
ale->datatype = ALE_ACT;
ale->adt = BKE_animdata_from_id(static_cast<ID *>(data));
key_data_from_adt(*ale, ma->adt);
Member

Love all these replacements. So much cleaner.

Love all these replacements. So much cleaner.
dr.sybren marked this conversation as resolved
@ -1369,0 +1358,4 @@
/* bAnimListElem::binding_handle is exposed as int32_t and not as binding_handle_t, so better
* ensure that these are still equivalent. */
static_assert(std::is_same_v<decltype(AnimationBinding::handle),
Member

Does this static assert really belong here? I feel like it should be placed near the field declaration itself, rather than in a function that happens to use it.

Does this static assert really belong here? I feel like it should be placed near the field declaration itself, rather than in a function that happens to use it.
Author
Member

I'll add a comment that explains why it's here.

I'll add a comment that explains why it's here.
dr.sybren marked this conversation as resolved
@ -385,9 +415,27 @@ short ANIM_animchannel_keyframes_loop(KeyframeEditData *ked,
*/
case ALE_GROUP: /* action group */
return agrp_keyframes_loop(ked, (bActionGroup *)ale->data, key_ok, key_cb, fcu_cb);
case ALE_ANIM: { /* Animation data-block. */
Member

It wasn't immediately clear to me why this case only executes on a single binding, rather than all bindings within the Animation datablock. As it is, it feels redundant with the ALE_ANIM_BINDING case below.

I think the reason in this case is because this is for when the Animation datablock is hierarchically displayed under the relevant ID in the channel list, and thus only the single binding to that ID is relevant. Is that correct? If so, it might be worth documenting that in a short comment.

It wasn't immediately clear to me why this case only executes on a single binding, rather than all bindings within the `Animation` datablock. As it is, it feels redundant with the `ALE_ANIM_BINDING` case below. I *think* the reason in this case is because this is for when the `Animation` datablock is hierarchically displayed *under* the relevant ID in the channel list, and thus only the single binding to that ID is relevant. Is that correct? If so, it might be worth documenting that in a short comment.
dr.sybren marked this conversation as resolved
@ -151,6 +151,7 @@ static int mouse_nla_tracks(bContext *C, bAnimContext *ac, int track_index, shor
break;
}
case ANIMTYPE_FILLACTD: /* Action Expander */
case ANIMTYPE_FILLANIM: /* Animation Expander */
Member

Since this is in NLA code where the new Animation datablock isn't supported, shouldn't this never trigger? Same with the ANIMTYPE_ANIM_BINDING further below.

If so, maybe just put them together near the bottom, with a comment explaining that.

Since this is in NLA code where the new `Animation` datablock isn't supported, shouldn't this never trigger? Same with the `ANIMTYPE_ANIM_BINDING` further below. If so, maybe just put them together near the bottom, with a comment explaining that.
dr.sybren marked this conversation as resolved
Sybren A. Stüvel force-pushed pr/baklava-animation-filtering from 19ba07341f to 82a7f7fb4d 2024-04-11 15:17:42 +02:00 Compare
Sybren A. Stüvel force-pushed pr/baklava-animation-filtering from 82a7f7fb4d to b9a63ac070 2024-04-11 17:07:00 +02:00 Compare
Author
Member

I've just force-pushed this PR into a rather different state. It now is just the one commit that adds the basic animation filtering support. This means that the Dopesheet and the Graph Editor can show F-Curves data from the Animation data-block. And that's it, no new editor, not even visualisation of Bindings. That'll all be handled in later PRs.

I've just force-pushed this PR into a rather different state. It now is just the one commit that adds the basic animation filtering support. This means that the Dopesheet and the Graph Editor can show F-Curves data from the Animation data-block. And that's it, no new editor, not even visualisation of Bindings. That'll all be handled in later PRs.
Sybren A. Stüvel added 1 commit 2024-04-11 17:17:57 +02:00
Author
Member

I'll close this PR and start a new one. The WIP commits get too noisy for a clean review, and also would cause noise when trying to figure out what happened when diving into the history.

I'll close this PR and start a new one. The WIP commits get too noisy for a clean review, and also would cause noise when trying to figure out what happened when diving into the history.

Pull request closed

Sign in to join this conversation.
No reviewers
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
3 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#119875
No description provided.