Fix T74927: Slow playback using Auto Normalization
ClosedPublic
Actions

Authored by Philipp Oeser (lichtwerk) on Mar 20 2020, 3:41 PM.

Details

Reviewers

Sybren A. Stüvel (sybren)
Sebastian Parborg (zeddb)
Brecht Van Lommel (brecht)

Maniphest Tasks

T74927: very slow playback if activated thee Auto normalization

Commits

rBd0b0a0a894e0: Fix T74927: Slow playback using Auto Normalization

Summary

Caused by rBedb3b7a323a1: Fix T67274: Graph Editor Normalization broken by Keyframe that uses Easing or…

Using evaluate_fcurve_only_curve actually causes quite a bit of slowdown [6x] compared to bezier forward differencing [which was used prior to rBedb3b7a323a1]
But full fcurve evaluation is desired with Dynamic Interpolation Effects [Back/Elastic] since their min/max will not to captured with forward differencing.

So now gain back speed [using bezier forward differencing] and only do the full fcurve evaluation for dynamic interpolation effects.

Diff Detail

Repository: rB Blender

Event Timeline

Philipp Oeser (lichtwerk) created this revision.Mar 20 2020, 3:41 PM

Philipp Oeser (lichtwerk) added projects: Animation & Rigging, User Interface.Mar 20 2020, 3:43 PM

Harbormaster completed remote builds in B7241: Diff 22975.Mar 20 2020, 3:45 PM

IIRC, I added evaluate_fcurve_only_curve because the result would not be correct on certain setups with the older method.

I don't have time to test this again right now though.

@Sebastian Parborg (zeddb), please make some time next week to for testing it.

@Brecht Van Lommel (brecht) I will do this on monday.

However, it got me thinking. Perhaps we shouldn't re-evaluate the curve max/min values if it is not needed?

This making playback that much slower indicates that we do this each frame.
Perhaps we should look into this so we could perhaps optimize playback further by caching results?

In D7196#172303, @Sebastian Parborg (zeddb) wrote:

@Brecht Van Lommel (brecht) I will do this on monday.

However, it got me thinking. Perhaps we shouldn't re-evaluate the curve max/min values if it is not needed?

This making playback that much slower indicates that we do this each frame.
Perhaps we should look into this so we could perhaps optimize playback further by caching results?

That might be good for the future [since this is also called on each draw], but it is not obvious (to me) when exactly this should be cached [fcurve data can be changed from many places...]
But for now, I would not mix fixes with future improvements tbh.

IIRC, I added evaluate_fcurve_only_curve because the result would not be correct on certain setups with the older method.

It would be good to have an example here.

Philipp and I discussed this some more.

This fix will make break when using the "Back/Elastic" interpolation method (as it will take the min/max values from the bezier curve data):

I guess if we do not want to look into or implement a caching system, we can simply add a few more code paths to handle these two types as well.
Perhaps this is preferable?

I don't know how much work there would be to create a good caching system for the fcurves, but if we go down that route I think we might get a some nice performance gains during playback.
However this can of course be done much later if we decide to do a smaller fix for this specific issue first.

Current master (Debug build) gives me ~6fps
Reverting rBedb3b7a323a1 (Debug build) gives me ~36fps

Please don't do benchmarks with debug builds. They're not representative of release build performance.

should we consider only keeping the "fixing" part of rBedb3b7a323a1 and revert back to the "old" method (using BKE_curve_forward_diff_bezier) that was used prior?

Sounds good to me. Ideally those two should have already been in two separate commits.

I had some grumpy remarks about the code quality (function should be split up, undocumented magic number 120 used), but since you're just bringing back old code I deleted the remarks again.

In D7196#172402, @Sebastian Parborg (zeddb) wrote:

This fix will make break when using the "Back/Elastic" interpolation method (as it will take the min/max values from the bezier curve data):

Not sure what you mean, the image looks fine to me.

This revision is now accepted and ready to land.Mar 23 2020, 2:32 PM

In D7196#172439, @Sybren A. Stüvel (sybren) wrote:

In D7196#172402, @Sebastian Parborg (zeddb) wrote:

This fix will make break when using the "Back/Elastic" interpolation method (as it will take the min/max values from the bezier curve data):

Not sure what you mean, the image looks fine to me.

Note the amplitude range on the left image (this patch).
It is not between +/-1.0.

I can make this even more extreme of course. I can make the left appear as if was a strait line too as the normalization code is not working at all for this case now.

Thx all for looking at this.

To me it looks like we have the following choices:

(1) accept this patch as-is [will result in amplitudes not forced into -1 to 1 for Back/Elastic], all interpolation methods back to previous speed
(2) force the correct fcurve evaluation [slow] only for Back/Elastic [revert back to old method for other interpolation methods, back to previous speed for these]
(3) do nothing for the moment, spend time on fcurve (normalization) caching [this would for sure make graph editor snappier in this particular case]

@Sybren A. Stüvel (sybren): I know you already accepted, could you place your vote once more? [I would probably go for (2) atm., would update patch accordingly]

I'd go for (2).