Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quicklook ar placement #4030

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pixlhero
Copy link

@pixlhero pixlhero commented Jan 3, 2023

Fixes #3989

Also had to fix the boom_2_.glb model because the model was offset quite a lot, which messed with the rotation required for quicklook AR.

I tried a few different approaches to reset the rotation of the model (like keeping a copy of the matrix). Only rotating it back worked for me though.

Copy link
Collaborator

@elalish elalish left a comment

Choose a reason for hiding this comment

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

Thank you! Probably a good idea to test with several different off-origin models to ensure the transformations are consistent between WebXR and QuickLook.

// necessary because quicklook internally rotates model when placing vertical.
// See https://github.com/google/model-viewer/issues/3989
if (usdzOptions.ar.planeAnchoring.alignment === 'vertical') {
model.rotateX((-90 * Math.PI) / 180);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not update the GLB; it's a good test precisely because many models are not aligned to the origin. I believe you'll need to also apply the scene.target translation to make it work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It sounds like USDZ may have particular assumptions about where the origin should be relative to the placement point, so you may also need to make use of $scene.boundingBox to find the translation you need.

Choose a reason for hiding this comment

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

Instead of (-90 * Math.PI) / 180 we can use -Math.PI / 2 here.

Also we have the same auto rotation issue with alignment "any" (https://developer.apple.com/documentation/realitykit/uniform-token-preliminary-planeanchoring-alignment), so maybe doing usdzOptions.ar.planeAnchoring.alignment !== 'horizontal' would be better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you like to take over this PR or make your own version?

Choose a reason for hiding this comment

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

I didn't understand what you were suggesting in the comments above with the translation and bounding box, and also every time I tried to do threejs operation like that translation followed by rotation I always do things wrong and lose my hairs, so I probably can't do any change related to that.
But not sure if we should go this way if removing the lines https://github.com/mrdoob/three.js/blob/aaafb4ffe8830932e31aa44003376457265d126a/examples/jsm/exporters/USDZExporter.js#L209-L210 also fixes the issue.

Choose a reason for hiding this comment

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

related comment #3989 (comment)

@elalish
Copy link
Collaborator

elalish commented Feb 14, 2023

@pixlhero Ping - are you planning to come back to this?

@pixlhero
Copy link
Author

@pixlhero Ping - are you planning to come back to this?

Not in the near future unfortunately. We worked around this issue by just not placing it on the wall in our project. So until I have some spare time between projects I won't be able to work on this.

@h36ahmed
Copy link

@pixlhero - has this been fixed or still being worked on?

@ekokotov
Copy link

any news how to enable vertical placement on IOS Quicklook?

@elalish
Copy link
Collaborator

elalish commented Jan 29, 2024

If anyone would like to take over this PR, it would be great to merge it. I may get to it eventually, but not sure when.

@pixlhero
Copy link
Author

Sorry about turning silent on this PR. I think I might be able to come back to this during next month. Probably not in the next two weeks though, but I put it in my calendar.

@elalish
Copy link
Collaborator

elalish commented Jan 31, 2024

Thanks!

@pixlhero
Copy link
Author

pixlhero commented Mar 1, 2024

Working on this now 🚀

@pixlhero pixlhero reopened this Mar 1, 2024
@pixlhero pixlhero marked this pull request as draft March 1, 2024 10:21
@elalish
Copy link
Collaborator

elalish commented Mar 11, 2024

Thanks! Let me know if you have any questions.

@vincentfretin
Copy link

I tested the changes here with model-viewer 3.4.0, it works fine for my models.
But it seems we have an issue when auto-rotate attribute is used on model-viewer tag, the usdz generated has a different y rotation each time we generate the usdz, and after the 90° rotation here it gives really weird results. The bounding box seems wrong and will result of bad snapping on the wall, the shadow is ok but the model is not correctly on the wall.

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.

ar-placement ignored on quick look
5 participants