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

vincentfretin · 2017-10-02T12:05:02Z

This definitely fix issue #3051
I don't know how to write a test for this, this seems bound to the VRControls and how it renders in the browser...

dmarcos · 2017-10-02T14:10:20Z

The proper solution for this is probably moving the logic to the restore pose of the camera component: https://github.com/aframevr/aframe/blob/master/src/components/camera.js#L99

vincentfretin · 2017-10-02T15:30:41Z

@dmarcos There is already a hasPositionalTracking check in camera to save the pose (position and rotation of the camera) yes. So you mean saving userHeight? But this is set on the VRControls object, not the camera. I'm really lost with this, and I don't have any positional tracking device to test this, only GearVR.

What I know it that before #3000, there was no this.controls.userHeight = this.getUserHeight(); line in look-controls, so the this.controls.userHeight stayed at 0.0. Adding a condition to it solved the issue for GearVR.

machenmusik · 2017-10-02T15:39:18Z

It feels to me as though we are trying to bake too many smarts into the camera -- when it comes to uses for render textures and spectator views, the camera is just that - not meant to be a stand-in for the user and presumed HMD positioning. I thought that (in current incarnation) look-controls was intended to contain the latter, and that from prior discussions we thought this area might need some refactoring. My recollection is that recent changes have handed to use absolute positioning not relative positioning for room scale etc., and this repeated application of relative offset may be a vestige of doing things the old way that should not be re-introduced regardless of where?

…refs aframevr#3051)

dmarcos · 2017-10-03T09:02:42Z

All right. I think we should then move the logic to save / restore the camera pose / position from the camera to look-controls as well as the userHeight property.

vincentfretin · 2017-10-08T09:47:36Z

One other thing that seems to differ between positional and non positional tracking devices is that with positional tracking you are placed to z -2 or something like that, and with GearVR you are probably at z: 0. So there are some examples, notably c-frame/aframe-super-hands-component#89 where you are too close to the object, here with the big cube.

DavidWyand · 2017-10-09T07:05:04Z

I just wanted to add that this fix works for me. It allows my project to work on GearVR with the Oculus browser, and the Vive using Firefox 56.0, without having to make any camera changes. Thanks!

vincentfretin · 2017-10-09T07:48:09Z

@dmarcos Maybe we can merge this and you folks work eventually on a refactoring in another branch?

ngokevin · 2017-10-10T19:13:49Z

Do this to fix tests: https://github.com/aframevr/aframe/pull/3149/files#diff-f9aac19c37c38417114cd801ed8016c4R12

ngokevin · 2017-10-10T21:23:00Z

Fixed the tests.

@dmarcos this fix make sense? We can refactor later.

olga-microsoft · 2017-10-10T23:29:36Z

@vincentfretin sorry for breaking GearVR and thank you for issuing the fix. I verified that it works as expected with Window Mixed Reality.

ngokevin · 2017-10-11T12:12:21Z

src/components/look-controls.js

