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 support for custom MDL nodes #2125

Conversation

krohmerNV
Copy link
Contributor

@krohmerNV krohmerNV commented Nov 27, 2024

The MDL generator is limited in a way that it only supported the mappings defined in the libraries/***lib/genmdl files.
With this pull request we now fully support nodes.
This comes in two flavors:

  1. External .mdl file references:
    We allow to specify an MDL module located in an MDL search path using the file attribute. In combination with the function attribute a particular exported function can be selected. There is one aspect that needs to be considered when using this approach:
  • Input and output names defined in MaterialX could conflict with reserved names in MDL. Therefore, we add a mxp_ prefix in case of such a conflict. The prefix is not applied in general because we want to be able to reference MDL modules without modifying them.
  1. Inline MDL source code:
    By specifying the sourcecode attribute, MDL code can be added directly. The generator will inline this code into the generated MDL code, based on the interface of the corresponding node definition. Here, more aspects need to be considered:
  • all input and input names defined in MaterialX are prefixed with mxp_ to keep things simple for authors that otherwise would need knowledge about the keywords in MDL.
  • because the source code parsed from the MaterialX document contains no line breaks, we need to forbid comments using //. Instead the /* comment */ syntax can be used.
  • to keep these inline source code nodes portable, it is not allowed to import additional MDL modules

Both variants support multiple outputs and for both variants tests have been added to illustrate and validate.
Additionally one more complex procedural material was added that makes use of the inline source code.

@krohmerNV
Copy link
Contributor Author

@jstone-lucasfilm I believe this PR does not really add something new. It just allows to use standard MaterialX functionality provided by the node.

@jstone-lucasfilm
Copy link
Member

This looks very promising, thanks @krohmerNV.

One concern that I have is the addition of language-specific examples to Materials/TestSuite, where previously all of our test suite examples have been language-independent. Since the introduction of that new pattern may take more design thought, I'm thinking that we may want to organize this contribution into two separate steps, where the first step introduces the new logic in MaterialXGenMdl that supports the usage of custom MDL nodes, and the second step is focused on a new design for our test suite that will accommodate language-specific tests.

What are your thoughts?

@krohmerNV
Copy link
Contributor Author

That's a reasonable approach, yes.
Should I remove the test cases for now?

@jstone-lucasfilm
Copy link
Member

@krohmerNV Sounds good!

@krohmerNV krohmerNV force-pushed the krohmer/custom_mdl_nodes branch from 3d3ed4c to 77ad9c2 Compare December 10, 2024 09:43
@krohmerNV
Copy link
Contributor Author

Should I remove the test cases for now?

done

This may be a useful change in the future, but let's omit it from this specific proposal for simplicity.

Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
@jstone-lucasfilm jstone-lucasfilm changed the title MDL: support for user defined node implementations Add support for custom MDL nodes Dec 11, 2024
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
@krohmerNV
Copy link
Contributor Author

Thank you @jstone-lucasfilm for all the fixes!

krohmerNV and others added 3 commits December 12, 2024 09:59
pass string parameter by reference

Signed-off-by: krohmerNV <42233792+krohmerNV@users.noreply.github.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

These changes look good to me, thanks @krohmerNV!

@jstone-lucasfilm jstone-lucasfilm merged commit 94ce95d into AcademySoftwareFoundation:main Dec 12, 2024
34 checks passed
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