Page MenuHome

Fix T81533: NLA Properly Draw FModifiers
ClosedPublic

Authored by Wayde Moss (GuiltyGhost) on Dec 29 2020, 9:25 PM.

Details

Summary

When NLA strips weren't time-aligned with the underlying action, then fcurve modifiers would not be drawn anchored to the strip. Fmodifiers were evaluating properly, they just weren't drawn with the proper offset and scale.

To fix it in this specific case, I've chosen to undo the keyframe remapping then remap the draw-evaluation-time from scene time to fcurve time. Afterward, I redo the keyframe remapping so the controls are properly drawn.

The Envelope fmodifier has special drawing code which was fixed too. In this case, no mapping at all was happening. The solution was similar, to remap the envelope control points from fcurve time to scene time.

Quick test file.

Diff Detail

Repository
rB Blender
Branch
arcpatch-D9953 (branched from master)
Build Status
Buildable 11928
Build 11928: arc lint + arc unit

Event Timeline

Wayde Moss (GuiltyGhost) requested review of this revision.Dec 29 2020, 9:25 PM
Wayde Moss (GuiltyGhost) created this revision.
  • pull remapping out of loop for efficiency (thanks Maxime for heads up)
Sybren A. Stüvel (sybren) requested changes to this revision.Jan 8 2021, 11:52 AM
Sybren A. Stüvel (sybren) added inline comments.
source/blender/editors/space_graph/graph_draw.c
586

use_nla_remap can be const

662–663

eval_start and eval_freq can be const

668

eval_time can be const

1038

2x "instead" makes this a bit more confusing than it is.

1039

What is the reason that you did the unmap-eval-remap here? Wouldn't it be more correct to make the FCurve Modifier evaluation NLA-aware? If so, maybe add a TODO here that indicates that. If this really is the best solution, remove the XXX as it has little meaning then.

1041

I know other code still uses 1/0 for true/false, but new code shouldn't.

This revision now requires changes to proceed.Jan 8 2021, 11:52 AM
Wayde Moss (GuiltyGhost) marked 5 inline comments as done.
  • apply review notes

I think it's fine to commit this to the 2.92 release branch. If you've never worked with the stabilizing branch before, it's basically committing to the blender-v2.92-release branch, pushing, then merging the release branch into master and pushing again. It's documented in more detail on the wiki Stabilizing Branch article.

This revision is now accepted and ready to land.Jan 14 2021, 12:54 PM

Thanks for accepting.

I thought I had submitted my reply to your question, sorry.

source/blender/editors/space_graph/graph_draw.c
1039

Then all evaluate_fcurve() calls would require passing an additional fmodifier_eval_time whenever we remap the fcurve. Keyframes and fmodifiers would be in different time spaces which seems unnecessarily complicated. The current code-base generally uses BKE_nla_tweakedit_remap() when working with both keys and fmodifiers.

Wayde Moss (GuiltyGhost) closed this revision.EditedJan 14 2021, 11:21 PM

Committed but manually closed due to forgetting to re-add diff reference in commit message.

(I also accidentally pushed this commit to 2.92 and master. The duplicate commit is https://developer.blender.org/rB40d391fa6068cf56765bfd187c68275f3bfe06e9 for reference)