fixes 2d fallback #157

caseyyee · 2018-02-23T23:29:07Z

doesn't send framedata into unity.

cvan · 2018-02-23T23:35:32Z

did you test this in Chrome Canary to make sure there are no console errors/warnings? (I can check in a bit.)

caseyyee · 2018-02-24T00:48:33Z

Hmm yeah, Im getting view frustum errors in canary with win MR.

caseyyee · 2018-02-27T02:01:20Z

@cvan r?

cvan

good work on figuring out the fix 👍

thanks for spending the time to track that down.

cvan · 2018-02-27T03:26:07Z

Assets/WebGLTemplates/WebVR/webvr.js

-              orientation: Array.from(orientation),
-              position: Array.from(position)
-            });
+      if (vrDisplay.isPresenting || isPolyfilled) {


can we add a comment explaining why we are checking isPolyfilled?

cvan · 2018-02-27T03:26:29Z

Assets/WebGLTemplates/WebVR/webvr.js


-      gameInstance.SendMessage('WebVRCameraSet', 'WebVRData', JSON.stringify(vrData));
+        gameInstance.SendMessage('WebVRCameraSet', 'WebVRData', JSON.stringify(vrData));
+      }

      if (!vrDisplay.isPresenting) {


should this include || isPolyfilled like above?

cvan · 2018-02-27T03:27:06Z

Assets/WebGLTemplates/WebVR/webvr.js

@@ -299,7 +301,7 @@
      }

      // Check to see if we are polyfilled.
-      var isPolyfilled = (vrDisplay.deviceId || '').indexOf('polyfill') > 0 ||
+      isPolyfilled = (vrDisplay.deviceId || '').indexOf('polyfill') > 0 ||


this should probably be a function defined elsewhere, but I suppose this is fine until #104 is addressed

cvan · 2018-02-27T03:27:17Z

Assets/WebVR/Models/Materials.meta

@@ -0,0 +1,10 @@
+fileFormatVersion: 2


accidental commit?

cvan

first off, thank you for fixing my mistake!

want to address this JS Console error (Error: VR display is not capable of presenting) that was introduced from this change though. also, see my few other comments.

cvan · 2018-02-27T05:55:55Z

Assets/WebGLTemplates/WebVR/webvr.js

@@ -323,6 +319,14 @@
    });
  }

