Skip to content
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

docs: Fix specification clarity of smoothstep #1851

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Aug 19, 2024

Swap the confusing edge0/edge1 nomenclature of the smoothstep
declaration to the more intuitive low/high.

It was pointed out on Slack by Arnon Marcus that the spec's
description of smoothstep was ambiguous about the behavior when
low==high: does it return 0 or 1 if x is the same value? The
documentation is unclear.

The implementation returned 1, which I think is the "correct" behavior
in the sense that it matches the results of step() function with that
edge. So update the documentation to match.

Also Arnon pointed out that things are especially weird if low > high,
it's non-monotonic. This seems to be fixed by simply reversing the
relative order of the if x < low and if x >= high tests:
basically, it also makes it match step(x, high) and be monotonic.

This is a cleaner formal definition of what smoothstep should do,
namely:

if (x >= high) {
    return 1.0f;
} else if (x < low) {
    return 0.0f;
} else {
    float t = (x - low) / (high - low);
    return (3.0f-2.0f*t)*(t*t);
}

@HardCoreCodin
Copy link

HardCoreCodin commented Aug 19, 2024

There would still be ambiguity when edge1 < x < edge0 or when edge1 = x < edge0.
So the distinction of < vs. <= is not the only source of ambiguity.
It is regarding which of the 2 comparisons is done first in code - regardless of < vs. <=.
For example, even with changing the spec to use < instead of <= for the low case, when edge1 < x < edge0 (i.e: edge1=2, x=3, edge0=4) or when edge1 = x < edge0 (i.e: edge1=3, x=3, edge0=4):
This implementation would return 0:

    if (x < e0) return 0.0f;
    else if (x >= e1) return 1.0f;
    else {
        float t = (x - e0)/(e1 - e0);
        return (3.0f-2.0f*t)*(t*t);
    }

This implementation would return 1:

    if (x >= e1) return 1.0f;
    else if (x < e0) return 0.0f;
    else {
        float t = (x - e0)/(e1 - e0);
        return (3.0f-2.0f*t)*(t*t);
    }

Both implementations comply with the same updated specification as proposed, and yet - they would return different results.
A better phrasing might be to incorporate ... . Otherwise, if ... somewhere in the spec, thereby implying an ordering to the checks.

@lgritz
Copy link
Collaborator Author

lgritz commented Aug 19, 2024

It was clearly the case that the prior < low vs <= low generated uncertainty as to the actual return value when val==low==high (is it 0 or is it 1?).

Oh, I see, you're talking about cases where they intentionally pass high < low?

@lgritz
Copy link
Collaborator Author

lgritz commented Aug 19, 2024

Would it help if the spec explicitly spelled out the code equivalent to show the order of comparisons?

    if (x < low) return 0.0f;
    else if (x >= high) return 1.0f;
    else {
        float t = (x - low)/(high - low);
        return (3.0f-2.0f*t)*(t*t);
    }

I think the desired behavior when low==high, matching step() that has only one threshold value, was the clear choice.

I admit that I haven't thought as carefully about what is the best behavior when somebody passes the nonsensical high < low. Is the above code adequate? I'm thinking maybe not. How about switching the order of comparisons:

    if (x >= high) return 1.0f;
    else if (x < low) return 0.0f;
    else {
        float t = (x - low)/(high - low);
        return (3.0f-2.0f*t)*(t*t);
    }

I think that makes it monotonic no matter what the relative order of low and high, right? We can say in this case that if high < low, it also falls back to matching step() with a threshold of high (which in this case is the lower of the two values).

An alternative specification would be to implicitly swap low and high if low > high. In other words, you just have two thresholds, specified in either order, and it's 0 when lower than the lowest threshold, and interpolates to 1 as it goes from the lower to the higher edge threshold. I like the elegance of that, but I don't see how to do it without adding expense, since it would require an additional conditional to detect the low>high case.

@HardCoreCodin
Copy link

Swapping would not only make it more expensive, it would also make it inconsistent - the "non sensical" case(s) occur naturally and implicitly when using image texture maps (even very simple ones). You could make an argument that this function is not aimed to be used with images, but that would not be a very strong argument.

I am not sure there necessarily even is a right or wrong ordering (I haven't thought hard on that and have no stake in it), what matters is that the specification states whichever is chosen to be the desired behavior, as opposed to leaving it to interpretation or arbitrary implementation choice.

@lgritz
Copy link
Collaborator Author

lgritz commented Aug 19, 2024

Yeah, I think the simplest solution is just to test >= high first. I believe that leads to consistent behavior, and also it's easy to explain that smoothstep(low,high) == step(high) any time that high <= low.

@lgritz
Copy link
Collaborator Author

lgritz commented Aug 21, 2024

Updated:

Swapped the relative order of the >=high and <low tests in order to make the high<low case also be sane.

Swap the confusing edge0/edge1 nomenclature of the smoothstep
declaration to the more intuitive low/high.

It was pointed out on Slack by Arnon Marcus that the spec's
description of smoothstep was ambiguous about the behavior when
low==high: does it return 0 or 1 if x is the same value? The
documentation is unclear.

The implementation returned 1, which I think is the "correct" behavior
in the sense that it matches the results of step() function with that
edge. So update the documentation to match.

Also Arnon pointed out that things are especially weird if low > high,
it's non-monotonic.  This seems to be fixed by simply reversing the
relative order of the `if x <= low` and `if x >= high` tests:
basically, it also makes it match step(x, high) and be monotonic.

This is a cleaner formal definition of what smoothstep should do,
namely:

    if (x >= high) {
        return 1.0f;
    } else if (x <= low) {
        return 0.0f;
    } else {
        float t = (x - low) / (high - low);
        return (3.0f-2.0f*t)*(t*t);
    }

Signed-off-by: Larry Gritz <[email protected]>
@lgritz
Copy link
Collaborator Author

lgritz commented Aug 23, 2024

Comments?

@lgritz
Copy link
Collaborator Author

lgritz commented Sep 3, 2024

Last chance for objections...

Copy link
Contributor

@sfriedmapixar sfriedmapixar left a comment

Choose a reason for hiding this comment

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

Aside from a typo in stdlib.md, LGTM

: Returns 0 if `x` $\le$ `edge0`, and 1 if `x` $\ge$ `edge1`, and performs a
linear interpolation between 0 and 1 when `edge0` $<$ `x` $<$ `edge1`.
This is equivalent to `step(edge0, x)` when `edge0 == edge1`. For `color`
: Returns 0 if `x` $<$ `low`, and 1 if `x` $\ge$ `high`, and performs a
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be x $\le$ low to match the .tex

Choose a reason for hiding this comment

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

I still feel the need to point out that any phasing of the form " returns 0 if x something low, and 1 if x something high" is going to remain ambiguous, because these are 2 separate checks against 2 separate inputs(!) which necessarily makes it possible for both conditions to be true - regardless of what comparison is used - at which point the phrasing is not specifying what should happen (which of the 2 satisfied conditions should determine the output).

jstone-lucasfilm pushed a commit to AcademySoftwareFoundation/MaterialX that referenced this pull request Sep 5, 2024
Following a conversation on ASWF slack this PR was posted to OSL refining the edge cases for `smoothstep()`.

AcademySoftwareFoundation/OpenShadingLanguage#1851

This PR aligns the GLSL/MSL code with this change.
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