Refactor: centralize responsibility for "Only Insert Needed" #121525

Merged
Nathan Vegdahl merged 13 commits from nathanvegdahl/blender:centralize_only_insert_needed into main 2024-05-10 17:11:31 +02:00
Member

Split off from #120428.

This moves the responsibility for handling the "Only Insert Needed"
flag from insert_keyframe_value() to the lower-level function
insert_vert_fcurve().

We're doing this because insert_vert_fcurve() is the common entry
point 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.

Split off from #120428. This moves the responsibility for handling the "Only Insert Needed" flag from `insert_keyframe_value()` to the lower-level function `insert_vert_fcurve()`. We're doing this because `insert_vert_fcurve()` is the common entry point 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.
Nathan Vegdahl added the
Module
Animation & Rigging
label 2024-05-07 15:28:23 +02:00
Nathan Vegdahl added 2 commits 2024-05-07 15:28:33 +02:00
78683a850b Refactor: centralize responsibility for "Only Insert Needed"
This makes `insert_vert_fcurve()` responsible for appropriately
handling the `Only Insert Needed` flag.

This is in preparation for the new layered action keyframing code (to
come in a later PR), since we want a common code path for old- and
new-style actions that always handles this flag correctly.  And just
generally this seems like a more sensible place for itto be handled
anyway.
Nathan Vegdahl added 1 commit 2024-05-07 15:34:43 +02:00
Nathan Vegdahl added 1 commit 2024-05-07 15:52:26 +02:00
Nathan Vegdahl changed title from WIP: Refactor: centralize responsibility for "Only Insert Needed" to Refactor: centralize responsibility for "Only Insert Needed" 2024-05-07 15:52:37 +02:00
Nathan Vegdahl requested review from Christoph Lendenfeld 2024-05-07 15:52:47 +02:00
Nathan Vegdahl reviewed 2024-05-07 15:59:10 +02:00
@ -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);
Author
Member

Note to reviewers: this is changed to INSERTKEY_NOFLAGS because the flag previously passed was actually not used by insert_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.

Note to reviewers: this is changed to `INSERTKEY_NOFLAGS` because the flag previously passed was actually not used by `insert_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.
Nathan Vegdahl added 1 commit 2024-05-07 16:07:28 +02:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
7b03b718e5
Make `new_key_needed()` take fcurve as a reference
Nathan Vegdahl requested review from Sybren A. Stüvel 2024-05-07 16:48:16 +02:00
Christoph Lendenfeld requested changes 2024-05-07 16:55:57 +02:00
Christoph Lendenfeld left a comment
Member

this 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

this 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
Author
Member

@blender-bot build

@blender-bot build
Author
Member

🤦 Thanks for catching that! I'll look into it as well, of course. :-)

🤦 Thanks for catching that! I'll look into it as well, of course. :-)
Author
Member

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. ;-)

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. ;-)
Nathan Vegdahl reviewed 2024-05-07 18:13:32 +02:00
@ -500,2 +466,2 @@
return SingleKeyingResult::NO_KEY_NEEDED;
}
/* Ignore "Insert Only Needed".
* NOTE: this blindly preserves old behavior that would have otherwise changed
Author
Member

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.

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.
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl added 1 commit 2024-05-07 18:14:18 +02:00
Nathan Vegdahl added 1 commit 2024-05-07 18:37:02 +02:00
Author
Member

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() call insert_keyframe_value() rather than insert_vert_fcurve(). Although maybe there was a specific reason that insert_vert_fcurve() was chosen for KeyframeStrip::keyframe_insert() rather than insert_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() or insert_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 chooses insert_vert_fcurve() to be that common entry point.

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()` call `insert_keyframe_value()` rather than `insert_vert_fcurve()`. Although maybe there was a specific reason that `insert_vert_fcurve()` was chosen for `KeyframeStrip::keyframe_insert()` rather than `insert_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()` or `insert_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 chooses `insert_vert_fcurve()` to be that common entry point.

Thanks for figuring that out :)

insert_vert_fcurve and insert_keyframe_value do different things.

insert_keyframe_value does cycle aware remapping and "insert needed" checks, then calls insert_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 into insert_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?

Thanks for figuring that out :) `insert_vert_fcurve` and `insert_keyframe_value` do different things. `insert_keyframe_value` does cycle aware remapping and "insert needed" checks, then calls `insert_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 into `insert_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?
Author
Member

insert_keyframe_value does cycle aware remapping and "insert needed" checks, then calls insert_vert_fcurve (in certain cases).

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.

I am wondering if we can remove insert_keyframe_value and move its functionality into insert_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, 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.

> `insert_keyframe_value` does cycle aware remapping and "insert needed" checks, then calls `insert_vert_fcurve` (in certain cases). 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. > I am wondering if we can remove `insert_keyframe_value` and move its functionality into `insert_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, 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.
Nathan Vegdahl added 1 commit 2024-05-10 11:34:48 +02:00
Christoph Lendenfeld approved these changes 2024-05-10 11:36:53 +02:00
Christoph Lendenfeld left a comment
Member

lgtm now

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

I expected this to change behavior but it doesn't seem to do that
Author
Member

It doesn't change behavior for the same reason as the comment I left further above:

Note to reviewers: this is changed to INSERTKEY_NOFLAGS because the flag previously passed was actually not used by insert_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.

In other words, insert_vert_fcurve() previously completely ignored INSERTKEY_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 to INSERTKEY_NOFLAGS.

It doesn't change behavior for the same reason as the comment I left further above: > Note to reviewers: this is changed to `INSERTKEY_NOFLAGS` because the flag previously passed was actually not used by `insert_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. In other words, `insert_vert_fcurve()` previously completely ignored `INSERTKEY_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 to `INSERTKEY_NOFLAGS`.
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl added 1 commit 2024-05-10 11:38:10 +02:00
Nathan Vegdahl added 1 commit 2024-05-10 11:46:11 +02:00
f69b308d41 Don't change` KeyframeStrip::keyframe_insert()`'s API yet
That's better left for when we actually start using the flags in the
functions that call it.
Nathan Vegdahl added 1 commit 2024-05-10 11:56:01 +02:00
Nathan Vegdahl added 1 commit 2024-05-10 11:57:17 +02:00
Sybren A. Stüvel approved these changes 2024-05-10 15:47:12 +02:00
Sybren A. Stüvel left a comment
Member

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.

Yes please 💜

PR LGTM.

> 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. Yes 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)

Add parentheses around `(flag & INSERTKEY_NEEDED)`
Author
Member

Gah! Thanks.

Gah! Thanks.
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl added 1 commit 2024-05-10 16:46:22 +02:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
eb20c5b1d8
Add missing parentheses
Author
Member

@blender-bot build

@blender-bot build
Nathan Vegdahl merged commit 58cf1a7c44 into main 2024-05-10 17:11:31 +02:00
Nathan Vegdahl deleted branch centralize_only_insert_needed 2024-05-10 17:11:34 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser Project (Legacy)
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
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#121525
No description provided.