handle vrdisplaypresentchange when triggering enter/exit VR outside aframe (fixes #2614) #2751

ngokevin · 2017-06-09T18:27:31Z

Description:

A user agent MUST dispatch this event type to indicate that a VRDisplay has begun or ended VR presentation. This event should not fire on subsequent calls to requestPresent() after the VRDisplay has already begun VR presentation.

Previously, we only handled exitVR on pressing the escape key. Thanks to @DigiTec for pointing in the right direction.

Changes proposed:

handle onvrdisplaypresentchange
tested
unit tests

cvan

thanks for tackling this. see my comments about the correct event name, filtering by the display property in the event emitted, and handling setting A-Frame vr-mode when vrdisplaypresentchange on entering VR

cvan · 2017-06-13T03:28:06Z

src/core/scene/a-scene.js

+
+        // Handler to exit VR (e.g., Oculus Browser back button).
+        this.onVRPresentChange = bind(this.onVRPresentChange, this);
+        window.addEventListener('onvrdisplaypresentchange', this.onVRPresentChange);


need to drop the on; the event is called vrdisplaypresentchange

cvan · 2017-06-13T03:39:11Z

src/core/scene/a-scene.js

+     */
+    onVRPresentChange: {
+      value: function () {
+        if (!this.is('vr-mode')) { return; }


can we have vr-mode set if vrdisplaypresentchange fires upon entering VR? if an A-Frame site wants to enter VR mode using the WebVR API directly, currently devs have to explicitly call $('a-scene').enterVR() (when the user-gesture requirement is pref'd off, or in full-VR-only browsers such as the Oculus Browser).

currently, calling navigator.getVRDisplays().then(displays => displays[0].requestPresent([{source: $('canvas[data-aframe-canvas]')}]) doesn't properly set vr-mode, etc., but it should.

this is actually necessary for properly entering VR mode upon page navigation (without the auto-enter-vr component, which forces entering VR mode, not responding to the user's already being in VR mode).

there are cases where sites would want to handle entering VR mode by using the WebVR API directly, without needing to special case for A-Frame scenes vs. three.js scenes vs. others. examples: the WebVR Agent, a JS user script/bookmarklet, a WebExtension/Chrome browser extension, etc.

this will require a couple changes to the enterVR method. specifically, instead of calling this.effect.requestPresent(…), we'll want to just check if if (this.checkHeadsetConnected() || this.isMobile) and call enterVRSuccess().

cvan · 2017-06-13T03:47:29Z

src/core/scene/a-scene.js

+     * on Oculus Browser.
+     */
+    onVRPresentChange: {
+      value: function () {


this should check if (evt.display && evt.display.getLayers && evt.display.getLayers().indexOf(self.canvas) === -1) { return; } so that VR mode isn't toggled for the wrong presentation event (examples: multiple A-Frame scenes on a page, or if there are two VR displays connected but the active A-Frame scene isn't the content being presented)

ngokevin · 2017-06-13T21:53:00Z

Thanks, updated. Didn't do the canvas checking here, maybe another PR, but enter/exit works on Oculus Browser now.

ngokevin · 2017-06-13T22:01:06Z

One sec...fixing last second introduced issue

cvan

great work one the changes. I've just a few more comments; lmk.

cvan · 2017-06-13T23:56:30Z

src/core/scene/a-scene.js

+    onVRPresentChange: {
+      value: function (evt) {
+        // Entering VR.
+        if (evt.display.isPresenting) {


awesome 👏🏽

cvan · 2017-06-14T00:03:58Z

src/core/scene/a-scene.js

        enterVRSuccess();
        return Promise.resolve();

        function enterVRSuccess () {
          self.addState('vr-mode');
-          self.emit('enter-vr', event);


it might be useful to preserve event since the event instance will have the reason, display, etc. properties.

those values could instead be determined through methods and property lookups on the scene element after enter-vr is emitted, but it'd help a bit (especially for displaying info, logging/testing, and the WebVR Agent) to know for which display the scene entered VR.

long story short: can you pass the {display: display} as the second argument?

I think before this was undefined, but I can rig it to pass something useful.

The event before was just the Enter VR button click event. I won't get to passing in a display in this PR.

cvan · 2017-06-14T00:04:30Z

src/core/scene/a-scene.js

@@ -176,24 +183,27 @@ module.exports.AScene = registerElement('a-scene', {
     * Call `requestFullscreen` on desktop.
     * Handle events, states, fullscreen styles.
     *
+     * @param {bool} fromExternal - Whether exiting VR due to an external event (e.g.,


thanks; good comment

cvan · 2017-06-14T00:09:10Z

src/components/scene/vr-mode-ui.js

-    sceneEl.addEventListener('enter-vr', bind(this.updateEnterVRInterface, this));
-    sceneEl.addEventListener('exit-vr', bind(this.updateEnterVRInterface, this));
+    this.updateEnterVRInterface = bind(this.updateEnterVRInterface, this);
+    sceneEl.addEventListener('enter-vr', this.updateEnterVRInterface);


I know you just added a intermediate variable 👍🏽
asking in case I missed something: there should be code here to remove the (three) event listeners, yeah?

cvan · 2017-06-14T00:13:49Z

src/components/scene/auto-enter-vr.js

 * or immediately (if possible) if display name contains data string.
- * The default data string is 'GearVR' for Carmel browser which only does VR.
+ * Default data string is `GearVR` for Carmel browser which only does VR.
 */
 module.exports.Component = registerComponent('auto-enter-vr', {


I'm beginning to question the value of this component. I see it served its purpose initially. And it's definitely helpful when in dev environments, especially with Gear VR and iOS (Cardboard). Thoughts, @ngokevin? (This is more intended as a general question, not an issue with this PR. If you agree this ought to be revisited, I'll file a proper bug.)

I don't have an entirely good grasp on the component, seems the useful thing we can keep and move to <a-scene> module is the display activate handlers. @machenmusik?

See #2707 - now that Oculus Browser is widely available, I think that auto-enter-vr should not be added to scene by default anymore - if developing for Carmel, easy enough to add back in

ngokevin · 2017-06-14T20:25:45Z

Fixing tests

…f aframe (fixes aframevr#2614)

dmarcos · 2017-06-15T05:30:52Z

src/core/scene/a-scene.js

@@ -109,16 +109,26 @@ module.exports.AScene = registerElement('a-scene', {

        // Add to scene index.
        scenes.push(this);
+
+        // Handler to exit VR (e.g., Oculus Browser back button).
+        this.onVRPresentChangeBound = bind(this.onVRPresentChange, this);


We don't need to rename and append bound to the method name.

Some leftovers from when I was debugging :P

ngokevin added the needs review label Jun 9, 2017

ngokevin force-pushed the oculusdeact branch from c3d6194 to c01a63b Compare June 12, 2017 19:17

ngokevin changed the title ~~handle onvrdisplaydeactivate to exitVR (fixes #2614)~~ handle onvrdisplaypresentchange to exitVR (fixes #2614) Jun 12, 2017

cvan suggested changes Jun 13, 2017

View reviewed changes

cvan added needs changes and removed needs review labels Jun 13, 2017

ngokevin force-pushed the oculusdeact branch from c01a63b to b0fdbe7 Compare June 13, 2017 21:52

ngokevin added needs review and removed needs changes labels Jun 13, 2017

ngokevin changed the title ~~handle onvrdisplaypresentchange to exitVR (fixes #2614)~~ handle vrdisplaypresentchange when triggering enter/exit VR outside aframe (fixes #2614) Jun 13, 2017

ngokevin added work in progress and removed needs review labels Jun 13, 2017

cvan suggested changes Jun 14, 2017

View reviewed changes

ngokevin force-pushed the oculusdeact branch 2 times, most recently from b947852 to 6166679 Compare June 14, 2017 18:03

ngokevin added needs review work in progress and removed work in progress needs review labels Jun 14, 2017

handle vrdisplaypresentchange when triggering enter/exit VR outside o…

ccb615a

…f aframe (fixes aframevr#2614)

ngokevin force-pushed the oculusdeact branch from 6166679 to ccb615a Compare June 14, 2017 21:58

ngokevin added needs review and removed work in progress labels Jun 14, 2017

dmarcos reviewed Jun 15, 2017

View reviewed changes

dmarcos merged commit 9ac9246 into aframevr:master Jun 15, 2017

dmarcos mentioned this pull request Jun 15, 2017

Enter VR button not appearing anymore (Oculus browser), impossible to enter VR #2781

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle vrdisplaypresentchange when triggering enter/exit VR outside aframe (fixes #2614) #2751

handle vrdisplaypresentchange when triggering enter/exit VR outside aframe (fixes #2614) #2751

ngokevin commented Jun 9, 2017 •

edited

Loading

cvan left a comment

cvan Jun 13, 2017

cvan Jun 13, 2017 •

edited

Loading

cvan Jun 13, 2017

ngokevin commented Jun 13, 2017

ngokevin commented Jun 13, 2017

cvan left a comment

cvan Jun 13, 2017

cvan Jun 14, 2017

ngokevin Jun 14, 2017

ngokevin Jun 14, 2017

cvan Jun 14, 2017

cvan Jun 14, 2017

cvan Jun 14, 2017

ngokevin Jun 14, 2017

machenmusik Jun 14, 2017

ngokevin commented Jun 14, 2017

dmarcos Jun 15, 2017

ngokevin Jun 15, 2017

handle vrdisplaypresentchange when triggering enter/exit VR outside aframe (fixes #2614) #2751

handle vrdisplaypresentchange when triggering enter/exit VR outside aframe (fixes #2614) #2751

Conversation

ngokevin commented Jun 9, 2017 • edited Loading

cvan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvan Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngokevin commented Jun 13, 2017

ngokevin commented Jun 13, 2017

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

Choose a reason for hiding this comment

ngokevin commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngokevin commented Jun 9, 2017 •

edited

Loading

cvan Jun 13, 2017 •

edited

Loading