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 fractal2d node #2183

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ld-kerley
Copy link
Contributor

Mirroring the existing behavior for <fractal3d>, we add the corresponding <fractal2d> node that uses the texcoord` input as its manifold.

Add support for OSL, MSL, GLSL, MDL (note : MDL not tested).

Updated specification documents to add <fractal2d> to the specification and remove it from the proposals document.

Also remove period inputs for the noises that don't have those inputs in the specification. This is already documented in the proposals document.

Note : in the GLSL noise library code I renamed the existing mx_fractal_noise_xxx() functions to mx_fractal3d_noise_xxx() to make the differentiation cleaner with the new mx_fractal2d_noise_xxx(). But I retained wrapper functions with the old name that just call through to the new name for backwards compatibility. I was unsure how much custom customer code might be referencing those functions. We can remove the shim functions when we have another breaking API release.

@jstone-lucasfilm jstone-lucasfilm changed the title Add <fractal2d> node Add fractal2d node Jan 23, 2025
return float4(c.x, c.y, c.z, a);
}

// TODO - do we want to rename these to mx_fractal3D_noise_float - if so where else might things need to be updated
Copy link
Member

Choose a reason for hiding this comment

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

That rename sounds logical to me, and I'm CC'ing @krohmerNV for his thoughts from the MDL side.

@krohmerNV
Copy link
Contributor

krohmerNV commented Jan 27, 2025

MDL is broken in this state!
I'll prepare a fix and if you are okay, I'll try to push it your branch.
One question though. It seems you copied all the 3d functions with only one change:
in 2D there is an position offset of: mxp_p+float3(19, 193)
while in 3D, these is: mxp_p+float3(19, 193, 17)
do we really need this difference here? we could use the same functions if you pass the 17 as well in 2D. Since it's a new function, I don't see why not. (probably the same for the other backends of course to have matching results)

@krohmerNV
Copy link
Contributor

krohmerNV commented Jan 27, 2025

MaterialX-mdl-fractal2d-node.patch
given the time zone difference and the scope of the patch, you can apply it easily yourself.
If you agree with the proposal above to use the same function for 2D and 3D, let me know. I can help out as well.

@ld-kerley
Copy link
Contributor Author

I applied the patch provided by @krohmerNV. I didn't realize that we have no MDL evaluation tests in the CI - we should try and work towards fixing that.

On the topic of reusing the 3D noise calls to provide the 2D fractal node - If I'm understanding the code - the 3D version is more expensive - so I think maybe it does make sense to keep separate 2D and 3D versions. Certainly I would propose we leave that work to a follow up PR where someone can profile the changes across all the languages and noise types - and in some cases it might be a win - I'm not sure.

@jstone-lucasfilm
Copy link
Member

@ld-kerley On MDL code validation using mdlc, this has long been a goal for the MaterialX project, and I believe @krohmerNV has been steadily working towards this goal!

On the proposed change to 3D noise functions, I think Kai is referring to the TODO above, which suggests that mx_fractal_noise_float would be renamed to mx_fractal3D_noise_float for consistency with the functions that you've added here.

I think that's a great idea, if you still support the idea of this rename, and it would make this changelist more complete and self-consistent.

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