Added support for VR headsets that do not provide stageParameters #3000
Conversation
…eParameters attribute.
src/components/look-controls.js
Outdated
| @@ -17,7 +18,9 @@ module.exports.Component = registerComponent('look-controls', { | |||
| enabled: {default: true}, | |||
| hmdEnabled: {default: true}, | |||
| reverseMouseDrag: {default: false}, | |||
| standing: {default: true} | |||
| standing: {default: true}, | |||
| userHeight: {default: 0}, | |||
There was a problem hiding this comment.
Are you using this property anywhere? it seems you are using userHeight from the camera component in getUserHeight
src/components/look-controls.js
Outdated
| * Return user height to use for standing poses, where a device doesn't provide an offset. | ||
| */ | ||
| getUserHeight: function () { | ||
| var headEl = this.data.headElement || this.el.sceneEl.camera.el; |
There was a problem hiding this comment.
What are the cases where the headElement is not the active camera?
There was a problem hiding this comment.
(e.g. spectator camera?)
src/components/look-controls.js
Outdated
| standing: {default: true} | ||
| standing: {default: true}, | ||
| userHeight: {default: 0}, | ||
| headElement: {type: 'selector'} |
There was a problem hiding this comment.
Can we add a description for the use cases of this property?
| @@ -52,4 +53,27 @@ suite('look-controls', function () { | |||
| window.dispatchEvent(new Event('mouseup')); | |||
| }); | |||
| }); | |||
|
|
|||
| suite('head-height', function () { | |||
|
Thank you @dmarcos for review. I removed the properties under question. |
src/components/look-controls.js
Outdated
| * Return user height to use for standing poses, where a device doesn't provide an offset. | ||
| */ | ||
| getUserHeight: function () { | ||
| var headEl = this.el.sceneEl.camera.el; |
There was a problem hiding this comment.
A scene could have multiple cameras. In this case we probably only care about the camera on the entity where this component is setup. I would use the public API this way:
var el = this.el;
var userHeight = el.hasAttribute('camera') && el.getAttribute('camera').userHeight || DEFAULT_CAMERA_HEIGHT;
return userHeight;There was a problem hiding this comment.
I'm worried about the coupling of look-controls and the camera components we have now. Moving userHeight to look-controls is something we have discussed in the past but we can do it in a separate PR
There was a problem hiding this comment.
Yes, this works and looks better. Thank you!
|
Thanks! Pretty close! A small couple of tweaks and we're ready to go. |
|
Nice! Thank you! Welcome as a contributor! |
|
Thank you! |
|
These changes introduced a regression for non positional devices like GearVR, see #3051 |
Description:
Fixes issue of inconsistent head and controller height offset, for cases when stageParameters property on VR display is null.
Changes Proposed:
Provide default user height, if this is the case to be consistent with tracked controls.