Page MenuHome

NLA: Evaluate Tweak Strip Within Synced Action Bounds
ClosedPublic

Authored by Wayde Moss (GuiltyGhost) on Apr 26 2020, 3:42 AM.
Tokens
"Love" token, awarded by gilberto_rodrigues."Love" token, awarded by Juangra_Membata."Love" token, awarded by anuarnor."Love" token, awarded by RedMser."Love" token, awarded by brilliant_ape."Love" token, awarded by bunny.

Details

Summary

This addresses T63675: Placing keyframe outside of NLA Strip frame range while in Tweak Mode no longer possible
For this patch, changes include D7602: NLA: Fix Sync Length due to the dependency but the only file of interest is anim_sys.c

Problem
The animator can not view the NLA result outside of the strip bounds. This is expected behavior. However, if they have flagged the strip to sync with the action bounds then their expectation is for the strip to evaluate based on the action bounds, not the only-updated-on-tweak-exit strip bounds. Further, it's not possible to insert keys outside of the strip bounds. This results in a workflow where the animator must determine the strip bounds before making adjustments to the action. This isn't trivial to determine so it's a trial and error process.

Solution
The NLA system now evaluates the strip based on the action bounds if it's flagged for syncing. Now the animator can freely insert and modify keys outside of the strip bounds. They will never have to touch the strip bounds directly. Changing the evaluation bounds is a simple as moving keys around.

Old: Unable to Insert Keys outside synced strip bounds

New: Able to insert keys outside synced strip bounds

Old: Unable to view NLA result outside synced strip bounds

New: Able to view NLA result outside synced strip bounds

Note: In the existing implementation, there is an inconsistency. If you unpin the track, then the tweaked strip evaluates and allows keyframe insertion outside the strip bounds, even if extend mode is None. Effectively, unpinning does close to what this patch does. If pinned (default), then the strip does not evaluate nor allow keyframing outside the strip bounds. So why not use unpinned strips? Unpinned strips treat the keyframe times as global times. Pinning is stored in the animation data, not per strip, so its an all or nothing use. Thus all actions will always evaluate at the same time no matter where any strip is placed time-wise. This makes all actions one-time use only which goes against the core use of the NLA system.

Example File:

Diff Detail

Repository
rB Blender
Branch
nlastrip_sync_eval_keys_outside_bounds (branched from master)
Build Status
Buildable 7733
Build 7733: arc lint + arc unit

Event Timeline

Wayde Moss (GuiltyGhost) requested review of this revision.Apr 26 2020, 3:43 AM
  • ..accidental code deletion
  • revert unrelated changes
  • include action bounds sync fix dependency
  • for completeness and convenience, might as well include applying the action fix on button press
Wayde Moss (GuiltyGhost) edited the summary of this revision. (Show Details)May 2 2020, 4:04 AM
Wayde Moss (GuiltyGhost) edited the summary of this revision. (Show Details)
Sybren A. Stüvel (sybren) requested changes to this revision.Jun 18 2020, 4:55 PM

Nice work, and I also don't understand the reason behind Blender's current behaviour ;-)
Just a few small nags.

source/blender/blenkernel/BKE_nla.h
103

It might be nice to have a comment that explains what this actually syncs. Can be as simple as one short sentence.

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

This variable seems unused.

2210–2214

The space between ends. and */ was actually correct, no need to change that in this patch.

source/blender/blenkernel/intern/nla.c
1363

Comments should be English sentences, so start with a capital and end with a period. Same goes for the other comments.

1367

The inner parentheses are unnecessary.

1373

The comment is basically a re-wording of the function name, so it doesn't add any information.

1376

Either configure your editor to do auto-formatting with Clang-Format, or run make format before submitting a patch.

source/blender/editors/space_nla/nla_edit.c
1856–1857

What a function does should be clear from the function name, so this comment is redundant.

This revision now requires changes to proceed.Jun 18 2020, 4:55 PM
Wayde Moss (GuiltyGhost) edited the summary of this revision. (Show Details)Aug 4 2020, 1:06 PM
Wayde Moss (GuiltyGhost) updated this revision to Diff 27395.EditedAug 4 2020, 1:24 PM
  • rebase
  • removed unused variable
  • cleaned up modified comments

Included example file in first post.

I apologize for the delay. I appreciate the review. I rebased onto D7602 (which also has been rebased) due to the dependency.

Thanks for the updates. Just one tiny nag about the use of abbreviations. You can just change that when committing, no need to re-upload the diff here.

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

Avoid abbreviations whenever possible. Just write "NLA evaluation" and "adjacent strips".

This revision is now accepted and ready to land.Aug 14 2020, 11:37 AM

Thank you for accepting the patch.

.. You can just change that when committing, no need to re-upload the diff here.

I don't have commit rights.

I don't have commit rights.

Oops, I saw commits listed on https://developer.blender.org/people/commits/143708/ and missed the fact that they were actually committed by someone else.