Fix #100718: NLA Hold Forward Inconsistency #107281

Wayde Moss · 2023-04-24T04:08:56+02:00

Wayde Moss commented

2023-04-24 04:08:56 +02:00

Problem:
For the Action Track, 'extrapolation=Hold Forward' behaves the same as
'Hold'. Animators expect 'Hold Forward' to behave consistently with the
rest of the NLA system but it does not.

Source of the problem:
Pre-patch, the problematic behavior was implemented as a special case
(see: animsys_create_action_track_strip() in anim_sys.c). It was added
added by commit 2826c2be54. The fix
effectively removes the special handling while keeping the core intent
of the original commit, to have the Action Track evaluate fcurve
extrapolation properly.

Solution:
For the Action Track, we now properly treat extrapolation Hold_Forward
just like the rest of the NLA system (strips): as bounded on the left
and infinite on the right. In (anim_sys.c) nlastrips_ctime_get_strip(),
an additional boundary check is required since the original commit
assumed the only possible case, when NLASTRIP_FLAG_NO_TIME_MAP is
enabled, is effectively extrapolation=HOLD.

Quick Test File:
The cube is keyed at F50 and F70 with linear interpolation. The NLA has extrap=HoldForward.
Expect (PR): at frames before F50, the cube to be at world origin.
Expect (broken version): at frames before F50, the cube extrapolates towards the top left.

Problem: For the Action Track, 'extrapolation=Hold Forward' behaves the same as 'Hold'. Animators expect 'Hold Forward' to behave consistently with the rest of the NLA system but it does not. Source of the problem: Pre-patch, the problematic behavior was implemented as a special case (see: animsys_create_action_track_strip() in anim_sys.c). It was added added by commit 2826c2be545ee0382ef37da0b0b919757b75f10a. The fix effectively removes the special handling while keeping the core intent of the original commit, to have the Action Track evaluate fcurve extrapolation properly. Solution: For the Action Track, we now properly treat extrapolation Hold_Forward just like the rest of the NLA system (strips): as bounded on the left and infinite on the right. In (anim_sys.c) nlastrips_ctime_get_strip(), an additional boundary check is required since the original commit assumed the only possible case, when NLASTRIP_FLAG_NO_TIME_MAP is enabled, is effectively extrapolation=HOLD. Quick Test File: The cube is keyed at F50 and F70 with linear interpolation. The NLA has extrap=HoldForward. Expect (PR): at frames before F50, the cube to be at world origin. Expect (broken version): at frames before F50, the cube extrapolates towards the top left.

pr_107281_NLA_HoldForward_Inconsistency.blend

840 KiB

Wayde Moss added the

Module

Animation & Rigging

label 2023-04-24 04:08:56 +02:00

Wayde Moss added this to the Animation & Rigging project 2023-04-24 04:09:43 +02:00

Brad Clark requested review from Sybren A. Stüvel 2023-04-24 05:24:24 +02:00

Brad Clark requested review from Brad Clark 2023-04-24 05:24:24 +02:00

Brad Clark commented

2023-04-24 05:32:37 +02:00

Just to add a note to make clear right now
Visualy the NLA shows the setting correctly. Bug that this patch is fixing is that while the display shows it is doing one thing (Hold forward) the result is as if it was still set to HOLD.

In the image the action currently getting keyframes "CubeAction" has the extrapolation set to Hold Forward, the display is showing that the orange track overlay starts at the first keyframe and extends to the right but the result of the extrapolation is still doing HOLD, resulting in the wrong result of the NLA being shown in the view port with one action or several.

Just to add a note to make clear right now ![image](/attachments/3a251ec3-8a18-484a-bc28-6957182d62f6) Visualy the NLA shows the setting correctly. Bug that this patch is fixing is that while the display shows it is doing one thing (Hold forward) the result is as if it was still set to HOLD. In the image the action currently getting keyframes "CubeAction" has the extrapolation set to Hold Forward, the display is showing that the orange track overlay starts at the first keyframe and extends to the right but the result of the extrapolation is still doing HOLD, resulting in the wrong result of the NLA being shown in the view port with one action or several.

image.png

42 KiB

Wayde Moss referenced this pull request

2023-04-24 06:08:44 +02:00

NLA Editor: "Hold Forward" for "Edited Action" track doesn't work #100718

Nathan Vegdahl approved these changes 2023-04-24 12:48:09 +02:00

Nathan Vegdahl left a comment

LGTM. And it indeed has the desired behavior when testing with the test file.

Brad Clark approved these changes 2023-04-24 16:23:13 +02:00

Brad Clark left a comment

Approved for fixing the bug that the display didn't match the desired extrapolation mode.

Brecht Van Lommel changed title from ~~Fix 100718 NLA Hold Forward Inconsistency~~ to Fix #100718: NLA Hold Forward Inconsistency

2023-04-25 13:01:57 +02:00

Sybren A. Stüvel reviewed 2023-05-04 09:36:43 +02:00

Sybren A. Stüvel left a comment

The code LGTM, just one question to clear up some confusion (more with the existing NLA code than your changes ;-) )

source/blender/blenkernel/intern/anim_sys.c Outdated

						
				@ -3219,0 +3224,4 @@

				   *

				   * Must set NLASTRIP_FLAG_NO_TIME_MAP, so Action Track fcurve evaluation extends beyond its

				   * keyframe bounds. */

				  r_action_strip->flag |= NLASTRIP_FLAG_USR_INFLUENCE | NLASTRIP_FLAG_NO_TIME_MAP;

