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

MDL 1.9 updates #2102

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

Conversation

krohmerNV
Copy link
Contributor

@krohmerNV krohmerNV commented Nov 1, 2024

add new version to GenMDL and remove uniform restriction from material IOR

Minor updates:

  • improve MDL printing by adding named parameters
  • handle 1-element mixes (basically scale) above layers
  • improve blur and hight to normal to return something meaningful (before totally broken)
  • improve non-material outputs that can be rendered, e.g. float3x3 and float4x4
  • preparations for MDL 1.10
  • remove shader parameters from the public MDL material interface. backsurfaceshader and displacementshader showed up with default values only after recent upstream

…tion from material IOR

Minor updates:
- improve MDL printing by adding named parameters
- handle 1-element mixes (basically scale) above layers
- improve blur and hight to normal to return something meaningful (before totally broken)
- improve non-material outputs that can be rendered, e.g. float3x3 and float4x4
@krohmerNV krohmerNV changed the title WIP: MDL 1.9 updates MDL 1.9 updates Nov 11, 2024
Copy link
Contributor

@ld-kerley ld-kerley left a comment

Choose a reason for hiding this comment

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

Overall looks good to me - I left a couple of small notes below - but nothing major.

I don't have a way to test the MDL backend at the moment - so relying on the unit tests and you to validate its generating the correct images etc, but code structure etc looks good to me.

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.

This looks like a great set of improvements, thanks @krohmerNV, and I had just a few recommendations for edits.

@@ -0,0 +1,372 @@
/*
* Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Would it be acceptable to use the latest MaterialX project copyright for this new file? Here's the text of that copyright notice for reference:

//
// Copyright Contributors to the MaterialX Project
// SPDX-License-Identifier: Apache-2.0
//

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to check. I do think so but I want to make sure.

source/MaterialXGenMdl/mdl/materialx/pbrlib_1_9.mdl Outdated Show resolved Hide resolved
source/MaterialXGenMdl/mdl/materialx/pbrlib_1_9.mdl Outdated Show resolved Hide resolved
// this produces NaN
float nx = S[0] - S[2] + (2.0*S[3]) - (2.0*S[5]) + S[6] - S[8];
float ny = S[0] + (2.0*S[1]) + S[2] - S[6] - (2.0*S[7]) - S[8];
// float nz = scale * ::math::sqrt(1.0 - nx*nx - ny*ny);
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to remove this unused original logic, since GitHub itself preserves the history of this file?

Copy link
Contributor

Choose a reason for hiding this comment

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

This code is ends up being part of the data library - right? if so then it will be installed in locations that aren't attached to any git repo - so perhaps this is a case where keeping comments around that help keep the code more readable is a reasonable step?

git history is not always the easiest place to find documentation.

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