Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

docs: Fix specification clarity of smoothstep #1851

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

Open
lgritz wants to merge 1 commit into AcademySoftwareFoundation:main
base: main
Choose a base branch
Loading
from lgritz:lg-smoothstep

Conversation

@lgritz
Copy link
Collaborator

@lgritz lgritz commented Aug 19, 2024
edited
Loading

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);
}

Copy link

HardCoreCodin commented Aug 19, 2024
edited
Loading

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.

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?

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.

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.

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.

ld-kerley reacted with thumbs up emoji

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 <lg@larrygritz.com>
Copy link
Collaborator Author

lgritz commented Aug 23, 2024

Comments?

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

@sfriedmapixar sfriedmapixar Sep 3, 2024

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

Copy link

@HardCoreCodin HardCoreCodin Sep 10, 2024

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

Reviewers

2 more reviewers

@HardCoreCodin HardCoreCodin HardCoreCodin left review comments

@sfriedmapixar sfriedmapixar sfriedmapixar requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /