Remove VREffect / VRControls in favor of the new WebGLRenderer API #3152

dmarcos · 2017-10-11T01:24:43Z

Tested on:

Moving THREE dependency to the dev branch temporarly until r88 release.

donmccurdy · 2017-10-14T20:09:26Z

examples/boilerplate/hello-world/index.html

  </head>
  <body>
-    <a-scene>
+    <a-scene debug>


unintended?

donmccurdy · 2017-10-14T20:11:57Z

src/components/look-controls.js

-    hmdQuaternion = hmdQuaternion.copy(this.dolly.quaternion);
-    hmdEuler.setFromQuaternion(hmdQuaternion, 'YXZ');
+    hmdEuler.setFromQuaternion(object3D.quaternion, 'YXZ');
+    console.log();


~~console.log();~~

donmccurdy · 2017-10-14T20:12:32Z

src/core/scene/a-scene.js


        // Enter VR on `vrdisplayactivate` (e.g. putting on Rift headset).
-        window.addEventListener('vrdisplayactivate', this.enterVRBound);
+        // window.addEventListener('vrdisplayactivate', this.enterVRBound);


donmccurdy · 2017-10-14T20:14:44Z

src/components/look-controls.js

  },

  play: function () {
+    this.camera = this.el.getObject3D('camera');


I don't see anywhere this.camera is being used.

dmarcos · 2017-10-14T21:30:49Z

Note the work in progress label

vincentfretin · 2017-10-28T15:49:56Z

I know it's a WIP, just giving my feedback.
On GearVR, Oculus browser and Samsung Internet, in or out vr mode, the eyes stay at y:0. In vr mode, the controller model appears at configured user-height.
With Samsung Internet, not in vr mode, the camera doesn't move anymore. In Oculus browser, this never worked.

dmarcos · 2017-11-02T11:56:06Z

@vincentfretin Thanks for testing. I believe it now should work on GearVR as expected and also Daydream.

vincentfretin · 2017-11-02T18:11:29Z

I tested on GearVR. For the user height stuff, it's ok now. But there is really something wrong with the camera, it seems that it's the ground that move, not my head. I really want to throw up right now, seriously. :) With a cursor on camera, the cursor doesn't follow my head, it stays where it was initially.

vincentfretin · 2017-11-02T18:13:12Z

@fernandojsg If you want to throw up, you can test that branch with GearVR. :)

dmarcos · 2017-11-02T18:19:46Z

@vincentfretin what example are you running?

vincentfretin · 2017-11-03T08:05:03Z

On my project actually https://github.com/vincentfretin/aframe-sandbox
On which example are you testing? So we can test the same thing.

vincentfretin · 2017-11-03T08:35:25Z

I use yarn with yarn workspaces in my project. If you want to test it with your branch:

git clone git@github.com:vincentfretin/aframe-sandbox.git
cd aframe-sandbox
./workspaces-install.sh
cd workspaces/aframe
git remote add dmarcos git@github.com:dmarcos/aframe.git
git fetch dmarcos removeVREffect
git checkout -b removeVREffect dmarcos/removeVREffect
cd -
yarn
replace `"main": "dist/aframe-master.js"` by `"main": "src/index.js"` in `node_modules/aframe/package.json`
npm start

vincentfretin · 2017-11-11T18:13:01Z

src/utils/device.js


 /**
- * Determine if a headset is connected by checking if the orientation is available.
+ * Determine if a headset is connected by checking if a vrDissplay is available.


typo vrDissplay => vrDisplay

vincentfretin · 2017-11-12T09:53:42Z

I tried to understand the changes of this PR, but this is too advanced for me to say what could be the issue. Can you see what I describe at least?

Comments are obsolete after all the code changes

dmarcos · 2017-12-07T20:20:54Z

r?

ngokevin · 2017-12-07T20:55:23Z

src/components/look-controls.js

  /**
-   * Handle positional tracking.
-   */
+    * Handle positional tracking.


