Refactor: centralize responsibility for "Only Insert Needed" #121525
No reviewers
Labels
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
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
Viewport & EEVEE
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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#121525
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "nathanvegdahl/blender:centralize_only_insert_needed"
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?
Split off from #120428.
This moves the responsibility for handling the "Only Insert Needed"
flag from
insert_keyframe_value()
to the lower-level functioninsert_vert_fcurve()
.We're doing this because
insert_vert_fcurve()
is the common entrypoint between the new animation system's and old animation system's
keyframing code. This therefore ensures that both systems will
behave the same way with respect to the "Only Insert Needed" flag.
KeyframeStrip::keyframe_insert()
consistent withinsert_vert_fcurve()
6d531a21b1WIP: Refactor: centralize responsibility for "Only Insert Needed"to Refactor: centralize responsibility for "Only Insert Needed"@ -495,3 +544,3 @@
for (n = 1, fp = value_cache; n < range && fp; n++, fp++) {
blender::animrig::insert_vert_fcurve(
fcu, {fp->frame, fp->val}, settings, eInsertKeyFlags(1));
fcu, {fp->frame, fp->val}, settings, INSERTKEY_NOFLAGS);
Note to reviewers: this is changed to
INSERTKEY_NOFLAGS
because the flag previously passed was actually not used byinsert_vert_fcurve()
, but now is. So this actually preserves the previous behavior.This same change is elsewhere in this PR as well, for the same reason.
new_key_needed()
take fcurve as a referencethis PR breaks
Insert Needed
including the unit tests. I can dig deeper on thursday as to why that is, as it's not immediately apparent from the code@blender-bot build
🤦 Thanks for catching that! I'll look into it as well, of course. :-)
Figured it out. It's because I'm a goofball. I'll make a comment directly on the code where I goofed, so you can laugh at me. ;-)
@ -500,2 +466,2 @@
return SingleKeyingResult::NO_KEY_NEEDED;
}
/* Ignore "Insert Only Needed".
* NOTE: this blindly preserves old behavior that would have otherwise changed
Here's where I goofed. This inhibiting of the flag, and the whole note along with it, is wrong. And that's because this is the very function where "Only Insert Needed" was previously handled.
I removed the old code that handled the flag here while moving the changes over from the other PR. And then I did a separate pass after committing that code to see where the flag needed to be ignored due to the changes to
insert_vert_fcurve()
. Hence how I ended up here.Browsing through the PR code myself, I'm also now wondering if this is actually the right thing to do.
For example, an alternative would be to simply have
KeyframeStrip::keyframe_insert()
callinsert_keyframe_value()
rather thaninsert_vert_fcurve()
. Although maybe there was a specific reason thatinsert_vert_fcurve()
was chosen forKeyframeStrip::keyframe_insert()
rather thaninsert_keyframe_value()
?In any case, the main thing is that we should choose a function that's the common entry point between the new and old animation systems for inserting keys into an fcurve. Whether that's
insert_vert_fcurve()
orinsert_keyframe_value()
I don't think matters too much. But whichever one it is should handle the common bits between the two systems, and not the bits specific to one or the other. This PR basically choosesinsert_vert_fcurve()
to be that common entry point.Thanks for figuring that out :)
insert_vert_fcurve
andinsert_keyframe_value
do different things.insert_keyframe_value
does cycle aware remapping and "insert needed" checks, then callsinsert_vert_fcurve
(in certain cases).So it does matter which function is called.
I am wondering if we can remove
insert_keyframe_value
and move its functionality intoinsert_vert_fcurve
.Not 100% sure about that though because there may be times when we want to ignore the lock on FCurves or cycle aware keying?
Yeah. And to some extent it kinds of feels to me like those belong together: they do feel to me like they're at the same "level" conceptually. However, the cycle-aware keying code, for example, is not currently shared between the old and new animation system (and maybe never will be?). So it feels to me like there's a tension there.
On top of that, the keying code already has about a million layers to it, so introducing yet another layer (e.g. a new function that sits above or below
insert_keyframe_value
) to resolve this leaves a poor taste in my mouth. If anything, I'd like to reduce the number of layers, and give them each clear responsibilities.Yeah, that might be a good idea. And then we can just inhibit the cycle-aware keying etc. for the new animation system. I don't love it, but it might be the best option we have for now, without a much larger refactor.
lgtm now
@ -1714,7 +1714,7 @@ static void propagate_curve_values(ListBase /*tPChanFCurveLink*/ *pflinks,
const float current_fcu_value = evaluate_fcurve(fcu, source_frame);
LISTBASE_FOREACH (FrameLink *, target_frame, target_frames) {
insert_vert_fcurve(
fcu, {target_frame->frame, current_fcu_value}, settings, INSERTKEY_NEEDED);
I expected this to change behavior but it doesn't seem to do that
It doesn't change behavior for the same reason as the comment I left further above:
In other words,
insert_vert_fcurve()
previously completely ignoredINSERTKEY_NEEDED
, so the fact that it was passed that flag in a couple of places (including here) was meaningless before. But now it's actually used, so to preserve behavior we have to change toINSERTKEY_NOFLAGS
.KeyframeStrip::keyframe_insert()
's API yet f69b308d41Yes please 💜
PR LGTM.
@ -278,1 +320,4 @@
{
BLI_assert(fcu != nullptr);
if (flag & INSERTKEY_NEEDED && !new_key_needed(*fcu, position[0], position[1])) {
Add parentheses around
(flag & INSERTKEY_NEEDED)
Gah! Thanks.
@blender-bot build