+    hasPositionalTracking = this.hasPositionalTracking !== undefined
+      ? this.hasPositionalTracking
+      : checkHasPositionalTracking();
+    if (hasPositionalTracking) {


Trying to understand. How come we still want to apply user height if there is positional tracking? Seems to me you don't want to apply an offset if your height is tracked, or we should check more specifically that there are no stage parameters? And how come it was adding up more and more each time enter VR?

cvan

looking good. thanks for this. let me know when you want another review. no rush. also, just so I know, which example(s) are you testing against? and on which platforms have you tested?

cvan · 2017-10-20T10:52:28Z

src/components/look-controls.js

@@ -1,8 +1,11 @@
 var registerComponent = require('../core/component').registerComponent;
 var THREE = require('../lib/three');
+var utils = require('../utils/');
 var DEFAULT_CAMERA_HEIGHT = require('../constants').DEFAULT_CAMERA_HEIGHT;
 var bind = require('../utils/bind');


feel free to change this to utils.bind

cvan · 2017-10-20T10:52:55Z

src/components/look-controls.js

 var DEFAULT_CAMERA_HEIGHT = require('../constants').DEFAULT_CAMERA_HEIGHT;
 var bind = require('../utils/bind');

+var checkHasPositionalTracking = utils.device.checkHasPositionalTracking;


do you want to import this function above?

cvan · 2017-10-20T10:53:45Z

src/components/look-controls.js

    this.controls.standing = data.standing;
-    this.controls.userHeight = this.getUserHeight();
+
+    hasPositionalTracking = this.hasPositionalTracking !== undefined


if this is already expect to be a Boolean, you can simplify this expression

cvan · 2017-10-20T10:54:52Z

tests/__init.test.js

    requestAnimationFrame: function () { return 1; },
-    cancelAnimationFrame: function (h) { return window.cancelAnimationFrame(1); }
+    requestPresent: resolvePromise,
+    submitFrame: function () { return; }


can you add a test for checking the tracking method and user height?

dmarcos · 2017-10-20T14:26:49Z

VRControls / VREffect will be soon gone. See #3152 I leave it open as a reference and a reminder to port the logic. Nothing to do here.

dmarcos · 2017-12-17T07:13:22Z

After the removal of VREffect / VRControls this PR is not applicable anymore.

vincentfretin mentioned this pull request Oct 2, 2017

Increasing offset when entering VR for non positional devices #3051

Closed

only set this.controls.userHeight if device has positional tracking (…

4da8fe4

…refs aframevr#3051)

vincentfretin force-pushed the fix-increasing-offset branch from 24fa2aa to 4da8fe4 Compare October 2, 2017 16:49

machenmusik mentioned this pull request Oct 3, 2017

User height offset is added multiple times when entering and exiting VR #3113

Closed

vincentfretin mentioned this pull request Oct 8, 2017

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

Closed

vincentfretin mentioned this pull request Oct 9, 2017

In the text it says 1.6 while in the table 0 #3142

Merged

fix tests for only setting userHeight if pos tracking

d422069

ngokevin added the needs review label Oct 10, 2017

olga-microsoft approved these changes Oct 10, 2017

View reviewed changes

ngokevin reviewed Oct 11, 2017

View reviewed changes

cvan suggested changes Oct 20, 2017

View reviewed changes

dmarcos added needs rebase and removed needs review labels Oct 20, 2017

robertlong mentioned this pull request Nov 21, 2017

Fixes for Daydream Hubs-Foundation/hubs#15

Merged

dmarcos closed this Dec 17, 2017

vincentfretin deleted the fix-increasing-offset branch April 15, 2023 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vincentfretin commented Oct 2, 2017 •

edited

Loading

dmarcos commented Oct 2, 2017

vincentfretin commented Oct 2, 2017

machenmusik commented Oct 2, 2017

dmarcos commented Oct 3, 2017 •

edited

Loading

vincentfretin commented Oct 8, 2017

DavidWyand commented Oct 9, 2017

vincentfretin commented Oct 9, 2017

ngokevin commented Oct 10, 2017

ngokevin commented Oct 10, 2017

olga-microsoft commented Oct 10, 2017

ngokevin Oct 11, 2017

cvan left a comment

cvan Oct 20, 2017

cvan Oct 20, 2017

cvan Oct 20, 2017

cvan Oct 20, 2017

dmarcos commented Oct 20, 2017

dmarcos commented Dec 17, 2017

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

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

Conversation

vincentfretin commented Oct 2, 2017 • edited Loading

dmarcos commented Oct 2, 2017

vincentfretin commented Oct 2, 2017

machenmusik commented Oct 2, 2017

dmarcos commented Oct 3, 2017 • edited Loading

vincentfretin commented Oct 8, 2017

DavidWyand commented Oct 9, 2017

vincentfretin commented Oct 9, 2017

ngokevin commented Oct 10, 2017

ngokevin commented Oct 10, 2017

olga-microsoft commented Oct 10, 2017

ngokevin Oct 11, 2017

Choose a reason for hiding this comment

cvan left a comment

Choose a reason for hiding this comment

cvan Oct 20, 2017

Choose a reason for hiding this comment

cvan Oct 20, 2017

Choose a reason for hiding this comment

cvan Oct 20, 2017

Choose a reason for hiding this comment

cvan Oct 20, 2017

Choose a reason for hiding this comment

dmarcos commented Oct 20, 2017

dmarcos commented Dec 17, 2017

vincentfretin commented Oct 2, 2017 •

edited

Loading

dmarcos commented Oct 3, 2017 •

edited

Loading