Skip to content

Conversation

@hybridherbst
Copy link
Contributor

@hybridherbst hybridherbst commented May 7, 2023

Related issue: #25996

Description

This PR fixes UV2 for AR QuickLook (needs to be texCoord2f[] instead of float2[]), and future proofs the exporter with the MaterialBindingAPI schema, which will be required starting with USD 23.08.
To quote,

23.08 will be the drop-dead for MaterialBindingAPI. non-compliant scenes will no longer render properly in 23.08

It also adds colorSpace = "Raw" for the normal map. QuickLook doesn't care but other viewers do.

There's more fixes needed - follow-up PR :)

This contribution is funded by Needle

{
uniform token info:id = "UsdUVTexture"
asset inputs:file = @textures/Texture_${ id }.${ isRGBA ? 'png' : 'jpg' }@
asset inputs:file = @textures/Texture_${ id }.${ isRGBA ? 'png' : 'jpg' }@ ${mapType === 'normal' ? rawTextureExtra : ''}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should something like this be required for metalness, roughness, and other maps?

Suggested change
asset inputs:file = @textures/Texture_${ id }.${ isRGBA ? 'png' : 'jpg' }@ ${mapType === 'normal' ? rawTextureExtra : ''}
asset inputs:file = @textures/Texture_${ id }.${ isRGBA ? 'png' : 'jpg' }@ ${texture.colorSpace === NoColorSpace ? rawTextureExtra : ''}

Note the import would need to be added at top of file.

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'm not entirely sure but don't think so, I believe it's required for normals because these need to be scaled with 2 * N - 1 (the scale and bias here) and end up outside 0..1 range, so it may not apply to "simply linear" textures. I'll ask!

Copy link
Contributor Author

@hybridherbst hybridherbst May 7, 2023

Choose a reason for hiding this comment

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

From the docs it looks like it should -
https://openusd.org/release/spec_usdpreviewsurface.html#:~:text=sourceColorSpace%20%2D%20token%20%2D%20auto
I asked in the USD Working Group, will report back here.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I believe it should be what Don is saying. I'll merge and amend it.
We can revert if needed when we hear from the USD Working Group.

@mrdoob mrdoob added this to the r153 milestone May 8, 2023
@mrdoob mrdoob merged commit faf95b2 into mrdoob:dev May 8, 2023
@hybridherbst hybridherbst deleted the usdzexporter-fix-primvars branch May 23, 2023 21:05
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