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

Nathan Vegdahl · 2024-05-07T15:28:23+02:00

Nathan Vegdahl commented

2024-05-07 15:28:23 +02:00

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

Refactor: centralize responsibility for "Only Insert Needed" 78683a850b

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.

Merge branch 'main' into centralize_only_insert_needed c6fb16b05d

Nathan Vegdahl added 1 commit 2024-05-07 15:34:43 +02:00

Make argument order of KeyframeStrip::keyframe_insert() consistent with insert_vert_fcurve() 6d531a21b1

Nathan Vegdahl added 1 commit 2024-05-07 15:52:26 +02:00

Blindly keep previous behavior where "Insert Only Needed" was ignored. f0d45c42ab

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

source/blender/animrig/intern/fcurve.cc Outdated

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

Nathan Vegdahl commented

2024-05-07 15:59:10 +02:00

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

Make new_key_needed() take fcurve as a reference

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

Nathan Vegdahl requested review from Sybren A. Stüvel 2024-05-07 16:48:16 +02:00

Nathan Vegdahl referenced this pull request

2024-05-07 16:52:20 +02:00

Anim: implement 3D viewport keyframing functionality for the Animation data-block #120428

Christoph Lendenfeld requested changes 2024-05-07 16:55:57 +02:00

Christoph Lendenfeld left a comment

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

Nathan Vegdahl commented

2024-05-07 17:39:54 +02:00

@blender-bot build

Nathan Vegdahl commented

2024-05-07 17:54:06 +02:00

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

Nathan Vegdahl commented

2024-05-07 18:05:14 +02:00

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

source/blender/animrig/intern/keyframing.cc Outdated

						
				@ -500,2 +466,2 @@

				    return SingleKeyingResult::NO_KEY_NEEDED;

				  }

				  /* Ignore "Insert Only Needed".

				   * NOTE: this blindly preserves old behavior that would have otherwise changed

Nathan Vegdahl commented

2024-05-07 18:13:32 +02:00

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

Remove erroneous inhibition of the "Only Insert Needed" flag 23f7cd7fed

Nathan Vegdahl added 1 commit 2024-05-07 18:37:02 +02:00

Merge branch 'main' into centralize_only_insert_needed d9ef589511

Nathan Vegdahl commented

2024-05-07 18:54:41 +02:00

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.

Christoph Lendenfeld commented

2024-05-09 11:35:33 +02:00

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?

Nathan Vegdahl commented

2024-05-10 10:22:39 +02:00

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

Merge branch 'main' into centralize_only_insert_needed bde323bd77

Christoph Lendenfeld approved these changes 2024-05-10 11:36:53 +02:00

Christoph Lendenfeld left a comment

lgtm now

source/blender/editors/armature/pose_slide.cc Outdated

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

Christoph Lendenfeld commented

2024-05-10 11:35:52 +02:00

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

Nathan Vegdahl commented

2024-05-10 11:51:14 +02:00

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`.

👍 1

nathanvegdahl marked this conversation as resolved

Nathan Vegdahl added 1 commit 2024-05-10 11:38:10 +02:00

Merge branch 'main' into centralize_only_insert_needed 648b2f9291

Nathan Vegdahl added 1 commit 2024-05-10 11:46:11 +02:00

Don't change KeyframeStrip::keyframe_insert()'s API yet f69b308d41

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

Get rid of some PR noise from not-fully-reverted changes 7f0afc8773

Nathan Vegdahl added 1 commit 2024-05-10 11:57:17 +02:00

Delete weird file that somehow appeared 9cf1d8f2ea

Sybren A. Stüvel approved these changes 2024-05-10 15:47:12 +02:00

Sybren A. Stüvel left a comment

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.

source/blender/animrig/intern/fcurve.cc Outdated

						
				@ -278,1 +320,4 @@

				{

				  BLI_assert(fcu != nullptr);

				  if (flag & INSERTKEY_NEEDED && !new_key_needed(*fcu, position[0], position[1])) {