AR autogenerate USDZ #2374

kolodi · 2021-05-11T14:00:44Z

Reference Issue #2373

Adding USDZ generation on the fly as an alternative to static ios-src (maintain both ways)

elalish · 2021-05-11T16:07:40Z

Thanks, I notice you reference my own PR on this topic; can you give a bit of detail of how yours is different or what it adds?

kolodi · 2021-05-11T16:22:09Z

Thanks, I notice you reference my own PR on this topic; can you give a bit of detail of how yours is different or what it adds?

Hello, yes.
I was preparing my PR and then realized that you have already the similar update ready.

I checked your implementation, the key differences in mine are:

using of a custom property to enable/disable autogeneration on the fly
releasing the generated URL after src is being changed otherwise continue using a previously generated one
disabling the AR button (in case user clicks many times on it during the export phase)
showing loading bar (for older devices there can be a noticeable time for export or the model can be heavy)
using a slightly different way for Blob creation
disabling the shadow in the scene before exporting, similarly to how it is during glTF export in scene graph.

kolodi · 2021-05-11T16:38:19Z

packages/model-viewer/src/features/ar.ts

@@ -327,7 +331,12 @@ configuration or device capabilities');
     * Takes a URL to a USDZ file and sets the appropriate fields so that Safari
     * iOS can intent to their AR Quick Look.
     */
-    [$openIOSARQuickLook]() {
+    async[$openIOSARQuickLook]() {
+      if (this.arAutogenerateUsdz && !this.iosSrc) {


Generate USDZ on the fly only if autogenerate property is set and there is not ios-src.
Also disable temporally the AR button so the user cannot press it multiple times during the export.

Take a look at the documentation updates in my PR and let me know what you think of my alternative solution to this.

Yes, the export of USDZ has a tons of limitations, I'm very aware of them and I had a really hard time creating dozens of workarounds about them. That was actually one of the main reasons why I wanted to leave a possibility to disable the on the fly generation, sometimes you still want your static prepared ahead USDZ file or set it manually from a pool of static variants or something like this, in any case we need a mechanism to turn it on/off

elalish · 2021-05-11T16:40:21Z

Fantastic! 2-6 are great; I simply hadn't gotten around to those yet. I think I prefer my method of keeping this from being a breaking change without introducing a new attribute. Do you agree? If so, I think merging our PRs would be ideal. Would you like to pull mine into yours and I can review?

packages/model-viewer/src/features/ar.ts

kolodi · 2021-05-11T16:43:20Z

packages/model-viewer/src/features/ar.ts

@@ -72,6 +73,8 @@ export const ARMixin = <T extends Constructor<ModelViewerElementBase>>(
    ModelViewerElement: T): Constructor<ARInterface>&T => {
  class ARModelViewerElement extends ModelViewerElement {
    @property({type: Boolean, attribute: 'ar'}) ar: boolean = false;
+    @property({type: Boolean, attribute: 'ar-autogenerate-usdz'})


Set this property to true if you want to enable on the fly USDZ generation

Did you see my comment about doing this without introducing a new attribute? If you look at my PR, I changed the logic a bit so that the presence of ios-src automatically enables Quick Look, while explicitly adding the quick-look ar-mode opts in to USDZ generation. I think I prefer that approach; what do you think? Would you mind merging in that part of my PR here?

Ok, quick-look presence can be enough. I also added blob clean up before creating a new one (see the commit)

kolodi · 2021-05-11T16:45:17Z

packages/model-viewer/src/features/ar.ts

@@ -340,9 +349,50 @@ configuration or device capabilities');
      const img = document.createElement('img');
      anchor.appendChild(img);
      anchor.setAttribute('href', modelUrl.toString());
+      if (this.arAutogenerateUsdz) {
+        anchor.setAttribute('download', 'model.usdz');


this tricks Safari to open the Quick Look. the file name has been chosen the same as it is in USDZExporter.js, although it could be anything.

In my testing it wasn't necessary on my iPhone SE. But it also probably doesn't hurt anything.

packages/model-viewer/src/features/ar.ts

elalish · 2021-05-11T16:43:18Z

packages/model-viewer/src/features/ar.ts

@@ -340,9 +349,50 @@ configuration or device capabilities');
      const img = document.createElement('img');
      anchor.appendChild(img);
      anchor.setAttribute('href', modelUrl.toString());
+      if (this.arAutogenerateUsdz) {
+        anchor.setAttribute('download', 'model.usdz');


What does this do? Is it necessary?

When I prepare the Blob, I use simply "application/octet-stream"for the type. So Safari need a "hint" to treat the link as a valid source to open Quick Look, that's why there is a download attribute. As a side effect you can also test the export in desktop browser as it will download the usdz file for you.

Ah, interesting. I think it worked for me because I used a more specific mime type; you can see in my PR. The button still doesn't appear on desktop Safari though, right?

Yes, you have to manually override IS_AR_QUICKLOOK_CANDIDATE constant :)

elalish · 2021-05-11T16:44:47Z

packages/model-viewer/src/features/ar.ts

+        type: 'application/octet-stream',
+      });
+
+      this.iosSrc = URL.createObjectURL(blob);


I'm not sure I like this internal detail being publicly visible; maybe better to create a new private symbol instead.

You mean showing the loading bar? I was initially thinking about showing some custom poster but then decided to just show some simulated (0.2) progress. On latest iPhone it is quite immediate to export a simple scene, but on older devices or when the scene is quite complex, it might require more time.

No, the loading bar is great. It's just that this.iosSrc is part of our public API. If someone replaced it without revoking it, bad things would happen. I just mean we should store our blob as a private member instead.

Ah, ok. Infact, ios-src continue to store only the url string not the actual blob. I need to check if when we call URL.revoke... if it actually dispose the blob and to not create a memory leak here.

Or the Blob is garbage collected after URL.revokeObjectURL is called?

I think it is probably still a good thing to store blob url in separate member. Let me make those changes.

packages/model-viewer/src/features/ar.ts

kolodi

Added more details using comments in code

mrdoob · 2021-05-11T18:27:31Z

showing loading bar (for older devices there can be a noticeable time for export or the model can be heavy)

You may want to try with the dev version of USDZExporter in Three.js. I think it should run faster now.

elalish · 2021-05-11T18:29:27Z

@mrdoob Honestly it already feels faster than downloading the separate USDZ, even on my old SE. I'd rather stick to released versions :). We'll update soon enough anyway.

elalish · 2021-05-11T18:32:21Z

packages/model-viewer/src/features/ar.ts

-      const modelUrl = new URL(this.iosSrc!, self.location.toString());
+      const modelUrl = this.arAutogenerateUsdz ?
+          new URL(this[$generatedIosUrl]!, self.location.toString()) :
+          new URL(this.iosSrc!, self.location.toString());


You can put the ternary op inside of the new URL() to simplify this a bit.

elalish · 2021-05-11T18:35:56Z

packages/model-viewer/src/features/ar.ts

@@ -72,6 +73,8 @@ export const ARMixin = <T extends Constructor<ModelViewerElementBase>>(
    ModelViewerElement: T): Constructor<ARInterface>&T => {
  class ARModelViewerElement extends ModelViewerElement {
    @property({type: Boolean, attribute: 'ar'}) ar: boolean = false;
+    @property({type: Boolean, attribute: 'ar-autogenerate-usdz'})


Did you see my comment about doing this without introducing a new attribute? If you look at my PR, I changed the logic a bit so that the presence of ios-src automatically enables Quick Look, while explicitly adding the quick-look ar-mode opts in to USDZ generation. I think I prefer that approach; what do you think? Would you mind merging in that part of my PR here?

elalish · 2021-05-11T18:51:41Z

packages/model-viewer/src/features/ar.ts

-    [$openIOSARQuickLook]() {
-      const modelUrl = new URL(this.iosSrc!, self.location.toString());
+    async[$openIOSARQuickLook]() {
+      if (this.arAutogenerateUsdz && !this[$generatedIosUrl]) {


Ah, I just noticed a problematic aspect of your feature 2: if the file changes due to the scene-graph API (switched variant, swapped texture/color etc) and you jump into Quick Look again, you won't see the changes due to caching the USDZ. We could either skip the cache entirely (I don't think the perf change is a huge deal, especially since going into Quick Look twice with the same model is probably pretty rare), or else we'd have to add some kind of isDirty for the scene-graph functions that would tell us to regenerate the USDZ. I think simpler is better, especially for the first version.

From the scene-graph I can see that this is used when changing something:

[$needsRender]() { this[$scene].isDirty = true; }

We can override this method in AR mixin to revoke our URL

Yeah, but the chance of breaking things is high. Again, I don't think the perf gain here is enough to justify the complexity. Let's just skip the USDZ caching and call it a win on memory footprint. 😁

alright, for now, let's make those bionic chips do their job :). I really hope they will finally let us use WebXR on iOS directly so we can avoid using USDZ for good :)

Here here! Someday...

elalish

This is looking just about ready to merge, thanks for the quick turn around! I'll remove the code changes from my PR and merge it afterwards just for the docs/examples updates. Can you please pull master and resolve whatever small conflict has arisen?

elalish · 2021-05-11T21:14:30Z

packages/model-viewer/src/features/ar.ts

@@ -53,6 +54,7 @@ const $arMode = Symbol('arMode');
 const $arModes = Symbol('arModes');
 const $arAnchor = Symbol('arAnchor');
 const $preload = Symbol('preload');
+const $generatedIosUrl = Symbol('generatedIosUrl');


I think this is vestigial now.

elalish · 2021-05-11T21:15:28Z

packages/model-viewer/src/features/ar.ts

-          } else if (
-              value === 'quick-look' && !!this.iosSrc &&
-              IS_AR_QUICKLOOK_CANDIDATE) {
+          } else if (value === 'quick-look' && IS_AR_QUICKLOOK_CANDIDATE) {


There's an extra logic block we'll need below this from my PR. You can either pull it in, or I can add it in mine with the docs update.

elalish · 2021-05-11T21:18:51Z

packages/model-viewer/src/features/ar.ts

      if (this.arScale === 'fixed') {
        if (modelUrl.hash) {
          modelUrl.hash += '&';
        }
        modelUrl.hash += 'allowsContentScaling=0';
      }
+


I read somewhere that Safari doesn't like hashes on blob URLs. Have you tried arScale="fixed" to see if it actually works in QuickLook? I'm not very concerned if it doesn't, so long as it doesn't crash, but I'd be pretty stoked if I'm wrong.

kolodi · 2021-05-11T21:35:36Z

This is looking just about ready to merge, thanks for the quick turn around! I'll remove the code changes from my PR and merge it afterwards just for the docs/examples updates. Can you please pull master and resolve whatever small conflict has arisen?

Ok, I'll try now

kolodi · 2021-05-11T21:52:08Z

This is looking just about ready to merge, thanks for the quick turn around! I'll remove the code changes from my PR and merge it afterwards just for the docs/examples updates. Can you please pull master and resolve whatever small conflict has arisen?

Ok, I'll try now

I have merged your usdz branch and (hopefully) resolved all conflicts correctly :)

elalish

Thank you! This is a really important feature.

hybridherbst · 2021-05-17T23:20:29Z

A question @elalish since this has been merged already: how would one now set the URL parameters on the USDZ file for configuring checkoutTitle etc.? There's similar URL parameters for Quick Look as for Scene Viewer, and we're using those a lot.

One idea here would be: could the ios-src still be set but contain a "magic" string for "insert autogenerated USDZ here"? This way the URL parameters would "just work".

elalish · 2021-05-17T23:24:32Z

Actually, could you give that a test for me first? I read that Safari doesn't handle query params on blob URLs, so I figured this wasn't going to work. If it does, I'll be happy to find a way to enable it.

kolodi · 2021-05-18T14:01:25Z

how would one now set the URL parameters on the USDZ file for configuring checkoutTitle etc.? There's similar URL parameters for Quick Look as for Scene Viewer, and we're using those a lot.

You mean parameter after a hashtag (docs), hmm, I never used them. Do they really work?

One can set ios-src with a string starting with "#", then we can check that this is a "hashtag part" only and not a valid URL therefore generate the USDZ on the fly and add the "hashtag part" to the link.

There is also a problem with the link being destroyed immediately after the click event, so your listener for "message" would not work... but I think we can delay destroying the link to the point when user turns back to the web page.

milesgreen · 2021-05-19T17:07:12Z

Actually, could you give that a test for me first? I read that Safari doesn't handle query params on blob URLs, so I figured this wasn't going to work. If it does, I'll be happy to find a way to enable it.

Ah. Maybe this is the cause of #2405 ?

elalish and others added 3 commits May 10, 2021 16:23

automatic ios-src generation

7d90750

added docs

78a9347

AR autogenerate USDZ

b3142dd

kolodi commented May 11, 2021

View reviewed changes

packages/model-viewer/src/features/ar.ts Outdated Show resolved Hide resolved

kolodi commented May 11, 2021

View reviewed changes

packages/model-viewer/src/features/ar.ts Show resolved Hide resolved

kolodi commented May 11, 2021

View reviewed changes

elalish reviewed May 11, 2021

View reviewed changes

kolodi commented May 11, 2021

View reviewed changes

separate field to store generated USDZ url

d58c6f7

elalish reviewed May 11, 2021

View reviewed changes

kolodi added 3 commits May 11, 2021 21:37

remove caching for now

785e876

Removing property. Clean up previous blob url.

899f90d

genrateUsdz boolean bug fix

42d77de

elalish requested changes May 11, 2021

View reviewed changes

Merge remote-tracking branch 'google/usdz' into USDZexport

321fc17

Merge branch 'master' into USDZexport

a506b6d

elalish approved these changes May 11, 2021

View reviewed changes

elalish merged commit 2baee76 into google:master May 11, 2021

elalish mentioned this pull request May 11, 2021

On the fly USDZ generation #2373

Closed

mrdoob mentioned this pull request May 13, 2021

GLTFLoader: Support KHR_materials_transmission physically-based transparency mrdoob/three.js#21000

Closed

elalish mentioned this pull request May 14, 2021

simplified USDZ and added docs #2380

Merged

renovate bot mentioned this pull request Jun 7, 2021

Update dependency @google/model-viewer to v1.7.2 EtherealEngine/etherealengine#1986

Merged

1 task

AR autogenerate USDZ #2374

AR autogenerate USDZ #2374

Conversation

kolodi commented May 11, 2021

Reference Issue #2373

elalish commented May 11, 2021

kolodi commented May 11, 2021

kolodi May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolodi May 11, 2021 • edited Loading

Choose a reason for hiding this comment

elalish commented May 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolodi May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolodi left a comment

Choose a reason for hiding this comment

mrdoob commented May 11, 2021

elalish commented May 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolodi May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elalish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolodi commented May 11, 2021

kolodi commented May 11, 2021

elalish left a comment

Choose a reason for hiding this comment

hybridherbst commented May 17, 2021

elalish commented May 17, 2021

kolodi commented May 18, 2021

milesgreen commented May 19, 2021

kolodi May 11, 2021 •

edited

Loading

kolodi May 11, 2021 •

edited

Loading

kolodi May 11, 2021 •

edited

Loading

kolodi May 11, 2021 •

edited

Loading