Page MenuHome

NLA: insert keyframes correctly for strips with non-Replace mode.
ClosedPublic

Authored by Alexander Gavrilov (angavrilov) on Nov 10 2018, 11:27 AM.

Details

Summary

NLA strips support using the keyframe values in a variety of ways:
adding, subtracting, multiplying, linearly mixing with the result
of strips located below in the stack. This is intended for layering
tweaks on top of a base animation.

However, when inserting keyframes into such strips, it simply inserts
the final value of the property, irrespective of these settings. This
in fact makes the feature nearly useless.

To fix this it is necessary to evaluate the NLA stack below the
edited strip and correctly compute the raw key that would produce
the intended final value, according to the mode and influence.

Diff Detail

Repository
rB Blender

Event Timeline

Committed the unrelated frame timing bugfix in Action & Graph editor code separately.

This is a nice improvement that now makes it possible to do basic animation layering in Blender.

But there are still issues if the two layered strips don't overlap completely where scaling can add up wrong.

And adding two clips together with scale values of 1 should not result in a result of scale 2 - otherwise your objects will always grow in size to the number of strips you are adding.

Regardless of these issues, your patch is already an improvement over what we have, so +1 from me to commit this.

This revision is now accepted and ready to land.Nov 12 2018, 3:23 PM
Joshua Leung (aligorith) requested changes to this revision.Nov 12 2018, 3:50 PM

IMO, this is the wrong approach.

You should not be polluting the API with crap for one case like this, and recomputing the NLA results every time you want to insert a keyframe. Instead, it'd be better to have a semi-permanent (but still runtime only - i.e. not saved to DNA/files) cache of the values for each channel affected by the NLA stack. We effectively need something like this to be in place anyway to solve another related issue here (e.g. the "thread safety" issue where you get different results if a short-lived strip affects values that are not keyed elsewhere, causing issues when jumping around the timeline instead of playing everything in forward order only).

This revision now requires changes to proceed.Nov 12 2018, 3:50 PM

You should not be polluting the API with crap for one case like this, and recomputing the NLA results every time you want to insert a keyframe.

What "crap for one case" - keyframing isn't just 'one case'. This problem has not been addressed in any way for almost 10 years by the look of it, so if the reason for that was "not polluting the API", then this is the wrong approach to think of the problem.

Instead, it'd be better to have a semi-permanent (but still runtime only - i.e. not saved to DNA/files) cache of the values for each channel affected by the NLA stack.

First of all, keyframe insertion has a frame argument, and can be done at any point in the timeline. Therefore, any such caches would not be useful.

We effectively need something like this to be in place anyway to solve another related issue here (e.g. the "thread safety" issue where you get different results if a short-lived strip affects values that are not keyed elsewhere, causing issues when jumping around the timeline instead of playing everything in forward order only).

This only needs caching a table of all channels that are keyed anywhere on the stack, which is a totally different issue from evaluation, and thus not very relevant. It also has a completely separate problem of cache invalidation.

Another thing - for inserting keyframes you need to evaluate separately the edited strip and everything below it, whereas for display everything has to be evaluated and blended together. Therefore you need different data for it.

This is a real problem that makes Add strips basically useless as there is no way to create them properly in the first place, so abstract discussions about 'polluting' the API and suggestions that basically amount in the information content to 'rewrite the whole NLA instead', are not helpful.

Rebased and resolved conflicts after removing AnimMapper.

the "thread safety" issue where you get different results if a short-lived strip affects values that are not keyed elsewhere, causing issues when jumping around the timeline instead of playing everything in forward order only.

Do you have any ideas about this? The only thing currently clear to me that it should follow a rule that if at any point in the timeline a channel is affected by NLA, it should be updated everywhere; however what to use for the fallback value is something to think about, and the only one clear option is the default value of the property.

As I understand it this is a pretty critical fix to make the NLA usable, and it's not that complicated. If there is no solution to handle this in a better way in the short term, then I really think we should take this approach.

In general I'm not sure why caching would be preferred over doing some extra work when inserting a keyframe. Performance isn't really a concern, and caches tend to have various ways of failing.

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

It might be good to put this next to nlaevalchan_accumulate, and refactor and bring the variable naming in sync to make it clear this function is the inverse of that.

Split off math functions for blending NLA channel values and move them together.

Alexander Gavrilov (angavrilov) marked an inline comment as done.EditedNov 12 2018, 9:00 PM

I actually intend to do a big refactoring of the NlaEvalChannel processing next, in order to solve the "thread safety" issue in some way, and also try to support an "intelligent add" mode, which would combine all transformation channels according to their type (the difficult part being that quaternions have to be processed as a unit, so the eval channels need to cover whole properties using arrays of values inside, instead of one channel per scalar value).

This patch however is a quite simple and important improvement that I wanted to do first - I've actually seen in the chat instances of people desperate enough they try to dig into the code hoping that there is a one-line fix for it.

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

Extracted the pure math parts from both functions and put them right next to each other.

@Joshua Leung (aligorith), do you have time to respond, and explain further why it should be done another way?

Or otherwise agree to go ahead with this? It would good if @Alexander Gavrilov (angavrilov) could keep going ahead with his animation system improvements.

Given the importance of this patch and to let @Alexander Gavrilov (angavrilov) continue working, I'm accepting this patch to be committed.

This revision was not accepted when it landed; it landed in state Needs Review.Fri, Dec 14, 8:59 PM
This revision was automatically updated to reflect the committed changes.
Alexander Gavrilov (angavrilov) marked an inline comment as done.