Skip to content

Conversation

@hybridherbst
Copy link
Contributor

@hybridherbst hybridherbst commented May 9, 2023

This PR fixes UsdTransform2d rotation for usdview and other USD-spec-compliant viewers that support the UsdTransform2d schema.
Notably, QuickLook does things differently. There is currently a const in the code to switch between QuickLook behaviour (which is not 100% correct yet, but comes close) and proper behaviour. Unfortunately there doesn't seem to be a way to make both happy until Apple fixes their bugs.

I believe we want to default to the QuickLook behaviour (and hopefully get that fixed) since e.g. model-viewer's main target is conversion to that.

Here is a better test file - the glTF sample model is not sufficiently accurate to show the QuickLook errors:
BetterTextureTransformTests.zip

Viewer exportForQuickLook=true exportForQuickLook=false
three.js (ground truth) 20230509-104237_chrome 20230509-104237_chrome
usdview 20230509-104357_python 20230509-104438_python
QuickLook 20230509-104844_chrome 20230509-104859_chrome

This contribution is funded by Needle

@hybridherbst hybridherbst marked this pull request as ready for review May 9, 2023 08:38
@mrdoob
Copy link
Owner

mrdoob commented May 9, 2023

Very nice!

@mrdoob
Copy link
Owner

mrdoob commented May 9, 2023

I believe we want to default to the QuickLook behaviour (and hopefully get that fixed) since e.g. model-viewer's main target is conversion to that.

I feel like exportForQuickLook=false should be the default behaviour.
And add a new option in parse() so <model-viewer> can enable it for now.

@hybridherbst
Copy link
Contributor Author

cc @elalish

@mrdoob mrdoob added this to the r153 milestone May 9, 2023
@hybridherbst
Copy link
Contributor Author

new option in parse()
Like this?
image

@mrdoob
Copy link
Owner

mrdoob commented May 9, 2023

Yeah!

@elalish
Copy link
Contributor

elalish commented May 9, 2023

Looks great, thanks!

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