docstring messed up a bit

ngokevin · 2017-12-07T20:56:09Z

src/components/look-controls.js

-  },
+    // Calculate polyfilled HMD quaternion.
+    this.polyfillControls.update();
+    hmdEuler.setFromQuaternion(this.polyfillObject.quaternion, 'YXZ');


I always wondered how come we do YXZ order?

It's the best order to prevent gimbal lock when converting Euler <-> Quaternions.

ngokevin · 2017-12-07T20:58:05Z

src/core/scene/a-scene.js

        var self = this;
-        var effect = this.effect;
-
+        var vrDisplay = utils.device.getVRDisplay();


we can initialize this in the if where it's needed

ngokevin · 2017-12-07T20:58:52Z

src/core/scene/a-scene.js

          self.emit('exit-vr', {target: self});
        }
-
-        function exitVRFailure (err) {


Will three.js notify on errors?

ngokevin · 2017-12-07T20:59:25Z

src/core/scene/a-scene.js

        // Enter VR via WebVR API.
-        if (!fromExternal && (this.checkHeadsetConnected() || this.isMobile)) {
-          return effect && effect.requestPresent().then(enterVRSuccess, enterVRFailure) || Promise.reject(new Error('VREffect not initialized'));
+        if (!fromExternal && (self.checkHeadsetConnected() || self.isMobile)) {


don't need to use self here

ngokevin · 2017-12-07T21:00:10Z

src/core/scene/a-scene.js


    resize: {
-      value: function () {
+      value: function (ll) {


ngokevin · 2017-12-07T21:00:43Z

src/core/scene/a-scene.js

        var embedded = this.getAttribute('embedded') && !this.is('vr-mode');
        var size;
-        var isEffectPresenting = this.effect && this.effect.isPresenting;
+        var vrDevice = this.renderer.vr.getDevice();


we can initialize these two variables out of the declaration block since they have some logic

ngokevin · 2017-12-07T21:01:47Z

src/core/scene/a-scene.js

    render: {
      value: function () {
-        var effect = this.effect;
        var delta = this.clock.getDelta() * 1000;


do we want to separate declare vars vs logic?

ngokevin · 2017-12-07T21:05:05Z

tests/components/camera.test.js

+      cameraEl.setAttribute('position', {x: 9, y: 9, z: 9});
      sceneEl.emit('exit-vr');
-      assert.shallowDeepEqual(cameraEl.getAttribute('position'), {x: 0, y: 1.6, z: 0});
+      assert.shallowDeepEqual(cameraEl.getAttribute('position'), {x: 6, y: 6, z: 6});


We should have a test preserve the user height check here?

ngokevin · 2017-12-07T21:06:30Z

tests/core/controls.test.js

+    var cameraEl = el.sceneEl.querySelector('[camera]');
+    this.orientation = [0, Math.PI, 0];
+    el.sceneEl.render();
+    var rotation = cameraEl.getAttribute('rotation');


vars on top for this file

dmarcos · 2017-12-08T08:03:00Z

I moved all the userHeight logic from camera to look-controls. It always felt weird to have the idea of user associated to the camera.

dmarcos · 2017-12-08T08:05:29Z

All the comments have been addressed

cvan · 2017-12-08T09:43:50Z

been watching this and testing locally. nice work! 👍

which Windows Mixed Reality headsets and which Android phones for Gear VR + Cardboard did you test with?

though it's deprecated, just out of curiosity, have you tested on desktop Chromium?

dmarcos · 2017-12-08T17:06:03Z

WebVR is already available on desktop Canary. If there’s any problem with the old Chromium builds I would encourage users to move to Canary.

It should not make any difference what Window MR headset it is tested on. I only have an Acer available.

For GearVR all devices should be equivalent besides performance capabilities. My testing device is a Galaxy S6

machenmusik · 2017-12-08T18:16:49Z

AFAIK at the moment, they still need to add Rift support to Chrome Canary - but soon experimental Chromium may not be necessary at all

ngokevin · 2017-12-08T21:41:01Z

A couple of more style-related nits to address from before and then r+wc

vincentfretin · 2018-02-02T14:33:53Z

Ok, I tested latest master, works great now on GearVR.

dmarcos · 2018-02-02T16:45:33Z

@vincentfretin Thank you 🙏

dmarcos added the work in progress label Oct 11, 2017

cvan mentioned this pull request Oct 12, 2017

Support for vive-tracker #2583

Closed

donmccurdy previously requested changes Oct 14, 2017

View reviewed changes

donmccurdy added this to the 0.8.0 milestone Oct 17, 2017

This was referenced Oct 19, 2017

Webxr refactor #3181

Closed

only set this.controls.userHeight if device has positional tracking #3111

Closed

dmarcos force-pushed the removeVREffect branch 5 times, most recently from 70e3e45 to 8bdcf25 Compare October 28, 2017 03:30

vincentfretin mentioned this pull request Oct 28, 2017

"Progressive controls with physics" example not usable with GearVR c-frame/aframe-super-hands-component#89

Closed

dmarcos force-pushed the removeVREffect branch 4 times, most recently from f75a7ad to 7714553 Compare November 2, 2017 11:54

vincentfretin reviewed Nov 11, 2017

View reviewed changes

dmarcos force-pushed the removeVREffect branch from 7714553 to 41b5276 Compare November 15, 2017 01:42

dmarcos force-pushed the removeVREffect branch 3 times, most recently from 642613d to 5108b68 Compare December 6, 2017 10:09

dmarcos force-pushed the removeVREffect branch 4 times, most recently from dd370c2 to 3042b9b Compare December 7, 2017 08:11

dmarcos added needs review and removed work in progress labels Dec 7, 2017

ngokevin requested changes Dec 7, 2017

View reviewed changes

dmarcos force-pushed the removeVREffect branch from 3042b9b to 9073d3e Compare December 8, 2017 08:01

dmarcos force-pushed the removeVREffect branch from 9073d3e to 1fb47bc Compare December 8, 2017 08:04

dmarcos force-pushed the removeVREffect branch 2 times, most recently from bcd4392 to bfd274e Compare December 12, 2017 09:11

Remove VREffect / VRControls in favor of the new WebGLRenderer API

b72d93c

dmarcos force-pushed the removeVREffect branch from bfd274e to b72d93c Compare December 12, 2017 09:54

dmarcos merged commit 3d188db into aframevr:master Dec 12, 2017

donmccurdy mentioned this pull request Mar 15, 2018

Reverse Mouse Drag has no effect #3459

Closed

Uh oh!

Remove VREffect / VRControls in favor of the new WebGLRenderer API #3152

Remove VREffect / VRControls in favor of the new WebGLRenderer API #3152

Uh oh!

Conversation

dmarcos commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

dmarcos commented Oct 14, 2017

vincentfretin commented Oct 28, 2017

dmarcos commented Nov 2, 2017

vincentfretin commented Nov 2, 2017

vincentfretin commented Nov 2, 2017

dmarcos commented Nov 2, 2017

vincentfretin commented Nov 3, 2017

vincentfretin commented Nov 3, 2017

Choose a reason for hiding this comment

vincentfretin commented Nov 12, 2017

dmarcos commented Dec 7, 2017

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

dmarcos commented Dec 8, 2017

dmarcos commented Dec 8, 2017

cvan commented Dec 8, 2017

dmarcos commented Dec 8, 2017

machenmusik commented Dec 8, 2017

ngokevin commented Dec 8, 2017

vincentfretin commented Feb 2, 2018

dmarcos commented Feb 2, 2018

Labels

6 participants

dmarcos commented Oct 11, 2017 •

edited

Loading