search

Unity-Technologies / usd-unity-sdk

Integration of Pixar's Universal Scene Description into Unity. UPDATE: This package has been superseded by our new bundle of USD packages. Please see README & link below for further details.
https://forum.unity.com/threads/new-openusd-packages-now-available.1524583/
Apache License 2.0
500 stars 77 forks source link

[USDU-347] Fix for export of Diffuse Color when a base map is set on material. #410

Closed vickycl closed 1 year ago

vickycl commented 1 year ago

Purpose of this PR

Ticket/Jira #: USDU-347

This PR means that both the diffuse texture and the diffuse colour will both be exported to USD. As the default in Unity is white, this shouldn't have an impact on the result when it's not set.

The bug repro actually describes a round trip, which is not fixed by this PR. This is because I'm not sure whether we should be importing both the texture and the colour from USD. The default color in Unity is white (1,1,1), whereas in USD the default value for diffuseColor is grey(0.18 ,0.18, 0.18). Currently the import behaviour is to try to import a connected texture (ie a base map), and if it isn't available, fallback to importing a uniform diffuse color. If we were to switch this to always import the diffuseColor even when there is a base map, I think we could end up breaking some cases as once inside Unity they would have this grey applied. But I'm not sure, this is a complicated topic with no clear documentation on how/ when the color is combined with the map or not in USD. Maybe that default is only set when there is no base map, in which case it would be fine. I think it's too high risk to change when the Importer package already seems to handle it (I tested importing my exported test with map + color and both were imported there).

Doesn't yet include HDRP but I want opinions before I go ahead with that

Testing

Functional Testing status:

Tested manually that a GameObject with a material with both a texture and a colour set appeared the same in USDView.

Performance Testing status:

Very tiny impact of now exporting two attributes.

Overall Product Risks

Complexity: low

Halo Effect: low

Additional information

Note to reviewers:

Reminder:

vickycl commented 1 year ago

The bug report in this export case was actually wrong. The diffuse color is being correctly exported, but due to a difference in how Unity and USD materials apply textures, it is exported as a texture scale, not a diffuse color (because diffuse color is ignored when a texture is connected). I've confirmed for built in, URP and HDRP that the texture scale is correctly set on export.

The import of this texture scale to a base color in the Unity material is missing, but as this is complex to fix (it is read into the texture sample in ImportConnectedTexture, but discarded as we only use the resulting Texture2D) and will be fixed better in future, I'll close this as won't fix.