[webvr] Support HTC Vive orientation and position tracking in the Gamepad API

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: kip, Assigned: daoshengmu)

Tracking

(Depends on 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [gfx-noted][webvr], )

Attachments

(3 attachments)

Reporter

Description

3 years ago
Ongoing discussions in the WebVR spec GitHub repository describe ways to support orientation and position tracking for controllers in the Gamepad API.  Chromium experimental builds include a pose object mirroring the VRPose in the WebVR API.

Updated

3 years ago
Whiteboard: [gfx-noted]
Assignee

Updated

3 years ago
Assignee: nobody → dmu
Assignee

Comment 1

3 years ago
gamepadPose attribute in Gamepad API extension 
https://w3c.github.io/gamepad/extensions.html#gamepadpose-interface

Added GamepadPose discussion in W3C github.
https://github.com/w3c/gamepad/pull/25
Assignee

Comment 5

3 years ago
mozreview-review
Comment on attachment 8803814 [details]
Bug 1299932 - Part 3: Handle OpenVR controller position and orientation;

https://reviewboard.mozilla.org/r/87970/#review87012

::: gfx/vr/gfxVROpenVR.cpp:263
(Diff revision 1)
>      }
>    }
>  
> -  ::vr::TrackedDevicePose_t poses[::vr::k_unMaxTrackedDeviceCount];
>    // Note: We *must* call WaitGetPoses in order for any rendering to happen at all
> -  mVRCompositor->WaitGetPoses(poses, ::vr::k_unMaxTrackedDeviceCount, nullptr, 0);
> +  mVRCompositor->WaitGetPoses(gTrackedPoses, ::vr::k_unMaxTrackedDeviceCount, nullptr, 0);

I am considering if we should move mVRCompositor->WaitGetPoses to VRManager::NotifyVsync because both of VRDisplayOpenVR and VRControllerOpenVR need TrackedDevicePose.

If we have time to refactor the current VR architecture. It should be a VRManager manages VR displays and controllers. This VRManager at animation time will update VR runtime to get the latest position and orientation of tracked devices, then ask VRDisplay to render and VRController to send events to Gamepad API.
Assignee

Comment 6

