fix issues with `{getFrameData,submitFrame} must be called within a VRDisplay.requestAnimationFrame callback.` (fixes issue #65); fix JS errors `Screen position out of view frustum` in Chrome Canary (fixes issue #64) #132

cvan · 2018-02-13T12:45:05Z

Fix #64
Fix #65

seems to work well. feedback to approach, including possible edge cases I've missed, welcome.
tested with HTC VIVE on Windows 10 in both Chrome Canary and Firefox Nightly.

caseyyee

@cvan Tested with FF and Chrome Canary, looks to be working well. The only thing that broke is that we're starting out in the ground.

Looks like were not hitting the stageParameters bits:
https://github.com/mozilla/unity-webvr-export/blob/master/Assets/WebGLTemplates/WebVR/webvr.js#L123-L124

cvan · 2018-02-14T06:58:42Z

@cvan Tested with FF and Chrome Canary, looks to be working well. The only thing that broke is that we're starting out in the ground.

thanks for testing. any ideas how to fix? did you check + build on master to compare?

cvan · 2018-02-14T06:59:47Z

Build/index.html

@@ -3,8 +3,8 @@
  <head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
-    <title>Unity-WebVR-Export</title>
-    <meta name="description" content="Unity-WebVR-Export">
+    <title> | Unity-WebVR-Export</title>


I'm surprised this hasn't been updated on master when y'all have built the project

Me too. I don't understand why.

have issue #116 filed. (I just upgraded to the .3 version of Unity; mayve that'll change things. I'll let you know.) thanks!

delapuente · 2018-02-14T08:02:16Z

Assets/WebGLTemplates/WebVR/webvr.js

-  function onUnityLoaded() {
+
+  function onUnityLoaded () {
+    // Get and hide Unity's `<canvas>` instance.


I don't get the comment. What you are hiding is the loader, not the <canvas>.

I think this got moved in some merge/stash-patch applied. will remove.

delapuente · 2018-02-14T08:05:00Z

Assets/WebGLTemplates/WebVR/webvr.js

-      console.log('vr exit present success!')
+  function onExitPresent () {
+    if (!vrDisplay && !vrDisplay.isPresenting) {
+      throw new Error('No VR display to exit VR mode');


I would use a console.warn() here and fail silently but it's your call.

for the VRManager, I've got in a local branch (initially started for this, and then for #104, and so on), I have turned these functions into methods, and this places nicely with it.

I'm cool with removing that for now. thanks for mentioning the inconsistency; it was bothering me too

I saw you replaced a different throw with a warning but what about this exception?

oh, I missed this one. got distracted with the launch logistics today. will resivit this on Tuesday. (I’m PTO tomorrow, and Monday is a US holiday.)

No problem. I'll do.

delapuente · 2018-02-14T08:40:32Z

Assets/WebGLTemplates/WebVR/webvr.js

+      // `vrDisplay.submitFrame(…)`. So for the first frame that this is called, we will
+      // abort early and request a new frame from the VR display instead.
+      if (vrDisplay.isPresenting && !submitNextFrame) {
+        submitNextFrame = false;


If the conditional is true, then submitNextFrame is already false. What's the point of this assignment?

doy, that's what I get for working late. I should be checking only !submitNextFrame. thanks.

and that order does matter; I don't know if the comment above makes sense. anything I clarify?

Something like the flag, if true, ensures that we are inside a vrDisplay.requestAnimationFrame. Anyway, it was clear enough for me. Perhaps we can think of a better fix when addressing #104

delapuente · 2018-02-14T08:43:46Z

Assets/WebGLTemplates/WebVR/webvr.js

-    window.requestAnimationFrame(onAnimate);
+  function onAnimate () {
+    if (!vrDisplay || !vrDisplay.isPresenting) {
+      windowRaf(onAnimate);


I think this is a kind of active polling, so you repeat the check on vrDisplay. I would prefer this to be event-driven but if that's not possible, can you clarify why you are rescheduling onAnimate in a comment?

I could actually remove these lines, good call. they were leftovers.

If they are leftovers, please, do it.

I haven’t step-debugged the rAF calls. but I think if I add a debugger call to the monkeypatched window.requestAnimationFrame function, I can see how often and when Unity’ s WebGL Engine makes the calls. I want to minimise the rAF calls as much as possible. in my testing today, I removed this block and didn’t notice any degradation.

Non-blocking. Let's move to #104

cvan · 2018-02-14T10:47:57Z

Build/manifest.webmanifest

@@ -22,7 +22,7 @@
  ],
  "screenshots": [
    {
-      "src": "https://raw.githubusercontent.com/caseyyee/unity-webvr-export/master/img/preview.gif",
+      "src": "https://raw.githubusercontent.com/mozilla/unity-webvr-export/master/img/preview.gif",


@delapuente: how come these changes weren't changed when you did the builds manually yourself? do we have something wonky set up here?

I'd be curious to get on Vidyo together, the three of us, and compare our Unity flows. I'm sure there are some interesting workflows and edge cases we're handling differently.

Happy to do it. Send an invitation if you want.

cvan · 2018-02-14T10:48:48Z

I pulled, rebased, but I'm still getting these conflicts according to GitHub. hmm… I'll have to look in the morning (my morning).

cvan · 2018-02-14T10:50:04Z

@delapuente @caseyyee: ignoring the Build/ mess, I just want to make sure that these changes (really just to Assets/WebGLTemplates/WebVR/webvr.js and Build/webvr.js) run smoothly for you and with no discernible regressions.

thanks for reviewing and testing, you both 👍

delapuente · 2018-02-14T10:51:09Z

No problem Chris, if you can not solve the issues, I will rebase it. I'm having problems with my VIVE right now and cannot wake the base stations up so I can not test it. Go to sleep. ZzzZzz

delapuente · 2018-02-14T16:26:09Z

This is inevitable:

@cvan Tested with FF and Chrome Canary, looks to be working well. The only thing that broke is that we're starting out in the ground.

Now, when not presenting, we are not updating the camera matrices (which is mandatory because getFrameData must be called inside vrDisplay.requestAnimationFrame too). For me, it makes perfect sense since we are returning the control to Unity's camera system. If this is the desired behaviour, we must also reset the effect of setting worldToCameraMatrix by calling Camera#ResetWorldToCameraMatrix on all the cameras of the WebVR rigging. It can be done when handling the End message sent from JavaScript to Unity.

delapuente · 2018-02-14T16:35:48Z

@cvan working as expected on my side. Solve the issues and grant me access to your repo if you need help with the rebasing. Nice work. These are all valuable steps towards #104

caseyyee · 2018-02-15T03:19:21Z

@delapuente with the polyfill, it should still update (this should be the old version still?)

If we replaced it with the newer one, then yeah. It won't hit that code path.

delapuente · 2018-02-15T08:17:41Z

@delapuente with the polyfill, it should still update (this should be the old version still?)

Even with the polyfill, the execution will never hit that code path (notice the test condition and the early return)

delapuente · 2018-02-15T08:43:35Z

@cvan, I've discarded your changes on Build/ to ease the rebase. I'll regenerate them and push to master once this is merged. Please, address the changes and remember to not commit the changes on Build/. Thank you so much, let's fix two bugs for the price of one!

…RDisplay.requestAnimationFrame callback.` (fixes issue #65); fix JS errors `Screen position out of view frustum` in Chrome Canary (fixes issue #64)

cvan added the bug label Feb 13, 2018

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

cvan self-assigned this Feb 13, 2018

cvan requested review from delapuente and caseyyee February 13, 2018 12:45

cvan mentioned this pull request Feb 13, 2018

requestAnimationFrame recursion on mobile #127

Closed

caseyyee reviewed Feb 14, 2018

View reviewed changes

cvan commented Feb 14, 2018

View reviewed changes

delapuente reviewed Feb 14, 2018

View reviewed changes

cvan commented Feb 14, 2018

View reviewed changes

cvan mentioned this pull request Feb 14, 2018

JS Screen position out of view frustum errors in Chrome Canary #64

Closed

delapuente mentioned this pull request Feb 15, 2018

Page load blocked on JS error TypeError: vrDisplay is null (thrown intermittently); causes Gecko to crash! #99

Closed

cvan and others added 3 commits February 15, 2018 18:11

fix issues with `{getFrameData,submitFrame} must be called within a V…

64a32fe

…RDisplay.requestAnimationFrame callback.` (fixes issue #65); fix JS errors `Screen position out of view frustum` in Chrome Canary (fixes issue #64)

address feedback

f002180

More nits

a403b60

delapuente merged commit 7faf2e8 into MozillaReality:master Feb 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix issues with `{getFrameData,submitFrame} must be called within a VRDisplay.requestAnimationFrame callback.` (fixes issue #65); fix JS errors `Screen position out of view frustum` in Chrome Canary (fixes issue #64) #132

fix issues with `{getFrameData,submitFrame} must be called within a VRDisplay.requestAnimationFrame callback.` (fixes issue #65); fix JS errors `Screen position out of view frustum` in Chrome Canary (fixes issue #64) #132

cvan commented Feb 13, 2018 •

edited by delapuente

Loading

caseyyee left a comment •

edited

Loading

cvan commented Feb 14, 2018

cvan Feb 14, 2018

delapuente Feb 14, 2018

cvan Feb 14, 2018

delapuente Feb 14, 2018

cvan Feb 14, 2018

delapuente Feb 14, 2018

cvan Feb 14, 2018

delapuente Feb 16, 2018

cvan Feb 16, 2018

delapuente Feb 16, 2018

delapuente Feb 14, 2018

cvan Feb 14, 2018

delapuente Feb 15, 2018

delapuente Feb 14, 2018

cvan Feb 14, 2018

delapuente Feb 16, 2018

cvan Feb 16, 2018

delapuente Feb 16, 2018

cvan Feb 14, 2018

delapuente Feb 14, 2018

cvan commented Feb 14, 2018

cvan commented Feb 14, 2018

delapuente commented Feb 14, 2018 •

edited

Loading

delapuente commented Feb 14, 2018 •

edited

Loading

delapuente commented Feb 14, 2018

caseyyee commented Feb 15, 2018

delapuente commented Feb 15, 2018

delapuente commented Feb 15, 2018 •

edited

Loading

fix issues with {getFrameData,submitFrame} must be called within a VRDisplay.requestAnimationFrame callback. (fixes issue #65); fix JS errors Screen position out of view frustum in Chrome Canary (fixes issue #64) #132

fix issues with {getFrameData,submitFrame} must be called within a VRDisplay.requestAnimationFrame callback. (fixes issue #65); fix JS errors Screen position out of view frustum in Chrome Canary (fixes issue #64) #132

Conversation

cvan commented Feb 13, 2018 • edited by delapuente Loading

caseyyee left a comment • edited Loading

Choose a reason for hiding this comment

cvan commented Feb 14, 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

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 commented Feb 14, 2018

cvan commented Feb 14, 2018

delapuente commented Feb 14, 2018 • edited Loading

delapuente commented Feb 14, 2018 • edited Loading

delapuente commented Feb 14, 2018

caseyyee commented Feb 15, 2018

delapuente commented Feb 15, 2018

delapuente commented Feb 15, 2018 • edited Loading

fix issues with `{getFrameData,submitFrame} must be called within a VRDisplay.requestAnimationFrame callback.` (fixes issue #65); fix JS errors `Screen position out of view frustum` in Chrome Canary (fixes issue #64) #132

fix issues with `{getFrameData,submitFrame} must be called within a VRDisplay.requestAnimationFrame callback.` (fixes issue #65); fix JS errors `Screen position out of view frustum` in Chrome Canary (fixes issue #64) #132

cvan commented Feb 13, 2018 •

edited by delapuente

Loading

caseyyee left a comment •

edited

Loading

delapuente commented Feb 14, 2018 •

edited

Loading

delapuente commented Feb 14, 2018 •

edited

Loading

delapuente commented Feb 15, 2018 •

edited

Loading