[glsl-in] Implement MathFunction::Clamp
#1502
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cwfitzgerald
added
area: front-end
cwfitzgerald
changed the title
MathFunction::ClampMathFunction::Clamp
jimblandy
requested changes
Nov 2, 2021
src/front/glsl/constants.rs
Outdated
| }, | ||
| width, | ||
| ), | ||
| _ => return Err(ConstantSolvingError::InvalidMathArg), |
There was a problem hiding this comment.
Generally, Naga IR follows the WGSL spec, and that says that the clamp function operates "component-wise when [the argument type] is a vector". It's fine not to bother handling vector arguments for now, but this should be a NotImplemented error, not an InvalidMathArg error.
There was a problem hiding this comment.
Done, though I haven't handled vector arguments yet.
JCapucho
requested changes
Nov 7, 2021
Change `ConstantSolvingError::InvalidMathArg` to `ConstantSolvingError::NotImplemented`
for the clamp function with the input of unimplemented types.
Unimplemented types:
- vec2
- vec3
- vec4
sjinno
force-pushed
the
1498-implement-clamp
branch
from
f47e1c8
to
0e0d885
Compare
sjinno
force-pushed
the
1498-implement-clamp
branch
3 times, most recently
from
b32c826
to
40ce0b9
Compare
…ions. Co-authored-by: JCapucho <jcapucho7@gmail.com>
sjinno
force-pushed
the
1498-implement-clamp
branch
from
40ce0b9
to
2d16938
Compare
JCapucho
approved these changes
Mar 17, 2022
jimblandy
approved these changes
Apr 11, 2022
src/front/glsl/constants.rs
Outdated
Comment on lines
562
to
564
| /// While rust does provide a `f64::max` method it has a different behavior than the | ||
| /// glsl `max` for `NaN`s, in rust if any of the arguments is a `NaN` then the other | ||
| /// is returned. |
There was a problem hiding this comment.
Capitalize "Rust", and write "GLSL" in all caps. Also, this is a run-on sentence; please change ", in rust if" to ". In Rust, if"
src/front/glsl/constants.rs
Outdated
| /// std::f64::NAN.max(1.0); | ||
| /// ``` | ||
| /// | ||
| /// Rust will return 1.0 while glsl should return NaN |
There was a problem hiding this comment.
Missing period at end of sentence.
src/front/glsl/constants.rs
Outdated
| } | ||
| } | ||
|
|
||
| /// Helper function to implement the glsl `min` function for floats |
There was a problem hiding this comment.
Same comments for glsl_float_min as glsl_float_max.
sjinno
force-pushed
the
1498-implement-clamp
branch
from
38a4e29
to
2ddd0d1
Compare
sjinno
force-pushed
the
1498-implement-clamp
branch
from
2ddd0d1
to
941d39a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As the title says, added the implementation of
MathFunction::Clamp. Feedback is very much appreciated! :)