F-Curve: Noise Modifier clamp depth
Changes PlannedPublic
Actions

Authored by Aaron Carlisle (Blendify) on Nov 4 2020, 8:56 AM.

Details

Reviewers

Summary

Clamp the depth to 16, numerical errors are present for values over 30 which could produce NAN results. This is the range for other areas of Blender like the noise node.

There should be no functional changes.

Diff Detail

Event Timeline

Aaron Carlisle (Blendify) requested review of this revision.Nov 4 2020, 8:56 AM

Aaron Carlisle (Blendify) created this revision.

Note, the range clamping is a bug fix and could be committed separately to 2.91 if it is safe to do so.

Split updating the function to D9455

What problem is this solving? Why is this the best solution for that problem?

This revision now requires changes to proceed.Nov 5 2020, 10:06 AM

The problem this solves, is if you if you add a noise modifier and increase the depth to a certain point the animation data will become NAN due to floating point precision. This is commented in the old texture code:

/* noisedepth MUST be <= 30 else we get floating point exceptions */

I clamped to 16 and not 30 to match the shader node.

An additional fix would be to add CLAMP_MAX(oct, 30); to BLI_gTurbulence/BLI_turbulence

I decided best to also clamp in the code

If there are functions that misbehave when their input is over a certain threshold, it certainly would help when that limitation is documented & checked for. At the minimum a BLI_assert(param < 30 || !"some message here") would be nice.
I don't think the proper solution is to limit the property in RNA only; making the RNA hard limit follow the limits set (and documented) by the rest of the code would be the correct approach IMO.