+  // Check to see if we are using polyfill.
+  function isPolyfilled() {


it should take a single argument (can name it display) instead of the global vrDisplay, but that can be handled in #104. regardless, thanks for making this a function! 👍

ah good call.

cvan · 2018-02-27T05:56:28Z

Assets/WebGLTemplates/WebVR/webvr.js

+        for (var i = 0; i < gamepads.length; ++i) {
+          var gamepad = gamepads[i];
+          if (gamepad && (gamepad.pose || gamepad.displayId)) {
+            if (gamepad.pose.position && gamepad.pose.orientation) {


this indentation level is getting pretty deep; can you move this up a line?

could, but this is all changing anyways. we will be handling this in another object class altogether.

yeah, that's fine

cvan · 2018-02-27T06:09:11Z

Assets/WebGLTemplates/WebVR/webvr.js

-        vrDisplay.hardwareUnitId;
-
-      if (isPolyfilled) {
+      if (isPolyfilled()) {
        showInstruction(document.querySelector('#novr'));
      } else {
        status.dataset.enabled = 'true';


I'm getting this Console error:

Error occurred getting VR display: Error: VR display is not capable of presenting at webvr.js:310 at <anonymous>

from line 310:

it's because of this value: vrDisplay.capabilities.canPresent === false; do you know why that is?

cvan · 2018-02-27T06:13:19Z

Assets/WebGLTemplates/WebVR/webvr.js

-              position: Array.from(position)
-            });
+      // Check for polyfill so that we can utilize its mouse-look controls.
+      if (vrDisplay.isPresenting || isPolyfilled()) {


so we'll need to modify this check when the FPS controls are added (#125) and webvr-polyfill (#113) are upgraded, respectively

after we get this regression fixed (my bad – sorry! and thank you!) …

we can ask @jsantell if there's any work in this area to support desktop.

and look what I found: https://github.com/jsantell/three-simple-fp-controls/blob/master/src/SimpleFPControls.js 👍

this is specific to three.js, but we have all the Unity-equivalent properties we need in WebVRCamera.cs.

The desktop (mouse, webcam) controls were removed because extra complexity, and it was unclear what the "native" desktop controls should be (something like OrbitControls were the default, only 3DOF, I prefer something FPS-y usually, like simple-fp-controls linked above). More explanation here. The example was updated to show supporting desktop in a few lines

That being said, I think something like polyfill.addDisplay(FPSControlsVRDisplay) could be implemented so that people could use shared desktop solutions (and link to it in polyfill) if they wanted. Open to hear more about supporting desktop!

FYI the simple-fp-controls were mostly lifted from Three's FPS controls, simplified a bit, and avoided pointer lock, could change key bindings, used mostly for experiments, use at your own peril 😄

@jsantell: thanks for stopping by to provide this info - all extremely helpful.

I 💯 agree re: FPS controls being preferable on desktop. I've seen first hand in person and across GitHub issues and social media much confusion with both users and developers in interacting with mouse-look controls. it just doesn't translate well, unless the scene is auto-rotating (à la Flickr's equirectangular photos) and/or with text hints + icons (à la Facebook's 360° embeds).

Given the nature of this project, I would provide all the fallbacks on the Unity side. To be honest, I don't see the need of the polyfill on the desktop since Unity developers are not creating Web content but using the Web as the distribution platform. The discussion on mobile is different.

cvan · 2018-03-01T05:04:56Z

this isn't working for me on Android phones (tested with S8 and Pixel)

@caseyyee can you try on your phone?

cvan

👍 looks good in the Vive (Windows), Daydream (Pixel)

(unrelated, but filed #181 for iOS)

delapuente · 2018-03-01T10:28:47Z

LGTM too. Good work here @caseyyee 👏

caseyyee · 2018-03-01T10:39:41Z

Thanks guys!

caseyyee requested a review from cvan February 23, 2018 23:29

cvan suggested changes Feb 27, 2018

View reviewed changes

cvan mentioned this pull request Feb 27, 2018

Add Free-Flight control behaviour to navigate the scene #125

Closed

cvan added this to the Polished community release milestone Feb 28, 2018

fixes 2d fallback

f40686a

cvan approved these changes Mar 1, 2018

View reviewed changes

caseyyee merged commit 2aef756 into MozillaReality:master Mar 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes 2d fallback #157

fixes 2d fallback #157

caseyyee commented Feb 23, 2018

cvan commented Feb 23, 2018

caseyyee commented Feb 24, 2018

caseyyee commented Feb 27, 2018

cvan left a comment

cvan Feb 27, 2018

cvan Feb 27, 2018

cvan Feb 27, 2018

cvan Feb 27, 2018

cvan left a comment

cvan Feb 27, 2018

caseyyee Feb 27, 2018

cvan Feb 27, 2018

caseyyee Feb 27, 2018

cvan Feb 28, 2018

cvan Feb 27, 2018

cvan Feb 27, 2018

cvan Feb 27, 2018

cvan Feb 27, 2018 •

edited

Loading

jsantell Feb 28, 2018

jsantell Feb 28, 2018

cvan Feb 28, 2018

delapuente Feb 28, 2018 •

edited

Loading

cvan commented Mar 1, 2018

cvan left a comment

delapuente commented Mar 1, 2018

caseyyee commented Mar 1, 2018

fixes 2d fallback #157

fixes 2d fallback #157

Conversation

caseyyee commented Feb 23, 2018

cvan commented Feb 23, 2018

caseyyee commented Feb 24, 2018

caseyyee commented Feb 27, 2018

cvan 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

cvan 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvan Feb 27, 2018 • 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

delapuente Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

cvan commented Mar 1, 2018

cvan left a comment

Choose a reason for hiding this comment

delapuente commented Mar 1, 2018

caseyyee commented Mar 1, 2018

cvan Feb 27, 2018 •

edited

Loading

delapuente Feb 28, 2018 •

edited

Loading