Key the Current Influence Value of the Strip when Pushing Down the First NLA Track
ClosedPublic
Actions

Authored by Robert Sunny (robertsunny) on Apr 21 2022, 6:46 PM.

Details

Reviewers

Sybren A. Stüvel (sybren)
Brad Clark (RiggingDojo)

Commits

rB2f49908a5e57: NLA: Key influence when pushing down to NLA

Summary

Comment out the if statement that checks whether the action is the first NLA track when it is pushed down, so the influence of the edited action will always be keyed on the first frame if it's not 1.0 whether it's the first NLA track or not.

This was a request from Brad Clark to help bring consistency to the behavior of the NLA. Right now the code was second guessing, overriding user settings. This caused users to not know that the behavior was different or working for other actions pushed down that were not the first one.

Diff Detail

Repository

rB Blender

Branch

arcpatch-D14719 (branched from master)

Build Status

Buildable 21897
Build 21897: arc lint + arc unit

Event Timeline

Robert Sunny (robertsunny) requested review of this revision.Apr 21 2022, 6:46 PM

Robert Sunny (robertsunny) created this revision.

Brad Clark (RiggingDojo) edited the summary of this revision. (Show Details)Apr 21 2022, 6:58 PM

Accepting this revision based on testing the change and having a much better user experience with expected results after the fix has been applied.

Question for code review, should the patch remove the code fully?

This revision is now accepted and ready to land.Apr 22 2022, 3:52 AM

Brad Clark (RiggingDojo) added 1 blocking reviewer(s): Sybren A. Stüvel (sybren).Apr 22 2022, 3:53 AM

This revision now requires review to proceed.Apr 22 2022, 3:53 AM

Sybren A. Stüvel (sybren) retitled this revision from Key the Current Influence Value of the when Pushing Down the First NLA Track to Key the Current Influence Value of the Strip when Pushing Down the First NLA Track.Apr 22 2022, 9:33 AM

Sybren A. Stüvel (sybren) edited the summary of this revision. (Show Details)

In D14719#395763, @Brad Clark (RiggingDojo) wrote:

Accepting this revision based on testing the change and having a much better user experience with expected results after the fix has been applied.

👍

Note that T54233 also doesn't mention anything about special treatment for the first strip, so it's not clear why this was done this way in the first place.

Question for code review, should the patch remove the code fully?

Yup. Both dead (i.e. code that's there but never used) and commented-out code should just be removed.

source/blender/blenkernel/intern/nla.c
1885	`is_first` is now no longer used, so the variable can be removed.
1916	This note is a bit hard to follow, because "consistency" can mean so many things. I'd suggest something like this: Even though the influence of the bottom strip is ignored by Blender, it's still important to transfer it to the strip to ensure no information is lost. After all, strips can be reordered later. Personally I feel that the `NOTE:` can be removed; such markers should be used (IMO) only for things that are more obscure / hard to realise at first glance.

This revision now requires changes to proceed.Apr 22 2022, 10:17 AM

Removed the "is_first" variable, the NOTE comment and the old code.

"Even though the influence of the bottom strip is ignored by Blender, it's still important to transfer it to the strip to ensure no information is lost. After all, strips can be reordered later."

The influence of the bottom strip IS NOT ignored though, it blends the amount of influence the animation has turning it off completely if 0 so I don't know why it had an exception.

Consistency in this context means that the resulting action is always what is user expected and set.. if a user sets the influence value to .5 and pushed it down,regardless of the number of actions, it should take that value and transfer it to the animated influence, instead of what it does now, where it does nothing the first time, then works as expected after that.