Skip to content

Conversation

@mbouron
Copy link
Contributor

@mbouron mbouron commented May 12, 2025

No description provided.

@mbouron mbouron force-pushed the to-upstream-v6 branch 2 times, most recently from 92affc3 to cd8b6fb Compare May 12, 2025 07:17
Base automatically changed from to-upstream-v6 to main May 12, 2025 22:39
*
* See: https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:compose/ui/ui-util/src/commonMain/kotlin/androidx/compose/ui/util/MathHelpers.kt;l=160
*/
static float fast_cbrt(float x)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the native cbrtf? (you can also drop the android copyright then)

Comment on lines 368 to 370
if (double_close_to(d, 0.0, FLT_EPSILON)) {
if (double_close_to(a, 0.0, FLT_EPSILON)) {
if (double_close_to(b, 0.0, FLT_EPSILON)) {
Copy link
Contributor

@ubitux ubitux May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to prevent a division by zero you can check directly against 0.0, it is enough to cover all NaN cases (assuming the numerator is finite). So I'm assuming this check is to prevent an Inf value (caused by overflows). Unfortunately you can't check for it like that. For example 3e-16 will pass the epsilon check, but 1e+308/3e-16 will make an Inf.

Note

You probably wanted to use DBL_EPSILON here since you're working with double precision. But in general, epsilon is never a good threshold against NaN/Inf (and actually using double epsilon here will make it worse).


static int double_close_to(double value, double ref, double eps)
{
return fabs(value - ref) < eps;
Copy link
Contributor

@ubitux ubitux May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref and eps are always the same in your calls, so it could probably be called close_to_zero and take only one parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 477 to 479
const double da = 3.0f * (b - a);
const double db = 3.0f * (c - b);
const double dc = 3.0f * (d - c);
Copy link
Contributor

@ubitux ubitux May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use f suffix if you're working with doubles?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

static float clamp_root(float r)
{
const float s = NGLI_CLAMP(r, 0.f, 1.f);
return fabsf(s - r) > FLT_EPSILON ? NAN : s;
Copy link
Contributor

@ubitux ubitux May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLT_EPSILON is, as far as I can tell, not a very good threshold choice as it's probably going to filter out all sorts of cases where the root is close to 0 or 1: it's a very restrictive value, and at the same time not 100%. A more arbitrary 1e-6 (that can be understood as a "precision" rounding) will likely do a better job.

Comment on lines +363 to +366
double a = 3.0 * (p0 - 2.0 * p1 + p2);
double b = 3.0 * (p1 - p0);
double c = p0;
double d = -p0 + 3.0 * (p1 - p2) + p3;
Copy link
Contributor

@ubitux ubitux May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'm assuming this is the cubic points being converted to polynomial coefficients, but the value are weirdly shuffled. You made your polynomial $dx^3+ax^2+bx+c$, which I'm assuming you did because you're making it monic later down the line and reusing a/b/c naming. I think it would make sense to keep it as $ax^3+bx^2+cx+d$ for clarity, then pass it down to another function as $f(\frac{b}{a}, \frac{c}{a}, \frac{d}{a})$ where you can name again the parameters a, b and c.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented it so it matches the ref mentioned in the comment, see: https://pomax.github.io/bezierinfo/#yforx

Comment on lines +375 to +382
double q = sqrt(b * b - 4.0 * a * c);
double a2 = 2.0 * a;

float root = clamp_root(((float) ((q - b) / a2)));
if (!isnan(root))
return root;

return clamp_root(((float) ((-b - q) / a2)));
Copy link
Contributor

@ubitux ubitux May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: there is a simpler quadratic formula which we're using in distmap where you find a middle point $m=-\frac{b}{2a}$ that you offset by $±\sqrt{\Delta}$ where $\Delta=m^2-\frac{c}{a}$ to get each root.

Since there are less subtractions, I also believe it leads to a lower risk of catastrophic cancellation (to be verified though).

}
return clamp_root((float) (-c / b));
} else {
double q = sqrt(b * b - 4.0 * a * c);
Copy link
Contributor

@ubitux ubitux May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing a check if the $b^2-4ac &lt; 0$

double u1 = fast_cbrt((float)(-q2 + sd));
double v1 = fast_cbrt((float)(q2 + sd));

return clamp_root((float)(u1 - v1 - a3));
Copy link
Contributor

@ubitux ubitux May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that in my experience the analytic cubic is filled with numerical instability and I wouldn't recommend it... I ended up dropping it from the distmap for a more stable iterative algorithm.

Comment on lines +436 to +439
double a = 1.0 / 3.0 + (p1 - p2);
double b = (p2 - 2.0 * p1);
double c = p1;
return 3.0 * ((a * t + b) * t + c) * t;
Copy link
Contributor

@ubitux ubitux May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend computing and caching the polynomial for faster evaluation.

Comment on lines +447 to +450
const easing_type a = PARAM(0, 0.0);
const easing_type b = PARAM(1, 0.0);
const easing_type c = PARAM(2, 1.0);
const easing_type d = PARAM(3, 1.0);
Copy link
Contributor

@ubitux ubitux May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here is something: with easing, the starting and ending points are always respectively (0,0) and (1,1). If you need some sort of offsetting, you can use easing_start_offset and easing_end_offset.

Next, you can't have the curve going backward, which means $b_x ≤ c_x$ must be verified (maybe this is already the case due to the points being in a single dimension).

All these constraints may also make the solver and friends way more convenient and may avoid a bunch of tests. You also won't need to extend the number of parameter from 2 to 4.

const easing_type c = PARAM(2, 1.0);
const easing_type d = PARAM(3, 1.0);

const easing_type v = NGLI_MAX(t, FLT_EPSILON);
Copy link
Contributor

@ubitux ubitux May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is $t=0$ not allowed? It should just be the starting point

return 1.0;
}

return evaluate_cubic(db, 0.f, r);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

evaluate_cubic() takes double arguments so 0.f should be 0.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

This is needed to support easings based on cubic Bézier curves.
@mbouron mbouron force-pushed the to-upstream-v7 branch 3 times, most recently from 2ea27d9 to 6f59a2f Compare November 11, 2025 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants