Fix ar #77

cdata · 2018-11-06T19:19:22Z

AR now works in iOS Safari 12+ and Web XR-capable Chrome Canary on Android.

Video recording of AR on Pixel 2

Video recording of AR on iPad Mini / iOS 12

Notable changes include:

A new ARRenderer has been added, that is modeled after the old ARView
Renderer incorporates ARRenderer and exposes its public API to Renderer consumers. I don't love this approach, but it seemed like the easiest factoring to start with. Open to suggestions
More resilient capability checks and more informative error messaging has been incorporated throughout ARRenderer
screenful has been replaced with a Fullscreen API polyfill approach, so that we just rely on standard APIs in the implementation
Fullscreen API and Intersection Observer polyfills have been selected for recommendation and usage in examples and tests
I had to go back to Chrome Canary 70.0.3538.2 (~September '18) on Android before I could reliably get XR hits

Test status on OSX:

Test status on iOS 12 Safari:

Fixes #55

jsantell

This looks great, thanks Chris! This is a tricky area to get right. Handful of comments about WebXR and some caveats I'm aware of.
One thing that bit me before (and I think it'll be easier to wrangle with current architecture) was changing scales between inline and AR (where inline is sized to a specific dimension, and AR is unmodified scale), and back to inline. Check out scaleModelToFitRoom() and resetModelPose() in ModelScene

jsantell · 2018-11-07T16:09:39Z

examples/ar.html

  <script src="../node_modules/resize-observer-polyfill/dist/ResizeObserver.js"></script>
+
+  <!-- Fullscreen polyfill is required to support all stable browsers: -->
+  <script src="../node_modules/fullscreen-polyfill/dist/fullscreen.polyfill.js"></script>


As a future idea, it'd be cool to test our example pages for all appropriate polyfills

jsantell · 2018-11-07T16:10:06Z

package.json

@@ -8,7 +8,7 @@
    "dist"
  ],
  "scripts": {
-    "clean": "rm -r ./lib ./dist",
+    "clean": "rm -rf ./lib ./dist",


Beat me to it 😆

jsantell · 2018-11-07T16:15:25Z

src/features/ar.js

+      console.log('Attempting to enter fullscreen and present in AR...');
+
+      try {
+        const enterFullscreen = this.requestFullscreen();


There are no browsers shipping with unprefixed/normalized fullscreen API -- this seems less like a 'bring your own polyfills to choose browser support' scenario and more like one we should normalize on our end

Although this is only used for WebXR mode. If we had a fullscreen feature for all models, I'd feel strongly about including a fullscreen API normalizer, but if it's just for WebXR, it's less critical.

My thoughts on this:

It is a classic case where a polyfill should be used (as opposed to every library normalizing it everywhere).

The polyfill is only needed for the Web XR case, which right now is a niche / experimental case

If you don't count Web XR, Chrome stable requires zero polyfills. This seems like a reasonable litmus test for whether we should ask for a polyfill or bundle some kind of normalization.

jsantell · 2018-11-07T16:17:38Z

src/test/xr-model-element-base-spec.js

+              .map((element, index) => {
+                return (index === 0) === element[$scene].isVisible;
+              })
+              .reduce(((l, r) => l && r), true);


jsantell · 2018-11-07T16:23:28Z

src/three-components/ARRenderer.js

+
+const $onWebXRFrame = Symbol('onWebXRFrame');
+const $onOutputCanvasClick = Symbol('onOutputCanvasClick');
+const $onFullscreenchange = Symbol('onFullscreenchange');


This instance isn't accessible to developers; we could get away with less "private" properties, or maybe combine several under a $state object in categories, e.g. all three.js scene components under the same $scene object or something. Just an idea, too many symbols can hurt readability IMO

Agreed 👍will tone down the symbol usage

jsantell · 2018-11-07T16:58:05Z

src/utils.js

@@ -17,6 +17,45 @@ import {Vector3} from 'three';

 export const deserializeUrl = (url) => url != null ? toFullUrl(url) : url;

+export const HAS_WEBXR_DEVICE_API = navigator.xr != null &&


I'd make a note that these can be true on Chrome release if the correct flags are set, even though there is no ARCore bridge shipping on that platform, and that these are only hints.

Will document

jsantell · 2018-11-07T16:59:01Z

test.html

+    // NOTE: This is needed in tests so that we don't have to manually trigger
+    // the IntersectionObserver polyfill in Safari. Ordinarily, it would be
+    // triggered by a resize or scroll event.
+    // @see https://github.com/w3c/IntersectionObserver/tree/master/polyfill#configuring-the-polyfill


jsantell · 2018-11-07T17:01:07Z

src/three-components/ARRenderer.js

+    this[$renderer].setSize(window.innerWidth, window.innerHeight);
+    this[$renderer].setPixelRatio(1);
+    this[$renderer].autoClear = false;
+    this[$renderer].gammaInput = true;


gammaInput can be removed (and is also being removed from three?)

jsantell · 2018-11-07T17:05:18Z

src/three-components/ARRenderer.js

+      this[$currentSession] = null;
+      session.cancelAnimationFrame(this[$rafId]);
+
+      await session.end();


After end()ing the session, we should defer to an end event on XRSession: https://immersive-web.github.io/webxr/#dom-xrsession-onend

The session can abort on its own accord and will need to clean it up when that happens

I think if the end event is called, ultimately we'll get a error while trying to end AR session for attempting to re-destroy it -- same result, but maybe would be better to have this execute the ending of the session, and an onEnd that cleans up everything else (which manually calling session.end() will call the event anyway, leading to the same path for both scenarios)

jsantell · 2018-11-07T17:06:52Z

src/three-components/Renderer.js

-    });
+    this.canvas = document.createElement('canvas');
+    this.context = this.canvas.getContext(
+        'webgl', {antialias: true, alpha: true, preserveDrawingBuffer: true});


We should be aggressive about only enabling alpha and preserveDrawingBuffer if we think there's a good chance of WebXR support, otherwise will have unneeeded overhead in 95% scenarios

Good call, will fix

jsantell · 2018-11-07T17:16:11Z

Also, were there any other issues on iOS devices that you've found? (I haven't tried in months)

cdata · 2018-11-08T06:25:16Z

@jsantell responded to the feedback. PTAL!

jsantell · 2018-11-08T17:40:48Z

src/features/ar.js

-      if (!buttonIsVisible) {
-        this[$enterARElement].style.display = 'none';
+      if (iosCandidate ||
+          (canShowButton && await renderer.supportsPresentation())) {


jsantell · 2018-11-08T17:50:10Z

src/three-components/ARRenderer.js

+    const gl = this.renderer.getContext();
+
+    await gl.setCompatibleXRDevice(device);
+    session.baseLayer = new XRWebGLLayer(session, gl, {alpha: true});


I don't think this third argument is necessary (and already configured on the context)

Okay. Just copied this configuration from ARView FWIW.

yeah there's been a handful of changes over the months inconsistently applied in demos due to platforms or discoveries across a handful of projects so I was extra cautious about all the old code I wrote months ago 😄

jsantell · 2018-11-08T17:57:30Z

src/three-components/ARRenderer.js

+      this.camera.updateMatrixWorld(true);
+      // NOTE: Clearing depth caused issues on Samsung devices
+      // @see https://github.com/googlecodelabs/ar-with-webxr/issues/8
+      this.renderer.clearDepth();


What's the reason for leaving this? This can be safely removed (as far as AR consequences go) and only prevents Samsung devices from working

Oversight, will fix

jsantell · 2018-11-08T17:57:41Z

src/three-components/Renderer.js


-import {isMobile} from '../utils.js';
+import {IS_AR_CANDIDATE} from '../constants.js';
+// import {IS_MOBILE} from '../constants.js';


nit comment

Will remove

jsantell · 2018-11-08T17:58:20Z

src/three-components/Renderer.js

+    // Only enable certain options when Web XR capabilities are detected:
+    if (IS_AR_CANDIDATE) {
+      Object.assign(
+          webGlOptions, {antialias: true, preserveDrawingBuffer: true});


antialias is redundant here

but i think you'll need alpha -- unless adding that in XRWebGLLayer solves that? I didn't think that was something you could change after instantiation, but if this works, great

Bug, actually. Should be alpha.

jsantell · 2018-11-08T18:04:24Z

src/three-components/ARRenderer.js

+      this[$currentSession] = null;
+      session.cancelAnimationFrame(this[$rafId]);
+
+      await session.end();


I think if the end event is called, ultimately we'll get a error while trying to end AR session for attempting to re-destroy it -- same result, but maybe would be better to have this execute the ending of the session, and an onEnd that cleans up everything else (which manually calling session.end() will call the event anyway, leading to the same path for both scenarios)

jsantell · 2018-11-08T18:05:11Z

src/three-components/ARRenderer.js

+    const presentedScene = this[$presentedScene];
+
+    const x = 0;
+    const y = 0;


If this is not using click coords, should make a note that it fires it out from the center of the screen

cdata force-pushed the fix-ar branch from 6b7213f to cad1886 Compare November 7, 2018 01:40

Update AR presentation for new rendering scheme

589f6a1

cdata force-pushed the fix-ar branch 3 times, most recently from c247f3a to 21958b6 Compare November 7, 2018 04:01

cdata requested a review from jsantell November 7, 2018 04:24

cdata changed the title ~~(WIP) Fix ar~~ Fix ar Nov 7, 2018

cdata assigned smalls Nov 7, 2018

Add initial test suite for AR features

7da2ec5

cdata force-pushed the fix-ar branch from 21958b6 to 7da2ec5 Compare November 7, 2018 16:11

jsantell reviewed Nov 7, 2018

View reviewed changes

Docs and tweaks based on feedback

faf2dd0

cdata mentioned this pull request Nov 8, 2018

Add modified equirect2cubemap to source tree #84

Merged

jsantell approved these changes Nov 8, 2018

View reviewed changes

Additional cleanup per feedback

81ab1e4

cdata merged commit 2f4cbd2 into master Nov 8, 2018

cdata deleted the fix-ar branch November 8, 2018 18:36

		@@ -17,6 +17,45 @@ import {Vector3} from 'three';

		export const deserializeUrl = (url) => url != null ? toFullUrl(url) : url;

		export const HAS_WEBXR_DEVICE_API = navigator.xr != null &&

Fix ar #77

Fix ar #77

Conversation

cdata commented Nov 6, 2018 • edited

jsantell 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdata Nov 7, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsantell commented Nov 7, 2018

cdata commented Nov 8, 2018

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

Choose a reason for hiding this comment

cdata commented Nov 6, 2018 •

edited

cdata Nov 7, 2018 •

edited