Sybren A. Stüvel commented

2023-05-04 09:36:10 +02:00

Is setting NLASTRIP_FLAG_NO_TIME_MAP when r_action_strip->extendmode == NLASTRIP_EXTEND_NOTHING necessary for this fix? Honest question, because the NLASTRIP_FLAG_NO_TIME_MAP seems to have two meanings (i.e. 'avoid using user-supplied time mapping', and 'avoid boundary checks'). According to the comment, it's the latter meaning that's important here, but it would seem that that's irrelevant when there shouldn't be any extrapolation.

Is setting `NLASTRIP_FLAG_NO_TIME_MAP` when `r_action_strip->extendmode == NLASTRIP_EXTEND_NOTHING` necessary for this fix? Honest question, because the `NLASTRIP_FLAG_NO_TIME_MAP` seems to have two meanings (i.e. 'avoid using user-supplied time mapping', and 'avoid boundary checks'). According to the comment, it's the latter meaning that's important here, but it would seem that that's irrelevant when there shouldn't be any extrapolation.

Wayde Moss commented

2023-05-04 20:12:23 +02:00

It's not necessary. I think I just accidentally mixed a bit of cleanup with the fix: There's no need for the NOTHING check anymore. Should I remove it or separate the cleanup into its own commit? I can see how it's confusing to have a commit that fixes something but has non-comment changes that are irrelevant to the fix, makes you wonder whether it's important or not.

NLASTRIP_FLAG_NO_TIME_MAP does mean 'avoid user supplied time-mapping', but not really 'avoid boundary check'. It's more 'avoid time clamping'.

It's not necessary. I think I just accidentally mixed a bit of cleanup with the fix: There's no need for the `NOTHING` check anymore. Should I remove it or separate the cleanup into its own commit? I can see how it's confusing to have a commit that fixes something but has non-comment changes that are irrelevant to the fix, makes you wonder whether it's important or not. `NLASTRIP_FLAG_NO_TIME_MAP` does mean 'avoid user supplied time-mapping', but not really 'avoid boundary check'. It's more 'avoid time clamping'.

Sybren A. Stüvel commented

2023-05-05 12:21:28 +02:00

Yeah, please split those up, it'll help for sure.

wbmoss_dev marked this conversation as resolved

Sybren A. Stüvel added this to the 3.6 LTS milestone 2023-05-04 09:37:18 +02:00

Brad Clark commented

2023-05-15 19:04:38 +02:00

is this ready to land?

Sybren A. Stüvel commented

2023-05-22 14:16:42 +02:00

is this ready to land?

I don't see the changes @wbmoss_dev said he'd make, so I'm still waiting for those.

> is this ready to land? I don't see the changes @wbmoss_dev said he'd make, so I'm still waiting for those.

Wayde Moss force-pushed pr_fix_100718_NLA_Hold_Forward_Inconsistency from 8db7faca4c to f03415f699

2023-05-23 08:08:00 +02:00

Compare

Wayde Moss commented

2023-05-23 08:10:51 +02:00

Rebased and split commit. Should be good to land.

🚀 1

Sybren A. Stüvel requested changes 2023-06-01 17:52:33 +02:00

source/blender/blenkernel/intern/anim_sys.c

						
				@ -994,0 +995,4 @@

				     * fcurve extrapolation isn't clamped to the keyframe bounds. */

				    bool in_range = IN_RANGE_INCL(ctime, strip->start, strip->end);

				    if (strip->flag & NLASTRIP_FLAG_NO_TIME_MAP) {

				      if (ELEM(strip->extendmode, NLASTRIP_EXTEND_HOLD)) {

Sybren A. Stüvel commented

2023-06-01 17:50:26 +02:00

Can't this just be coded like this? I don't see the need for ELEM() here.

switch (strip->extendmode) {
  case NLASTRIP_EXTEND_HOLD: in_range = true; break;
  case NLASTRIP_EXTEND_HOLD_FORWARD: in_range = ctime >= strip->start; break;
}

Can't this just be coded like this? I don't see the need for `ELEM()` here. ```c switch (strip->extendmode) { case NLASTRIP_EXTEND_HOLD: in_range = true; break; case NLASTRIP_EXTEND_HOLD_FORWARD: in_range = ctime >= strip->start; break; } ```

Nate Rupsis referenced this pull request

2023-06-21 03:25:00 +02:00

Fix #100718: NLA Hold Forward Inconsistency #109182

Nate Rupsis commented

2023-06-21 03:26:57 +02:00

I'll finish landing this change in 109182 (Since I don't believe I can update this PR...)

I'll finish landing this change in [109182](https://projects.blender.org/blender/blender/pulls/109182) (Since I don't believe I can update this PR...)

👍 1

Nate Rupsis closed this pull request

2023-06-21 03:26:59 +02:00

Brad Clark commented

2023-07-01 22:57:44 +02:00

Question, is this bug something we can target for 3.6.1 since I think we are set on yes it should work from the module meeting?

Sybren A. Stüvel referenced this pull request

2023-07-03 12:28:25 +02:00

Fix #100718: NLA Hold Forward Inconsistency #109182

Sybren A. Stüvel removed this from the Animation & Rigging project 2023-09-14 14:25:21 +02:00