Autodesk / maya-usd

A common USD (Universal Scene Description) plugin for Autodesk Maya
760 stars 202 forks source link

[MAYA-114979] UsdPreviewSurface export has several correctness problems #1834

Closed spiffmon closed 2 years ago

spiffmon commented 2 years ago

Describe the bug We only recently became aware that the PreviewSurface export logic that Pixar added (but do not yet use internally, which is why we didn't notice) has three serious problems, two of which prevent materials with normal maps from rendering correctly anywhere except Apple products (because they chose to code around the problems, off-spec).

  1. The normal map textures being used generally do not contain accurate profiles or colorSpace or gamma information, and therefore Hio texture reading logic believes they are sRGB, when non-floating-point-encoded normal maps are in fact universally linear. Unless Maya has absolute control over the provenance of the textures, we therefore suggest that the exporter set inputs:sourceColorSpace = "linear" for the UsdUVTexture node that reads a normal map.
  2. 8-bit normal map textures must be mapped into the [-1, 1] range for UsdPreviewSurface, which can only be accomplished by authoring inputs:scale and inputs:bias appropriately on the normal map reader.
  3. Shader prims cannot be nested under other Shader prims. Although this does not currently cause problems in UsdImaging, it is a violation of the UsdShade Object Model, and may cause problems in future. De-nesting the texture-coord readers from the texture readers will also allow all texture readers to share the same one, producing more efficient shading.

Steps to reproduce Steps to reproduce the behavior:

  1. Export any geometry with UsdPreviewSurface shading, and a normal map
  2. In 21.11 or dev branch, run usdchecker in the result.

Expected behavior We should see no warnings from usdchecker, and the normals should look the same in usdview as they did in Maya. In future releases, we would like to turn the three usdchecker warnings into errors.

Additional context Pixar may be able to help here, and hopes to at least provide a "fixup script" that will correct these problems in most cases of existing assets, but these patterns are unfortunately getting copied by others, so it would be great to get this addressed as soon as possible, and our resources are limited.

Thanks much and apologies for the trouble.

dgovil commented 2 years ago

Hi all, I know this issue is pretty new but I was wondering if it was possible to get a sense of whom would be taking on this work, and when they thought it might be up for PR?

I ask because my team is quite eager to get these changes, so we can make our USD exports comply to Spiff's requests (as our USD files end up being what a lot of people refer to).

We are also totally open to creating the PRs for any of these too, but we wanted to make sure that we wouldn't be doubling up on work if Pixar or Autodesk were to be making the PRs instead.

Anyway, I just figured I'd ask here since we'll have to plan our asset exports and re-exports according to these changes, and plan any changes to the Maya USD exporter code accordingly too.

Cheers

kxl-adsk commented 2 years ago

We are doing a little bit of research before proposing a direction we would like to take and validating it with the community.

JGamache-autodesk commented 2 years ago

For points 1 and 2, the proper values can be set in the attribute editor of the file node that is directly connected to the normal input of the UsdPreviewSurface node:

These settings will produce the correct sourceColorSpace, scale and bias values in the exported file. We are a bit concerned about hardcoding these values on export because we risk overwriting values explicitly set by the artist, and we would suggest the following solution:

For point 3, we need to fix the code.

While on the subject of UsdShade graph correctness:

4- We also had, in the past, subtle shading errors caused by color correction on an opacity/roughness/metalness combined RGB texture (aka "ORM texture").

5- We currently produce shading graphs that connect to attributes inside a NodeGraph without declaring boundary ports and using them. This is currently compliant with the usdchecker script, but we are wondering if such graphs are valid.

Here is an example of a potentially invalid graph: BackPeekingToVarname.zip Notice how the mx_varname input of /pPlane1/Looks/GridSG/MaterialX/MayaNG_MaterialX/MayaGeomPropValue_file1 bypasses 2 NodeGraph boundaries to connect to /pPlane1/Looks/GridSG.inputs:file1:mx_varname. Same thing in the other direction, with /pPlane1/Looks/GridSG/MaterialX.outputs:surface passing through one NodeGraph boundary to connect to /pPlane1/Looks/GridSG/MaterialX/Grid.outputs:surface.

Is this the proper way to connect such a graph: NodeGraphConnectedBothWays.zip ?

dgovil commented 2 years ago

Our lookdev artist had a concern about the proposal that I'll share here:

The issue with the suggested workflow of setting scale/bias to 2,-1 won't work with OCIO configs being enabled, because only a single color_picker role is allowed.

"If you are using an OCIO configuration file that defines the color_picking role, that color space is the only one available." https://knowledge.autodesk.com/support/maya-lt/learn-explore/caas/CloudHelp/cloudhelp/2020/ENU/MayaLT-TextureBaking/files/GUID-EBC11F85-9CAB-4D33-ABF4-836AD47D5C76-htm.html

so in our workflow where color_picker role is display3 texture, values outside 0-1 aren't even allowed. and you cant turn on/off color management on a per-color-picker basis.

spiffmon commented 2 years ago

I cannot comment on the complexity @dgovil raises, but otherwise, all of your points and proposal seem spot-on, @JGamache-autodesk . Only note on gain/scale and offset/bias is to point out that for UsdUVTexture, they are float4, not float3, so need to be extended.

Great point on ORM textures - hadn't caught that, but your proposal sounds good. Would you file a USD Issue to get usdchecker to apply the same check?

Regarding NodeGraphs, yes, your "NodeGraphConnectedBothWays" example looks just right to me: Each NodeGraph fully encapsulates its contents, so there should be no connectins originiating "inside" that poke outside, and no onnections outside targeting interior nodes of the NodeGraph. WOuld you file a second Issue for usdchecker to start enforcing UsdShadeConnectableAPI encapsulation as well as the prim-level encapsulation checks it's currently performing?

Thanks so much! spiff

JGamache-autodesk commented 2 years ago

Hi @dgovil, I think a Maya attribute preset could bypass the color_picker role and wrap these values in an easy to use package. Can you temporarily deactivate OCIO on your setup, create one preset for a "file" node as "USD normal map" and edit the MEL file so it contains only the following:

startAttrPreset( "file" );
blendAttrStr "colorSpace" "Raw"; 
blendAttr "colorGainR" 2; 
blendAttr "colorGainG" 2; 
blendAttr "colorGainB" 2; 
blendAttr "colorOffsetR" -1; 
blendAttr "colorOffsetG" -1; 
blendAttr "colorOffsetB" -1; 
blendAttr "alphaGain" 1; 
blendAttr "alphaOffset" 0; 
endAttrPreset();

Then see if that presets applies as expected with OCIO enabled and locked.

JGamache-autodesk commented 2 years ago

This has been logged internally as MAYA-114979

dgovil commented 2 years ago

@JGamache-autodesk Thanks for the suggestion. I'll ask our TD and color people to try out the attribute preset after we're back from the Thanksgiving break next week. 🦃

dgovil commented 2 years ago

@JGamache-autodesk so we tried your suggestion with the presets, and that did work. It does perhaps feel a bit fragile (in that I don't know if future Maya versions would honor this, and will require a little tooling unless maya-usd itself handles things), but definitely does work as you said right now.

dgovil commented 2 years ago

Oh one other concern that has come up here, the colorOffset cannot be set below 0.

setAttr file1.colorOffset -1 -1 -1;
getAttr file1.colorOffset;
// Error: line 1: setAttr: Cannot set the attribute 'file1.colorOffsetR' below its minimum value of 0. // 
applyPresetToNode "file1" "" "" "normalmap" 1;
getAttr file1.colorOffset;
// Result: 0 0 0 //