MaterialX
MaterialX copied to clipboard
AcademySoftwareFoundation
Invalid color value and unnecessary BOM header.
Color channel values should be separated by commas (standard_surface_copper.mtlx). Unnecessary BOM file header (lib/mx_microfacet_diffuse.glsl).
mx_latlong_map_lookup() function is not required for custom multi-environment texture lookups. It's better to move this function declaration to corresponded mx_environment_fis.glsl and mx_environment_prefilter.glsl files.
Multiple environment lookups require an additional argument for all _bsdf_indirect( functions that will be passed to mx_environment_radiance() and mx_environment_irradiance() functions.
We are using a simple structure for it:
struct IBL { mat3x4 transform; uint diffuse; uint specular; };
It's possible to manually patch shaders during generation, but it would be better to have a custom argument that can be used for multiple IBL support.
Thanks for this proposal, @Tellusim, and two fixes listed in the title (color value and BOM header) look like clear improvements.
I'm less sure about the move of mx_latlong_projection and mx_latlong_map_lookup into implementation-specific files, since this has the unfortunate side effect of duplicating code that was previously centralized, making it more challenging to maintain this code over time. Perhaps we should consider this change separately, so that it doesn't hold up the integration of your original two fixes?
The problem with mx_latlong_map_lookup is the sampler2D type. Combined samplers only exist in OpenGL and Vulkan. Another good solution for us would be a macro block for mx_latlong_projection()/mx_latlong_map_lookup() functions or move all environment texture lookup functions into the separate file. It will not duplicate code. Please tell us what works best for you, and we will create a pool request.
Ah, that makes sense, and I wasn't aware of that restriction. In that case, the best approach might be to move these functions to a new environment.glsl file in the same pbrlib/genglsl/lib folder, and this new file would be included directly by dependent files such as environment_fis.glsl and environment_prefilter.glsl.
If that approach works for you, then I'd recommend putting this in a new pull request dedicated to that change, in order to make tracking more straightforward in the future.
Here is the commit with dedicated mx_environemt.glsl file with mx_latlong_projection()/mx_latlong_map_lookup() functions inside.
Might I suggest to move the USD change to another PR?
Perhaps this:
- USDUvTexture is the color variant of the spec and the spec has colorspace attributes (which still need some resolution). A vector3 variant would be useful.
- Seems the vec3 variant should only be a normal map so the scale, bias and default can be set appropriately to perform *2 -1. w/ default value { 0.5, 0.5, 1.0 }. It is not easy to just route a different set of normals in tangent space as no utility exists for this AFAIK, so 0,0,1 is "safe" as a default. There is no "tangent" space to specify input normals with (e.g. "Ntangent").
BTW @jstone-lucasfilm, I think the translation graph also needs to be fixed up so if it's a normalmap input to std surface then it needs to be removed and replaced with a USDUvTexture or an image node with additional scale + bias (which is what the graph of USDUvTexture basically is). But worth checking with Spiff if about how much to pursue USDUvTexture ?
@pablode Thanks for the explanation. This makes this material much different from StandardSurface and Gltf PBR. We will fix the UsdPrevewSurface tangent space orientation on our side. @kwokcb The modification is reverted back. But this difference is a good place for making mistakes.
@Tellusim, Yes, within the MaterialX ecosystem, the interpretation for the normal input makes it an "outlier". Will follow-up on ASWF to see what folks have to say on this and added in this issue (https://github.com/AcademySoftwareFoundation/MaterialX/issues/1060).
@kwokcb Let's leave aside questions about improving upon the UsdPreviewSurface shading model, since that's a separate specification maintained by the USD team at Pixar, and focus on the changes proposed here.
@Tellusim It may be valuable to split this pull request into independent pieces (e.g. environment map refactor, additional normalize operations for tangent frames, and syntax fixes), so that we can focus on the impact of each one separately.
For the additional normalize operations, it would help to have some visual motivation for this change, to get a sense of what artifacts it is designed to address. Usually I wouldn't expect two separate normalize calls to be required when generating a tangent frame, and more background on this specific proposal would be valuable.
@jstone-lucasfilm Here is the comparison of normalized and non-normalized tangent spaces: https://youtu.be/4OvuNRB-A_M https://youtu.be/-EfCR-NzbFI PS: yes, we will split pool requests.
That comparison looks pretty compelling, @Tellusim, and I'd be open to analyzing this proposal in a separate changelist. In your example renders, are there relatively strong normal maps on the surface? That could potentially be the property that differentiates it from the standard examples in our test suite.
@Tellusim Just a reminder that this pull request should be split into separate proposals for each related set of changes (e.g. data library fixes, GLSL file reorganization, renormalization of tangent frames).