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

Add <fract> node #2169

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

Conversation

pablode
Copy link
Contributor

@pablode pablode commented Jan 4, 2025

As discussed, this PR adds a node that uses the intrinsic of GLSL/MSL/MDL.

(Note: not tested with MDL & OSL, but should match their specifications.)

@jstone-lucasfilm
Copy link
Member

This looks great to me, thanks @pablode, and I'm CC'ing @dbsmythe, @anderslanglands, and @ld-kerley for their thoughts.

To address the testing errors, you'd just need to add a single example of each type variant of fract in the Materials/TestSuite folder, following the pattern for modulo seen here:

https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/resources/Materials/TestSuite/stdlib/math/math_operators.mtlx#L325

Node: <fract>
The fraction of a float or vector.
-->
<nodedef name="ND_fract_float" node="fract" nodegroup="math">
Copy link
Contributor

Choose a reason for hiding this comment

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

for other operators we provide node definitions for both the color3/4 and vector2/3/4 data types. Has it already been discussed elsewhere? Is there a reason to not also include versions here for color3/4?

Copy link
Contributor Author

@pablode pablode Jan 7, 2025

Choose a reason for hiding this comment

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

Good observation. I was hesitant to add these definitions since defining math operations (e.g. modulo) on colors felt a bit sketchy. What if color means "wavelengths" in the future? Besides, the values are usually normalized in the [0, 1] range, making the operator less useful.

But yes, I'll add them with the next push.

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