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

Improve MDL thin-film support #1282

Conversation

krohmerNV
Copy link
Contributor

  • push down the thin film parameters to the nodes that can use them, i.e., dielectric, conductor and generalized schlick

  • removed additional inputs, that transport thin film parameters, from the public material interface by making them non-editable

  • make sure thin_film will not result in undefined behavior when passing zeros as parameters (0 thickness disables thin_film but 0 ior is invalid)

  • Vertical Layering, push base deeper than one level, needs review, contains TODOs for mix, add, and multiply (If not 100% sure this is correct, we can drop that commit and postpone.

@pablode
Copy link
Contributor

pablode commented Mar 17, 2023

Hello! I'm seeing a regression when I use these changes with my MaterialX/MDL-based renderer:

Before After
latest_master_1 new_pr_1

I've prepared a glTF+mtlx version of this asset in hope that it can be reproduced with the MDL SDK DXR example:
maneki_gltf.zip

@krohmerNV
Copy link
Contributor Author

krohmerNV commented Mar 20, 2023

I can reproduce it. And it showed another problem as well:
When reading the the normal texture, the output is color and connecting that to a float3 in MDL requires and explicit type construction (because color could be spectral).
The MaterialX document is not passing the validation either:

[09:07:28] [MDL_D3D12] [WARNING] [MTLX] Validation warnings for 'S:\content\gltf\maneki\maneki.mtlx'
Mismatched types in port connection: <output name="normal_rgb" type="vector3" output="rgb" nodename="normal">

@jstone-lucasfilm can you tell how such UsdUVTexture should work in MaterialX?

But I'm a bit surprised about the missing gold here. I'll investigate and keep you posted.

image

@pablode
Copy link
Contributor

pablode commented Mar 20, 2023

Ah right, I brought up the color3-vector3 issue in #1038 before.

I actually work around this in my renderer by patching the network, but dumped the source too early.

I believe there were talks about adding a <vector3> specialization of the UsdUVTexture node, which would solve this problem: #1070 (comment)

- push down the thin film parameters to the nodes that can use them, i.e., dielectric, conductor and generalized schlick
- removed additional inputs, that transport thin film parameters, from the public material interface by making them non-editable
- make sure thin_film will not result in undefined behavior when passing zeros as parameters (0 thickness disables thin film but 0 ior is invalid)
Due to bug in the MDL SDK this case is not handled correctly. A thickness of 0.0 should disable the layer completely. And IOR smaller than 1.0 is handled wrong.
Adding this workaround which can be removed when bumping to MDL 1.8
@krohmerNV krohmerNV force-pushed the PR-improved-MDL-thin_film-support branch from 0814f70 to 084e1ca Compare March 20, 2023 13:34
@krohmerNV
Copy link
Contributor Author

Looking into the regression and it revealed an MDL bug which will be fixed with MDL 1.8. Meanwhile, I added a workaround to detect the problematic case and handle it correctly.

@krohmerNV
Copy link
Contributor Author

about the color to float conversion, I left a comment with a suggestion in #1038

@kwokcb
Copy link
Contributor

kwokcb commented Mar 20, 2023

If we want the vec3 version of USDUVTexture how about we bring it up in the MTLX/USD channel and see what folks have to say.
IMO, it's an implementation variant and not a spec change but would need to discuss as it still produces 2 implementation signature variants for USD to consume.

@krohmerNV
Copy link
Contributor Author

@jstone-lucasfilm and @kwokcb is there something outstanding for this PR? I believe the type of the usd texture return rgb is a different topic discussed in #1038

@jstone-lucasfilm
Copy link
Member

@krohmerNV As long as the original regression noted by @pablode has been addressed, I'm not aware of any outstanding issues, and I'll plan to take a closer look at these changes soon.

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>
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.

Thanks for this improvement, @krohmerNV, and this looks good to me!

@jstone-lucasfilm jstone-lucasfilm merged commit 597bce9 into AcademySoftwareFoundation:main Apr 7, 2023
12 checks passed
Michaelredaa pushed a commit to Michaelredaa/MaterialX that referenced this pull request Oct 21, 2023
- push down the thin film parameters to the nodes that can use them, i.e., dielectric, conductor and generalized schlick
- removed additional inputs, that transport thin film parameters, from the public material interface by making them non-editable
- make sure thin_film will not result in undefined behavior when passing zeros as parameters (0 thickness disables thin_film but 0 ior is invalid)
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.

None yet

4 participants