WIP: Anim: Baklava, add Animation data-block to anim filtering code #119875
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#119875
Loading…
Reference in New Issue
No description provided.
Delete Branch "dr.sybren:pr/baklava-animation-filtering"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.be4eb400ff
to80c8778632
4424a4bee4
to9399e1158c
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
dfef9e8f99
to20df792ab3
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.
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};
This makes the Rust programmer in me very nervous:
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.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.20df792ab3
to19ba07341f
@ -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)
I think the semantics of this macro no longer match its name. Maybe rename it to
ANIMDATA_HAS_ACTION
.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.
@ -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
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.
@ -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);
Love all these replacements. So much cleaner.
@ -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),
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.
I'll add a comment that explains why it's here.
@ -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. */
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 theALE_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.@ -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 */
Since this is in NLA code where the new
Animation
datablock isn't supported, shouldn't this never trigger? Same with theANIMTYPE_ANIM_BINDING
further below.If so, maybe just put them together near the bottom, with a comment explaining that.
19ba07341f
to82a7f7fb4d
82a7f7fb4d
tob9a63ac070
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'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