3 years ago
Kip, do you think we should move mVRCompositor->WaitGetPoses(); (https://dxr.mozilla.org/mozilla-central/rev/c845bfd0accb7e0c29b41713255963b08006e701/gfx/vr/gfxVROpenVR.cpp#262) from GetSensorState to VRManager::NotifyVsync.

I know you fetch sensor tracking states at GetSensorState() for OpenVR, Oculus, and OSVR. But, they seem to be defined in order to support HMD. We probably need to consider the case of VR controllers. Ideally, we should get all tracked devices once at a frame, then set the states to VRDisplayHost and VRControllerHost. 

Or, we have to suppose VRDisplayHost::GetSensorState() would be called per frame and make a way to let VRController get their devices' sensor state from VRDisplayHost?
Flags: needinfo?(kgilbert)
Reporter

Comment 7

3 years ago
(In reply to Daosheng Mu[:daoshengmu] from comment #6)
> Kip, do you think we should move mVRCompositor->WaitGetPoses();
> (https://dxr.mozilla.org/mozilla-central/rev/
> c845bfd0accb7e0c29b41713255963b08006e701/gfx/vr/gfxVROpenVR.cpp#262) from
> GetSensorState to VRManager::NotifyVsync.
> 
> I know you fetch sensor tracking states at GetSensorState() for OpenVR,
> Oculus, and OSVR. But, they seem to be defined in order to support HMD. We
> probably need to consider the case of VR controllers. Ideally, we should get
> all tracked devices once at a frame, then set the states to VRDisplayHost
> and VRControllerHost. 
> 
> Or, we have to suppose VRDisplayHost::GetSensorState() would be called per
> frame and make a way to let VRController get their devices' sensor state
> from VRDisplayHost?

We should not move the HMD pose code or otherwise integrate it with the controller pose routines, as we would lose an optimization that reduces perceived latency in the HMD.

The WebVR spec requires us to return a consistent pose for the HMD until the next frame; however, we defer getting that pose as long as possible in order to get sub-frame late pose prediction updates from the underlying API.  After the first request for the pose; however, we must return the same pose until the end of the frame.  These guarantees are not required for controllers.

Unlike the HMD pose, the controller poses are not used for re-projection of the scene with technologies like Async Time Warp and Async Space Warp.  For tracked controllers, latency is still important, but not as critical to preventing users from feeling sick.

It would be acceptable to poll the controller poses separately, once per frame, at vsync time.
Flags: needinfo?(kgilbert)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 11

3 years ago
mozreview-review
Comment on attachment 8803812 [details]
Bug 1299932 - Part 1: Implement gamepadPose attribute in Gamepad API;

https://reviewboard.mozilla.org/r/87966/#review87678

::: dom/gamepad/Gamepad.h:13
(Diff revision 2)
>  #define mozilla_dom_gamepad_Gamepad_h
>  
>  #include "mozilla/ErrorResult.h"
>  #include "mozilla/dom/GamepadBinding.h"
>  #include "mozilla/dom/GamepadButton.h"
> +#include "mozilla/dom/GamepadPose.h"

This include file probably we don't need.

::: dom/gamepad/GamepadPose.h:120
(Diff revision 2)
> +  bool HasOrientation() const;
> +  bool HasPosition() const;
> +  void GetPosition(JSContext* aJSContext,
> +                   JS::MutableHandle<JSObject*> aRetval,
> +                   ErrorResult& aRv);
> +  //Float32Array* GetPosition() {return &mPosition;}

Remove this line

::: dom/gamepad/GamepadPose.cpp:201
(Diff revision 2)
> +}
> +
> +void
> +GamepadPose::SetPoseState(const GamepadPoseState& aPose)
> +{
> +  // TODO: check if assign successfully

Remove this!

::: dom/webidl/moz.build:819
(Diff revision 2)
>  if CONFIG['MOZ_GAMEPAD']:
>      GENERATED_EVENTS_WEBIDL_FILES += [
>          'GamepadAxisMoveEvent.webidl',
>          'GamepadButtonEvent.webidl',
>          'GamepadEvent.webidl',
> +        'GamepadPoseStateEvent.webidl'

Remove this!
Assignee

Comment 12

3 years ago
mozreview-review
Comment on attachment 8803813 [details]
Bug 1299932 - Part 2: Support gamepadPose in GamepadManager;

https://reviewboard.mozilla.org/r/87968/#review87680

::: dom/webidl/moz.build
(Diff revision 2)
>  
>  if CONFIG['MOZ_GAMEPAD']:
>      GENERATED_EVENTS_WEBIDL_FILES += [
>          'GamepadAxisMoveEvent.webidl',
>          'GamepadButtonEvent.webidl',
> -        'GamepadEvent.webidl',

Redundant changes, I need to remove this.
Reporter

Comment 13

3 years ago
mozreview-review
Comment on attachment 8803812 [details]
Bug 1299932 - Part 1: Implement gamepadPose attribute in Gamepad API;

https://reviewboard.mozilla.org/r/87966/#review87844

Looks good.  r=me with the issues I marked addressed and those that you have marked as well.

::: dom/gamepad/GamepadPose.h:42
(Diff revision 2)
> +  Cap_LinearAcceleration = 1 << 4,
> +  /**
> +   * Cap_StageParameters is set if the Gamepad is capable of room scale VR
> +   * and can report the StageParameters to describe the space.
> +   */
> +  Cap_StageParameters = 1 << 5,

Would Cap_StageParameters apply to Gamepads?

Perhaps this is relevant in the future if we add some api to test intersection of pose with the bounds described by the stage parameters, but this hasn't been specced yet.

Perhaps remove for now?

::: dom/gamepad/GamepadPose.h:82
(Diff revision 2)
> +           && angularVelocity[1] == aPose.angularVelocity[1]
> +           && angularVelocity[2] == aPose.angularVelocity[2]
> +           && angularAcceleration[0] == aPose.angularAcceleration[0]
> +           && angularAcceleration[1] == aPose.angularAcceleration[1]
> +           && angularAcceleration[2] == aPose.angularAcceleration[2]
> +           && linearVelocity[1] == aPose.linearVelocity[1]

Are we missing linearVelocity[0] here?

::: dom/tests/mochitest/general/test_interfaces.html:408
(Diff revision 2)
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      "GamepadButton",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      "GamepadEvent",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    {name: "GamepadPose", disabled: true},

Be sure to include a DOM peer in the review for this file and .webidl file changes

::: modules/libpref/init/all.js:199
(Diff revision 2)
>  #ifdef RELEASE_OR_BETA
>  pref("dom.gamepad.non_standard_events.enabled", false);
>  #else
>  pref("dom.gamepad.non_standard_events.enabled", true);
>  #endif
> +pref("dom.gamepad.extensions.enabled", false);

Perhaps we could automatically enable dom.gamepad.extensions.enable when dom.vr.enabled is set?

If we must enable separately, then we need to be sure to include this pref in the TestPilot experiment which will also flip dom.vr.enabled in production.

Looks okay to me either way.
Attachment #8803812 - Flags: review?(kgilbert) → review+
Reporter

Comment 14

3 years ago
mozreview-review
Comment on attachment 8803813 [details]
Bug 1299932 - Part 2: Support gamepadPose in GamepadManager;

https://reviewboard.mozilla.org/r/87968/#review87848

Looks good to me, thanks!  r=me with the redundant changes lines you have marked removed.
Attachment #8803813 - Flags: review?(kgilbert) → review+

Comment 15

3 years ago
mozreview-review
Comment on attachment 8803813 [details]
Bug 1299932 - Part 2: Support gamepadPose in GamepadManager;

https://reviewboard.mozilla.org/r/87968/#review87876
Attachment #8803813 - Flags: review?(cleu) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 19

3 years ago
mozreview-review
Comment on attachment 8803812 [details]
Bug 1299932 - Part 1: Implement gamepadPose attribute in Gamepad API;

https://reviewboard.mozilla.org/r/87966/#review88038

::: dom/gamepad/GamepadPose.cpp:103
(Diff revision 3)
> +                         JS::MutableHandle<JSObject*> aRetval,
> +                         ErrorResult& aRv)
> +{
> +  if (mPoseState.flags & GamepadCapabilityFlags::Cap_Position) {
> +    // Lazily create the Float32Array
> +    mPosition = Float32Array::Create(aJSContext, this, 3, mPoseState.position);

Kip, I notice mPosition is always non-null here, so I always need to create a new Float32Array. However, I saw your code in VRPose (https://dxr.mozilla.org/mozilla-central/rev/3f4c3a3cabaf94958834d3a8935adfb4a887942d/dom/vr/VRDisplay.cpp#328). You check the mPosition if it is non-null, otherwise creating a new one. I am not sure if I do something wrong on implementing cycle collection. Could you give me some comments?
Assignee

Comment 20

3 years ago
(In reply to Daosheng Mu[:daoshengmu] from comment #19)
> Comment on attachment 8803812 [details]
> Bug 1299932 - Part 1: Implement gamepadPose attribute in Gamepad API;
> 
> https://reviewboard.mozilla.org/r/87966/#review88038
> 
> ::: dom/gamepad/GamepadPose.cpp:103
> (Diff revision 3)
> > +                         JS::MutableHandle<JSObject*> aRetval,
> > +                         ErrorResult& aRv)
> > +{
> > +  if (mPoseState.flags & GamepadCapabilityFlags::Cap_Position) {
> > +    // Lazily create the Float32Array
> > +    mPosition = Float32Array::Create(aJSContext, this, 3, mPoseState.position);
> 
> Kip, I notice mPosition is always non-null here, so I always need to create
> a new Float32Array. However, I saw your code in VRPose
> (https://dxr.mozilla.org/mozilla-central/rev/
> 3f4c3a3cabaf94958834d3a8935adfb4a887942d/dom/vr/VRDisplay.cpp#328). You
> check the mPosition if it is non-null, otherwise creating a new one. I am
> not sure if I do something wrong on implementing cycle collection. Could you
> give me some comments?

needinfo? kip.
Flags: needinfo?(kgilbert)
Assignee

Comment 21

3 years ago
(In reply to :kip (Kearwood Gilbert) from comment #7)
> (In reply to Daosheng Mu[:daoshengmu] from comment #6)
> > Kip, do you think we should move mVRCompositor->WaitGetPoses();
> > (https://dxr.mozilla.org/mozilla-central/rev/
> > c845bfd0accb7e0c29b41713255963b08006e701/gfx/vr/gfxVROpenVR.cpp#262) from
> > GetSensorState to VRManager::NotifyVsync.
> > 
> > I know you fetch sensor tracking states at GetSensorState() for OpenVR,
> > Oculus, and OSVR. But, they seem to be defined in order to support HMD. We
> > probably need to consider the case of VR controllers. Ideally, we should get
> > all tracked devices once at a frame, then set the states to VRDisplayHost
> > and VRControllerHost. 
> > 
> > Or, we have to suppose VRDisplayHost::GetSensorState() would be called per
> > frame and make a way to let VRController get their devices' sensor state
> > from VRDisplayHost?
> 
> We should not move the HMD pose code or otherwise integrate it with the
> controller pose routines, as we would lose an optimization that reduces
> perceived latency in the HMD.
> 
> The WebVR spec requires us to return a consistent pose for the HMD until the
> next frame; however, we defer getting that pose as long as possible in order
> to get sub-frame late pose prediction updates from the underlying API. 
> After the first request for the pose; however, we must return the same pose
> until the end of the frame.  These guarantees are not required for
> controllers.
> 
> Unlike the HMD pose, the controller poses are not used for re-projection of
> the scene with technologies like Async Time Warp and Async Space Warp.  For
> tracked controllers, latency is still important, but not as critical to
> preventing users from feeling sick.
> 
> It would be acceptable to poll the controller poses separately, once per
> frame, at vsync time.

Finally, I decide to use mVRSystem->GetDeviceToAbsoluteTrackingPose(vr::TrackingUniverseSeated,...) to get the pose of VR controllers. I have confirmed the result it returns is the same as mVRCompositor->WaitGetPoses(). I think GetDeviceToAbsoluteTrackingPose() is an alternative way for room-scale for the standing pose, and we have already provided StageParameter for this field.
Assignee

Comment 22

3 years ago
mozreview-review
Comment on attachment 8803814 [details]
Bug 1299932 - Part 3: Handle OpenVR controller position and orientation;

https://reviewboard.mozilla.org/r/87970/#review88088

::: gfx/vr/gfxVROpenVR.cpp:606
(Diff revision 3)
>      }
> +
> +    // Start to process pose
> +    vr::TrackedDevicePose_t poses[vr::k_unMaxTrackedDeviceCount];
> +    mVRSystem->GetDeviceToAbsoluteTrackingPose(vr::TrackingUniverseSeated, 0.0f,
> +                                               poses, vr::k_unMaxTrackedDeviceCount);

I should move this GetDeviceToAbsoluteTrackingPose() function out of this for-loop (Ln 577).
Assignee

Comment 23

3 years ago
mozreview-review-reply
Comment on attachment 8803812 [details]
Bug 1299932 - Part 1: Implement gamepadPose attribute in Gamepad API;

https://reviewboard.mozilla.org/r/87966/#review87678

> This include file probably we don't need.

We still need it because GamepadBinding uses it.
Reporter

Comment 24

3 years ago
mozreview-review
Comment on attachment 8803814 [details]
Bug 1299932 - Part 3: Handle OpenVR controller position and orientation;

https://reviewboard.mozilla.org/r/87970/#review88216

r=me with the small nit I marked addressed.

::: gfx/vr/gfxVROpenVR.cpp:617
(Diff revision 3)
> +
> +      // NOTE! mDeviceToAbsoluteTracking is a 3x4 matrix, not 4x4.  But
> +      // because of its arrangement, we can copy the 12 elements in and
> +      // then transpose them to the right place.  We do this so we can
> +      // pull out a Quaternion.
> +      memcpy(&m._11, &pose.mDeviceToAbsoluteTracking, sizeof(float) * 12);

I added a "components" member to gfx::Matrix4x4 so you can use a cleaner syntax:

memcpy(m.components, &pose......
Attachment #8803814 - Flags: review?(kgilbert) → review+
Reporter

Comment 25

3 years ago
(In reply to Daosheng Mu[:daoshengmu] from comment #20)
> (In reply to Daosheng Mu[:daoshengmu] from comment #19)
> > Comment on attachment 8803812 [details]
> > Bug 1299932 - Part 1: Implement gamepadPose attribute in Gamepad API;
> > 
> > https://reviewboard.mozilla.org/r/87966/#review88038
> > 
> > ::: dom/gamepad/GamepadPose.cpp:103
> > (Diff revision 3)
> > > +                         JS::MutableHandle<JSObject*> aRetval,
> > > +                         ErrorResult& aRv)
> > > +{
> > > +  if (mPoseState.flags & GamepadCapabilityFlags::Cap_Position) {
> > > +    // Lazily create the Float32Array
> > > +    mPosition = Float32Array::Create(aJSContext, this, 3, mPoseState.position);
> > 
> > Kip, I notice mPosition is always non-null here, so I always need to create
> > a new Float32Array. However, I saw your code in VRPose
> > (https://dxr.mozilla.org/mozilla-central/rev/
> > 3f4c3a3cabaf94958834d3a8935adfb4a887942d/dom/vr/VRDisplay.cpp#328). You
> > check the mPosition if it is non-null, otherwise creating a new one. I am
> > not sure if I do something wrong on implementing cycle collection. Could you
> > give me some comments?
> 
> needinfo? kip.

In the case of VRPose::GetPosition, I am just trying to delay the cost of creating the Float32Array until it is known to be needed.  I expect a new VRPose to be created if the value will ever change.  One exception is if Cap_Position is not set, I always return nullptr.

I think that a DOM peer could do a better job than me in recommending the best approach.  I'd suggest adding a comment for the reviewer to double-check the cycle collection there while they review your webidl changes.
Flags: needinfo?(kgilbert)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 29

3 years ago
In this update, following the comment 22 and comment 24. Also, I refer the comment 13, automatically enabling dom.gamepad.extensions while dom.vr.enable is enabled. Currently, we only have VR uses this gamepad extension API, for other cases, we should check Preferences::GetBool("dom.gamepad.extensions.enabled"), and then allow it to send GamepadChangeEvent to GamepadManager. In this way, it could reduce the messages in IPC channel.

So far, try looks good (https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1518961e60e)
Assignee

Comment 30

3 years ago
mozreview-review
Comment on attachment 8803812 [details]
Bug 1299932 - Part 1: Implement gamepadPose attribute in Gamepad API;

https://reviewboard.mozilla.org/r/87966/#review88282

qdot, please help me take look at my implementation of GamepadPose cycle collection. I am not sure why I can't use the same way as VRPose to manage the timing for new Float32Array().

::: dom/gamepad/GamepadPose.cpp:26
(Diff revision 4)
> +  tmp->mPosition = nullptr;
> +  tmp->mLinearVelocity = nullptr;
> +  tmp->mLinearAcceleration = nullptr;
> +  tmp->mOrientation = nullptr;
> +  tmp->mAngularVelocity = nullptr;
> +  tmp->mAngularAcceleration = nullptr;

I declare these attributes of GamepadPose unlink here.

::: dom/gamepad/GamepadPose.cpp:104
(Diff revision 4)
> +                         ErrorResult& aRv)
> +{
> +  if (mPoseState.flags & GamepadCapabilityFlags::Cap_Position) {
> +    // Lazily create the Float32Array
> +    mPosition = Float32Array::Create(aJSContext, this, 3, mPoseState.position);
> +    if (!mPosition) {

Ideally, we should not new Float32Array frequently. As we discussed at Comment 25, I refer the code from VRPose https://dxr.mozilla.org/mozilla-central/rev/3f4c3a3cabaf94958834d3a8935adfb4a887942d/dom/vr/VRDisplay.cpp#328 but the pointer is not null.

qdot, do you have any suggestion here?
Ok, so I'm trying to understand the code here and how GamepadPose/VRPose relate. It looks like VRPoses are new objects created as needed but expected to be one-offs per VRPose state (not updating internally), while GamepadPose is actually stored on the Gamepad for the lifetime of the Gamepad object. Is my assumption correct there?
Assignee

Comment 32

3 years ago
(In reply to Kyle Machulis [:qdot] from comment #31)
> Ok, so I'm trying to understand the code here and how GamepadPose/VRPose
> relate. It looks like VRPoses are new objects created as needed but expected
> to be one-offs per VRPose state (not updating internally), while GamepadPose
> is actually stored on the Gamepad for the lifetime of the Gamepad object. Is
> my assumption correct there?

I think you are right. VRPose would be a new object while someone asks for it. Like https://dxr.mozilla.org/mozilla-central/rev/1561c917ee27c3ea04bd69467e5b8c7c08102f2a/dom/webidl/VRDisplay.webidl#220

In other case of VRPose in VRFrameData, it would be asked by getFrameData(VRFrameData frameData), and its life cycle is also short.
Well, since we're expecting gamepads to stick around, lazy creation may not be as big of a deal. Don't know if that would help your CC issues or not though.

Also, it looks like GamepadPose and VRPose are mostly the same code. Would there be a way to abstract that back to some sort of Generic Pose interface then just derive both from that?
Assignee

Comment 34

3 years ago
(In reply to Kyle Machulis [:qdot] from comment #33)
> Well, since we're expecting gamepads to stick around, lazy creation may not
> be as big of a deal. Don't know if that would help your CC issues or not
> though.
> 
> Also, it looks like GamepadPose and VRPose are mostly the same code. Would
> there be a way to abstract that back to some sort of Generic Pose interface
> then just derive both from that?

Indeed, for the case of Gamepad, lazy creation is not so important. I will abstract VRPose and GamepadPose to Pose interface for reusing the duplicate code.

Thanks.
Unmarked myself for review until next version is up.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 39

3 years ago
In this update, I make both VRPose and GamepadPose derive Pose interface.

Try looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce963276d25c, but need to check browser_TelemetryGC.js test fail if it is relative to my patches.
Comment on attachment 8803812 [details]
Bug 1299932 - Part 1: Implement gamepadPose attribute in Gamepad API;

https://reviewboard.mozilla.org/r/87966/#review90880

::: dom/gamepad/GamepadPose.cpp:58
(Diff revision 5)
> +void
> +GamepadPose::GetPosition(JSContext* aJSContext,
> +                         JS::MutableHandle<JSObject*> aRetval,
> +                         ErrorResult& aRv)
> +{
> +  if (mPoseState.flags & GamepadCapabilityFlags::Cap_Position) {

There's still similar code here between GamepadPose and VRPose. Outside of the boolean check for whether to create or return, everything is the same. Why not move the similar code up into Pose, with an extra bool parameter in the Pose method for whether or not things should be created?
Attachment #8803812 - Flags: review?(kyle) → review-
Whiteboard: [gfx-noted] → [gfx-noted][webvr]
Assignee

Comment 41

3 years ago
(In reply to Kyle Machulis [:qdot] from comment #40)
> Comment on attachment 8803812 [details]
> Bug 1299932 - Part 1: Implement gamepadPose attribute in Gamepad API;
> 
> https://reviewboard.mozilla.org/r/87966/#review90880
> 
> ::: dom/gamepad/GamepadPose.cpp:58
> (Diff revision 5)
> > +void
> > +GamepadPose::GetPosition(JSContext* aJSContext,
> > +                         JS::MutableHandle<JSObject*> aRetval,
> > +                         ErrorResult& aRv)
> > +{
> > +  if (mPoseState.flags & GamepadCapabilityFlags::Cap_Position) {
> 
> There's still similar code here between GamepadPose and VRPose. Outside of
> the boolean check for whether to create or return, everything is the same.
> Why not move the similar code up into Pose, with an extra bool parameter in
> the Pose method for whether or not things should be created?

Thanks for the comment, I think I should make a SetFloat32Array() at Pose class. And in GamepadPose and VRPose, they just check if-else and send a bool to Pose::SetFloat32Array(). If the bool is true, we call Float32Array::Create() for allocating a new array to the member attribute, like mPosition, otherwise, we just set the existing member attribute to the return value.

Sounds like a good plan?
Ok, I think that'll work.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 46

3 years ago
In this update, I follow the discussion at Comment 41 to move Float32Array creation to Pose class and do some rebase work. Try looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7bd33539b1f1ed7b1a4825313f71697cc635eff.
Assignee

Comment 47

3 years ago
mozreview-review-reply
Comment on attachment 8803812 [details]
Bug 1299932 - Part 1: Implement gamepadPose attribute in Gamepad API;

https://reviewboard.mozilla.org/r/87966/#review90880

> There's still similar code here between GamepadPose and VRPose. Outside of the boolean check for whether to create or return, everything is the same. Why not move the similar code up into Pose, with an extra bool parameter in the Pose method for whether or not things should be created?

I have put the Float32Array creation to Pose class.
Comment on attachment 8803812 [details]
Bug 1299932 - Part 1: Implement gamepadPose attribute in Gamepad API;

https://reviewboard.mozilla.org/r/87966/#review91654

This is super close! One more fix and I think we should be ready to go.

::: dom/gamepad/GamepadPose.cpp:60
(Diff revision 6)
> +                         JS::MutableHandle<JSObject*> aRetval,
> +                         ErrorResult& aRv)
> +{
> +  if (mPoseState.flags & GamepadCapabilityFlags::Cap_Position) {
> +    SetFloat32Array(aJSContext, aRetval, mPosition,
> +                    mPoseState.position, 3, true, aRv);

Ok, I think I didn't get what you meant by the if/else when you mentioned this earlier, and that logic seems extraneous. Why not just get rid of the if/else and make this  

SetFloat32Array(aJSContext, aRetval, mPosition,
mPoseState.position, 3, 
(mPoseState.flags & GamepadCapabilityFlags::Cap_Position)
, aRv);

Seems like you could do that for the functions below and in VRPose also.
Attachment #8803812 - Flags: review?(kyle) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 52

3 years ago
mozreview-review-reply
Comment on attachment 8803812 [details]
Bug 1299932 - Part 1: Implement gamepadPose attribute in Gamepad API;

https://reviewboard.mozilla.org/r/87966/#review91654

> Ok, I think I didn't get what you meant by the if/else when you mentioned this earlier, and that logic seems extraneous. Why not just get rid of the if/else and make this  
> 
> SetFloat32Array(aJSContext, aRetval, mPosition,
> mPoseState.position, 3, 
> (mPoseState.flags & GamepadCapabilityFlags::Cap_Position)
> , aRv);
> 
> Seems like you could do that for the functions below and in VRPose also.

Good catching!
Assignee

Comment 53

3 years ago
mozreview-review
Comment on attachment 8803812 [details]
Bug 1299932 - Part 1: Implement gamepadPose attribute in Gamepad API;

https://reviewboard.mozilla.org/r/87966/#review91906

::: dom/base/Timeout.h:21
(Diff revision 7)
>  class nsGlobalWindow;
>  class nsIPrincipal;
>  class nsITimeoutHandler;
>  class nsITimer;
> +class nsIEventTarget;
>  

qDot, I have followed your comment to modify my patches. But, we have a new enemy that is --disable-gamepad.

In order to support it, I move Pose.h & Pose.cpp to dom/base, I think it makes more sense. Moreover, I adjust the including and forward-declaration in Timeout.h because after I move Pose.cpp to dom/base/moz.build, it can't understand PopupControlState. So, I need to do some changes.

Hope it would not be too bad...
Assignee

Comment 54

3 years ago
mozreview-review
Comment on attachment 8803812 [details]
Bug 1299932 - Part 1: Implement gamepadPose attribute in Gamepad API;

https://reviewboard.mozilla.org/r/87966/#review91908

::: dom/gamepad/GamepadPoseState.h:36
(Diff revision 7)
> +  Cap_All = (1 << 5) - 1
> +};
> +
> +MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(GamepadCapabilityFlags)
> +
> +struct GamepadPoseState

I also move GamepadPoseState to GamepadPoseState.h from GamepadPose.h to avoid linking some unused files, like GamepadPose.cpp. This is for --disable-gamepad as well. Thank you, --disable-gamepad!
Comment on attachment 8803812 [details]
Bug 1299932 - Part 1: Implement gamepadPose attribute in Gamepad API;

https://reviewboard.mozilla.org/r/87966/#review92036

::: dom/vr/VRDisplay.cpp:286
(Diff revision 7)
>  VRPose::GetPosition(JSContext* aCx,
>                      JS::MutableHandle<JSObject*> aRetval,
>                      ErrorResult& aRv)
>  {
> -  if (!mPosition && mVRState.flags & gfx::VRDisplayCapabilityFlags::Cap_Position) {
> -    // Lazily create the Float32Array
> +  SetFloat32Array(aCx, aRetval, mPosition, mVRState.position, 3,
> +    !mPosition && mVRState.flags & gfx::VRDisplayCapabilityFlags::Cap_Position,

nit: Might bool() cast here and below to make things look like they do in GamepadPose, or else remove that cast in GamepadPose
Attachment #8803812 - Flags: review?(kyle) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 59

3 years ago
mozreview-review-reply
Comment on attachment 8803812 [details]
Bug 1299932 - Part 1: Implement gamepadPose attribute in Gamepad API;

https://reviewboard.mozilla.org/r/87966/#review92036

> nit: Might bool() cast here and below to make things look like they do in GamepadPose, or else remove that cast in GamepadPose

I choose to use bool() cast because it is a TypedEnumBits which does not support implicit conversion.
Try looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=7423f9155f6514ae993112d8b5ac617386727beb
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 60

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/83adb3995ae6
Part 1: Implement gamepadPose attribute in Gamepad API; r=kip,qdot
https://hg.mozilla.org/integration/autoland/rev/a8b7a04fa150
Part 2: Support gamepadPose in GamepadManager; r=kip,lenzak800
https://hg.mozilla.org/integration/autoland/rev/84cb8f1a5b2b
Part 3: Handle OpenVR controller position and orientation; r=kip
Keywords: checkin-needed

Comment 61

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/83adb3995ae6
https://hg.mozilla.org/mozilla-central/rev/a8b7a04fa150
https://hg.mozilla.org/mozilla-central/rev/84cb8f1a5b2b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Why do you need the HeaderFile annotation on GamepadPose?

Why is Pose unconditionally exposed?  Is there a spec draft defining the Pose interface?  If so, why is it not linked from the IDL?  If there is not such a draft, why are we ok taking a common term like that?
Flags: needinfo?(dmu)
Assignee

Comment 63

3 years ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #62)
> Why do you need the HeaderFile annotation on GamepadPose?
> 
Indeed. We don't need this HeaderFile annotation.

> Why is Pose unconditionally exposed?  Is there a spec draft defining the
> Pose interface?  If so, why is it not linked from the IDL?  If there is not
> such a draft, why are we ok taking a common term like that?

Pose is not a spec draft. Its mission is for making some duplicate code from GamepadPose and VRPose can be inherited. I think rename Pose to PoseBase would make more sense. Or, you have better suggestion of the naming of Pose class? Thanks.
Flags: needinfo?(dmu)
> Its mission is for making some duplicate code from GamepadPose and VRPose can be inherited.

Do you want inheritance here, or a mixin?  That is, when GamepadPose and VRPose are standardized, do you expect them to have a common base interface like this, or just to share some commonly named members?  Will there be any APIs that accept an abstract Pose?

> Or, you have better suggestion of the naming of Pose class?

I don't offhand, but this is one reason we have things like intents to implement/ship (which I haven't seen for this bug; please make that happen) and standards discussion: to get more eyes on issues like this.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #64)
> > Its mission is for making some duplicate code from GamepadPose and VRPose can be inherited.
> 
> Do you want inheritance here, or a mixin?  That is, when GamepadPose and
> VRPose are standardized, do you expect them to have a common base interface
> like this, or just to share some commonly named members?  Will there be any
> APIs that accept an abstract Pose?

Thanks for the feedback. Much appreciated.

> > Or, you have better suggestion of the naming of Pose class?
> 
> I don't offhand, but this is one reason we have things like intents to
> implement/ship (which I haven't seen for this bug; please make that happen)
> and standards discussion: to get more eyes on issues like this.

* Back in July 2014, Vlad filed bug 1036600 to originally cover adding WebVR support to Firefox.
* Bug 1218482 was filed in October 2015 to enable by default and ship WebVR, but we have since rewritten the WebVR API spec.
* Back in October 2015, Kip emailed an `Intent to Ship` email to the `dev-platform` mailing list: https://lists.mozilla.org/pipermail/dev-platform/2015-October/012108.html.
* Bug 1256444 is now the bug we are actively tracking to ship WebVR in release, still pref'd off under the `dom.vr.enabled` flag.

We have a W3C WebVR Community Group (https://www.w3.org/community/webvr/), and we (Mozilla, Google, Samsung, Microsoft) are all actively implementing the Editor's Draft (https://w3c.github.io/webvr/), soon to be Candidate Recommendation and eventual W3C Recommendation. We are close to also having a Working Group.

We have several implementers, web developers, and standards folks exchanging communication and coordinating in the issues of the W3C WebVR GitHub repo: https://github.com/w3c/webvr

Any further guidance is greatly appreciated. Thanks for your help.
That's all great, but we're talking about the Pose class, which you said is not covered by any of that stuff...
Assignee

Comment 67

3 years ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #64)
> > Its mission is for making some duplicate code from GamepadPose and VRPose can be inherited.
> 
> Do you want inheritance here, or a mixin?  That is, when GamepadPose and
> VRPose are standardized, do you expect them to have a common base interface
> like this, or just to share some commonly named members?  Will there be any
> APIs that accept an abstract Pose?
> 

It should be a mixin that means no one can initiate it and a user is intended to use it through a (sub)class. I expect GamepadPose and VRPose to share the functionality and members that Pose already has.

There is no API discussing for accepting Pose as an abstract class. We just want to abstract the same code back to some sort of Generic Pose interface then just derive both from that.

> > Or, you have better suggestion of the naming of Pose class?
> 
> I don't offhand, but this is one reason we have things like intents to
> implement/ship (which I haven't seen for this bug; please make that happen)
> and standards discussion: to get more eyes on issues like this.

There is no spec for Pose yet, and I didn't think about making it as a spec.

For GamepadPose spec, we can find it here, https://w3c.github.io/gamepad/extensions.html#gamepadpose-interface.
the discussion, https://github.com/w3c/gamepad/pull/25.

Much appreciated for the feedback.
OK.  So for Pose you can obviously share implementation in our C++ code via a Pose superclass without doing any sharing in IDL.  So the real question is what should be shared across _specs_ here and how.

You can define a Pose or GenericPose mixin like so:

  [NoInterfaceObject]
  interface Pose {
    // members here
  };

and then have your two real pose interfaces do:

  VRPose implements Pose;
  GamepadPose implements Pose;

Or you could do interface inheritance like what was checked in.  In either case, our IDL should match the IDL in the relevant specs, so which one we do is a spec-level question.
Assignee

Comment 69

3 years ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #68)
> OK.  So for Pose you can obviously share implementation in our C++ code via
> a Pose superclass without doing any sharing in IDL.  So the real question is
> what should be shared across _specs_ here and how.
> 
> You can define a Pose or GenericPose mixin like so:
> 
>   [NoInterfaceObject]
>   interface Pose {
>     // members here
>   };
> 
> and then have your two real pose interfaces do:
> 
>   VRPose implements Pose;
>   GamepadPose implements Pose;
> 
> Or you could do interface inheritance like what was checked in.  In either
> case, our IDL should match the IDL in the relevant specs, so which one we do
> is a spec-level question.

I just file Bug 1317706 for the following work. Thanks for the comment.

Updated

3 years ago
Depends on: 1320343

Updated

3 years ago
Depends on: 1320349
Assignee

Updated

3 years ago
Depends on: 1322284
You need to log in before you can comment on or make changes to this bug.