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.

Sign up for GitHub

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

Jump to bottom

fix (Nodes): fix type of clamp and saturate #733

Merged
merged 3 commits into from
Jan 10, 2024
Merged

fix (Nodes): fix type of clamp and saturate #733

merged 3 commits into from
Jan 10, 2024

Conversation

0b5vr
Copy link
Contributor

@0b5vr 0b5vr commented Dec 25, 2023 •
edited
Loading

What

@0b5vr 0b5vr added the bug Something isn't working label Dec 25, 2023
@0b5vr 0b5vr self-assigned this Dec 25, 2023
@0b5vr
fix (Nodes): fix type of clamp and saturate
6e3b9dc 
- `clamp` can be unary or binary
  - See: https://github.com/mrdoob/three.js/blob/r160/examples/jsm/nodes/math/MathNode.js#L301
  - I have no confidence of the type name `UnaryOrBinaryOrTernary`. Would you come up with a better name?
- `saturate` is unary
@Methuselah96
Copy link
Contributor

Looks like you may have been working on this change around the same as I made some changes to Ternary. Does this change not fix this? Or do we know for sure that clamp and saturate are special in that they allow missing arguments?

@0b5vr
Copy link
Contributor Author

0b5vr commented Jan 10, 2024

Looks like you may have been working on this change around the same as I made some changes to Ternary. Does this change not fix this? Or do we know for sure that clamp and saturate are special in that they allow missing arguments?

I'm sure other Ternary nodes like refract, smoothstep, or mix doesn't work properly without the second or the third arguments. Tested on https://threejs.org/examples/?q=tsl#webgpu_tsl_editor

saturate is definitely Unary, it doesn't have the second or the third arguments.
clamp has default parameter for the second and the third arguments as I indicated in the link.

@Methuselah96
Copy link
Contributor

Good call, I've updated those to be required again. Not sure why my mind thought they should all be that way.

@Methuselah96 Methuselah96 merged commit d73eca1 into master Jan 10, 2024
3 checks passed
@Methuselah96 Methuselah96 deleted the saturate-is-unary branch January 10, 2024 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Methuselah96 Methuselah96 Methuselah96 approved these changes

Assignees

@0b5vr 0b5vr

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@0b5vr @Methuselah96