Fix #110169: nearly flat normalized fcurves jump around in editor #110796

Nathan Vegdahl · 2023-08-04T15:59:28+02:00

Nathan Vegdahl commented

2023-08-04 15:59:28 +02:00

The root cause was a classic fixed-size epsilon issue. The code that
checked if an fcurve was effectively flat, and thus shouldn't be
normalized, used a fixed-size epsilon that was reasonable for values
close-ish to zero, but didn't work well for values >= 1.0.

This patch addresses the issue by introducing a robust ulps-based
comparison, to ensure that a minimum number of representable floats
exist between the min/max values of the curve. This approach scales
appropriately up and down to both huge and tiny values.

The root cause was a classic fixed-size epsilon issue. The code that checked if an fcurve was effectively flat, and thus shouldn't be normalized, used a fixed-size epsilon that was reasonable for values close-ish to zero, but didn't work well for values >= 1.0. This patch addresses the issue by introducing a robust ulps-based comparison, to ensure that a minimum number of representable floats exist between the min/max values of the curve. This approach scales appropriately up and down to both huge and tiny values.

Nathan Vegdahl added 1 commit 2023-08-04 15:59:42 +02:00

d7f5974ad6 Fix #110169 : nearly flat normalized fcurves jump around in editor

The root cause was a classic fixed-size epsilon issue.  The code that
checked if an fcurve was effectively flat, and thus shouldn't be
normalized, used a fixed-size epsilon that was reasonable for values
close-ish to zero, but didn't work well for values >= 1.0.

This patch addresses the issue by introducing a robust ulps-based
comparison, to ensure that a minimum number of representable floats
exist between the min/max values of the curve.  This approach scales
appropriately up and down to both huge and tiny values.

Nathan Vegdahl requested review from Christoph Lendenfeld 2023-08-04 16:01:34 +02:00

Nathan Vegdahl requested review from Brecht Van Lommel 2023-08-04 16:01:48 +02:00

Nathan Vegdahl commented

2023-08-04 16:06:36 +02:00

I wrote a new ulps-based function ulp_diff_ff() to implement this fix. I think this new function will generally be useful in resolving certain kinds of floating point issues in the future as well. The advantage of it over the existing compare_ff_relative() is that the latter also includes a standard epsilon-based comparison, and if the epsilon is zero the ulps-based comparison doesn't work correctly between floats of different sign. So this new function allows robust pure ulps-based thresholds to be used.

However, I can sometimes be a little overzealous about floating point issues. So maybe this is overkill. I'm also not totally sure if I've even put the function in the best place (e.g. maybe one of the new C++ files would be a better place for new functions?).

I wrote a new ulps-based function `ulp_diff_ff()` to implement this fix. I think this new function will generally be useful in resolving certain kinds of floating point issues in the future as well. The advantage of it over the existing `compare_ff_relative()` is that the latter also includes a standard epsilon-based comparison, and if the epsilon is zero the ulps-based comparison doesn't work correctly between floats of different sign. So this new function allows robust pure ulps-based thresholds to be used. However, I can sometimes be a little overzealous about floating point issues. So maybe this is overkill. I'm also not totally sure if I've even put the function in the best place (e.g. maybe one of the new C++ files would be a better place for new functions?).

Nathan Vegdahl added the

Module

Animation & Rigging

label 2023-08-04 16:13:04 +02:00

Nathan Vegdahl added this to the Animation & Rigging project 2023-08-04 16:13:11 +02:00

Brecht Van Lommel commented

2023-08-04 16:37:03 +02:00

Can we use ulp_diff_ff in the implementation of compare_ff_relative, since it seems like it would correct some issues there?

I think there's usually a good reason to do both absolute and relative comparisons like compare_ff_relative. Relative differences don't work well near zero, there can be bigger errors when the value comes from a computation that involved bigger values.

Can we use `ulp_diff_ff` in the implementation of `compare_ff_relative`, since it seems like it would correct some issues there? I think there's usually a good reason to do both absolute and relative comparisons like `compare_ff_relative`. Relative differences don't work well near zero, there can be bigger errors when the value comes from a computation that involved bigger values.

Christoph Lendenfeld commented

2023-08-04 16:52:24 +02:00

ulp_diff_ff(min_coord, max_coord) > 64 this means there are 64 floating point steps between min and max?
I think we can bump that even further since the drawing is still a bit weird at that value
The difference between the values here is 0.00001

`ulp_diff_ff(min_coord, max_coord) > 64` this means there are 64 floating point steps between min and max? I think we can bump that even further since the drawing is still a bit weird at that value The difference between the values here is 0.00001 ![image](/attachments/279b6efb-a6d8-4d2b-ab28-3f511c5f41f4)

image.png

18 KiB

Nathan Vegdahl commented

2023-08-04 17:44:13 +02:00

Can we use ulp_diff_ff in the implementation of compare_ff_relative, since it seems like it would correct some issues there?

Will do.

I think there's usually a good reason to do both absolute and relative comparisons like compare_ff_relative. Relative differences don't work well near zero, there can be bigger errors when the value comes from a computation that involved bigger values.

Agreed. compare_ff_relative definitely has its place as well. But in some cases (such as this fix, I think) a pure ulp-based approach is more appropriate. It's good to have both available.

> Can we use `ulp_diff_ff` in the implementation of `compare_ff_relative`, since it seems like it would correct some issues there? Will do. > I think there's usually a good reason to do both absolute and relative comparisons like `compare_ff_relative`. Relative differences don't work well near zero, there can be bigger errors when the value comes from a computation that involved bigger values. Agreed. `compare_ff_relative` definitely has its place as well. But in some cases (such as this fix, I think) a pure ulp-based approach is more appropriate. It's good to have both available.

Nathan Vegdahl commented

2023-08-04 17:47:41 +02:00

I think we can bump that even further since the drawing is still a bit weird at that value

Good call. I was trying to be conservative, but I think you're right that I was being over-conservative in this case.

> I think we can bump that even further since the drawing is still a bit weird at that value Good call. I was trying to be conservative, but I think you're right that I was being over-conservative in this case.

Nathan Vegdahl added 1 commit 2023-08-04 18:19:44 +02:00

e5d2c923d1 Fix: compare_ff_relative() could be wrong with floats of different signs

Specifically, when relying on the ulps code path.  This commit changes
it to use the more robust ulp_diff_ff() for its ulps code path.

Nathan Vegdahl added 1 commit 2023-08-04 18:35:32 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

f43ee5d23e Bump up the minimum ulp difference to normalize an fcurve

Brecht Van Lommel approved these changes 2023-08-04 20:26:26 +02:00

Brecht Van Lommel commented

2023-08-04 20:26:50 +02:00

@blender-bot build

Nathan Vegdahl merged commit d4d0daa934 into main

2023-08-07 10:53:10 +02:00

Nathan Vegdahl deleted branch fix_110169_broken_fcurve_normalization

2023-08-07 10:53:11 +02:00

Nathan Vegdahl referenced this issue from a commit

2023-08-07 10:53:12 +02:00

Fix #110169: nearly flat normalized fcurves jump around in editor

Nathan Vegdahl referenced this pull request

2023-08-07 11:13:43 +02:00

Broken FCurve rendering #110169

Hannes Loeschke referenced this issue from a commit

2023-08-08 17:36:13 +02:00

Fix #110169: nearly flat normalized fcurves jump around in editor

Christoph Lendenfeld referenced this pull request

2023-10-06 17:02:01 +02:00

Broken curve drawing in normalized view in graph editor #97485

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #110169: nearly flat normalized fcurves jump around in editor #110796