-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
smoothstep returning out of range values for 0 length steps #68128
Comments
It would be nice if this was also reflected on the docs. I don't think it's currently 100% up to date with how the function works since the
(There's a typo too btw: ...based on the where x... should be ...based on where the x...) From a glance, it seems that at least in OpenGL
So if we expect the same (which the current doc alludes to), the function could fall back to conditionals when the edges are close to equal. Something like if (is_equal_approx(p_from, p_to)) {
if (p_s <= p_from)
return 0;
if (p_s >= p_to)
return 1;
return 0.5;
} Not sure if it's any better though. |
I think I agree with the function you wrote, except the 0.5 result should almost never happen so you can drop an if statement and always give 0 or 1. |
I guess the check for equality is to avoid a division by zero in the next line: Lines 392 to 405 in 604abb4
I would probably just clamp
|
That's going to give you a headache if p_to is less than p_from ... |
That could be changed with one extra if. Taking inspiration from the latter function, move_toward: ...
float range = abs(p_to - p_from) > CMP_EPSILON? p_to - p_from : SIGN(p_to - p_from) * CMP_EPSILON;
... Admittedly, it's getting quite messy... |
To stir the pot a little more, it looks like the shaders smoothstep defaults to
|
If the values are 'equal' we have no reliable way of knowing whether they are ascending or descending (if they do differ it is likely because the 'same' value was arrived at via a different sequence of equations and the delta is nothing more than rounding errors) so we don't know whether greater or less than should be the 0. A lot of times what I really want here is an error, at least in debug mode, because it means I've screwed up. I should not be calling this function with these values. Whatever is chosen, however, if I multiply it by 255 and insert it into a colour channel it should not be possible for it to overflow into a different channel. If I mutiply the return value by the length of an array it should always give a valid index. I've made a mistake, but I'd like to fail gracefully. |
That's not a valid input. In such a case we should just error and return
For shaders it is undefined behaviour, see: https://registry.khronos.org/OpenGL-Refpages/gl4/html/smoothstep.xhtml In my experience the result can really be anything. Some devices handle it gracefully while others give you numbers that make no sense. For us, I would just add an error condition when Final code for float version would be:
|
If my memory serves correctly there was some discussion somewhere about the doc being a bit unspecific about the undefined behaviour of smoothstep. The only real error case is when from and to are equal or close to equal. When to is less than from the function just "flips" for lack of a better word. I've seen this "trick" used quite often in shaders. E.g:
That said if Wikipedia is to trust, the definition does assume from being the smaller of the two and at least, for example, Unity disallows using lesser to (defaults to returning 0). Personally, I favour the "flipping" behaviour and would only error out on equal inputs, but it's not really that big of a deal either way. It might even be better to follow the norm if for some reason the shader implementation changes in the future. |
I need to chime in here (how has this been untouched for a year?) The function is also quite incorrectly documented. SmoothStep doesn't "interpolate a value", it returns a value from 0 to 1 (except in the edge case discussed here). I like the suggestion of using 0.5 if the values are approximately equal. If the extra cases of the value being inside and outside the range can be added (so logical instead of analytical solution), that would be good instead of trying to make the code maximally branchless like for a GL. The documentaion error is also there in the C# version: /// <summary>
/// Returns a number smoothly interpolated between <paramref name="from" /> and <paramref name="to" />,
/// based on the <paramref name="weight" />. Similar to <see cref="M:Godot.Mathf.Lerp(System.Single,System.Single,System.Single)" />,
/// but interpolates faster at the beginning and slower at the end.
/// </summary>
/// <param name="from">The start value for interpolation.</param>
/// <param name="to">The destination value for interpolation.</param>
/// <param name="weight">A value representing the amount of interpolation.</param>
/// <returns>The resulting value of the interpolation.</returns>
public static float SmoothStep(float from, float to, float weight)
{
if (Mathf.IsEqualApprox(from, to))
return from;
float num = Math.Clamp((float) (((double) weight - (double) from) / ((double) to - (double) from)), 0.0f, 1f);
return (float) ((double) num * (double) num * (3.0 - 2.0 * (double) num));
} |
This function is definitely wrong in it's current implementation; but a fix has been held up for quite a while trying to find a good implementation. My thoughts are as follows: Degenerate Value (when from == to)It almost certainly makes no difference whether we return 0.0, 0.5, or 1.0, as floats aren't perfectly accurate except in extremely rare/contrived cases. Practically speaking no-one will know how far If I had to pick an answer then I'd pick 0.5 - as it's both splitting the difference, and the solution to the somewhat similar Grandi's series. Range OrderThe current implementation supports both Proposed ImplementationI propose the following implementation based on the thoughts above: static _ALWAYS_INLINE_ double smoothstep(double p_from, double p_to, double p_s) {
if (is_equal_approx(p_from, p_to)) {
return 0.5;
}
double s = CLAMP((p_s - p_from) / (p_to - p_from), 0.0, 1.0);
return s * s * (3.0 - 2.0 * s);
}
static _ALWAYS_INLINE_ float smoothstep(float p_from, float p_to, float p_s) {
if (is_equal_approx(p_from, p_to)) {
return 0.5f;
}
float s = CLAMP((p_s - p_from) / (p_to - p_from), 0.0f, 1.0f);
return s * s * (3.0f - 2.0f * s);
} Proposed DocumentationReturns a smooth cubic Hermite interpolation between When When This S-shaped curve is the cubic Hermite interpolator, given by |
It actually wrong, this means the output is always 0.5 regardless of So correct would be (and i'm ignoring for a minute Malcolms suggestion to reverse the curve if
I really don't think there is a point of returning
|
Ah yes, in the degenerate case you can consider EVERY value to be outside the range, so it's got to be 0.0 or 1.0. If we treat the zero-sized range as "normal" polarity (not inverted) then the correct code is as you stated: if (is_equal_approx(p_from, p_to)) {
return p_s < p_from ? 0.0 : 1.0;
} |
Godot version
4.0.dev (merge #68060)
System information
Windows 11
Issue description
In math_funcs.h the two smoothstep functions return a value clamped between 0.0 and 1.0 except when the from and to values are almost equal, in which case they return the value of from, which could be 7 million for all we know. I'm confident they should return a value between 0.0 and 1.0 instead. Personally I think 0.5 would be a decent compromise.
Steps to reproduce
I'm not reproducing this I'm just eyeballing the code. It's only 3 lines long.
Minimal reproduction project
No response
The text was updated successfully, but these errors were encountered: