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

Nodes: Rename remainder() to modInt(). #29092

Merged
merged 8 commits into from
Aug 11, 2024

Conversation

cmhhelgeson
Copy link
Contributor

@cmhhelgeson cmhhelgeson commented Aug 9, 2024

Description

Between WGSL and GLSL, there are differences in how the commonly used modulus operation is handled. GLSL has a function mod() that computes x modulo y. It does not permit the use of the % operator to perform the same operation. Meanwhile, WGSL does allows the use of the % operator to compute the modulus of two numbers, and therefore has no equivalent function mod. Therefore, when the % operator is used in a TSL shader via the remainder() function, that shader will be valid for the renderer's WebGPUBackend, but invalid for its WebGLBackend. While a user could simply write their shader to use remainder or mod depending on what graphics API they are targeting, this solution negates the inherent portability of TSL shaders across shader languages.

Modified to simply rename remainder and mod to make their usage more clear.

Copy link

github-actions bot commented Aug 9, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
685.1 kB (169.6 kB) 685.1 kB (169.6 kB) +0 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
462 kB (111.4 kB) 462 kB (111.4 kB) +0 B

@sunag
Copy link
Collaborator

sunag commented Aug 9, 2024

remainder() is used for integer, and mod() for float. You tried replace remainder to mod() in your example? I think that rename remainder() to modInt() would be less confuse instead of change it.

@cmhhelgeson
Copy link
Contributor Author

remainder() is used for integer, and mod() for float. You tried replace remainder to mod() in your example? I think that rename remainder() to modInt() would be less confuse instead of change it.

Yes that fixed it. I did also try an arbitrary modulus with WebGL and that worked fine. I'm not sure why so much documentation seems to indicate that % is not allowed in GLSL at all? If it's okay, can I just change this pull request to implement the suggested name change.

@sunag
Copy link
Collaborator

sunag commented Aug 9, 2024

If it's okay, can I just change this pull request to implement the suggested name change.

Sounds great!

@cmhhelgeson
Copy link
Contributor Author

If it's okay, can I just change this pull request to implement the suggested name change.

Sounds great!

Ok cool, I'll also make sure there's a deprecation option per the new standards.

@sunag
Copy link
Collaborator

sunag commented Aug 9, 2024

Could you keep mod() name? I think just rename remainder() to modInt() is better.

@cmhhelgeson cmhhelgeson changed the title Nodes: Fix OperatorNode remainder() function when forceWebGL is set to true Nodes: Rename remainder() to modInt(). Aug 9, 2024
@sunag sunag added this to the r168 milestone Aug 10, 2024
@@ -303,4 +303,14 @@ addNodeElement( 'bitXor', bitXor );
addNodeElement( 'shiftLeft', shiftLeft );
addNodeElement( 'shiftRight', shiftRight );


export const remainder = ( ...params ) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think could be good add what release it was deprecated

export const remainder = ( ...params ) => { // @deprecated, r168

@sunag sunag changed the title Nodes: Rename remainder() to modInt(). Nodes: Rename remainder() to modInt(). Aug 11, 2024
@sunag sunag merged commit 6114fcc into mrdoob:dev Aug 11, 2024
12 checks passed
@cmhhelgeson cmhhelgeson deleted the operator_remainder_fix branch August 14, 2024 21:23
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.

2 participants