Closed Bug 1306415 Opened 4 years ago Closed 4 years ago

[webvr] Implement VRFrameData and VRDisplay.getFrameData

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: kip, Assigned: kip)

References

()

Details

Attachments

(2 files, 2 obsolete files)

The WebVR 1.1 Spec added a new interface, VRFrameData:

https://w3c.github.io/webvr/#interface-vrframedata

This is used with the new VRDisplay.getFrameData function:

https://w3c.github.io/webvr/#dom-vrdisplay-getframedata
Depends on: 1306486
Blocks: 1306486
No longer depends on: 1306486
The patches in this bug are rebased onto Bug 1306427.  Please land Bug 1306427 first.
Depends on: 1306427
Patches rebased onto Bug 1306429 as well.  Please land Bug 1306427 and Bug 1306429 first.
Depends on: 1306429
Attachment #8798590 - Flags: review?(bugs)
Comment on attachment 8798589 [details] [diff] [review]
Bug 1306415 - Part 1:  Implement VRFrameData and VRDisplay.getFrameData

@Smaug: Would you be able to review the webidl / dom bits of this one?

Perhaps @Daosheng could take a look at the rest of the implementation.
Attachment #8798589 - Flags: review?(daoshengmu)
Attachment #8798589 - Flags: review?(bugs)
Sorry I could not upload to mozreview.  These patches are rebased onto others that have not yet landed.
Comment on attachment 8798589 [details] [diff] [review]
Bug 1306415 - Part 1:  Implement VRFrameData and VRDisplay.getFrameData

>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(VRFrameData)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mParent, mPose)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
>+  tmp->mPose = nullptr;
You already unlink mPose above

>+VRFrameData::LazyCreateMatrix(JS::Heap<JSObject*>& aArray, gfx::Matrix4x4& aMat, JSContext* aCx,
>+                 JS::MutableHandle<JSObject*> aRetval, ErrorResult& aRv)
align the latter arguments under the arguments in the first line
Attachment #8798589 - Flags: review?(bugs) → review+
Attachment #8798590 - Flags: review?(bugs) → review+
Updated with review feedback from Smaug
Attachment #8798589 - Attachment is obsolete: true
Attachment #8798589 - Flags: review?(daoshengmu)
Attachment #8798590 - Flags: review?(daoshengmu)
Attachment #8798590 - Flags: review?(daoshengmu)
Attachment #8799016 - Flags: review?(dmu)
Comment on attachment 8799016 [details] [diff] [review]
Bug 1306415 - Part 1:  Implement VRFrameData and VRDisplay.getFrameData, r=smaug

Review of attachment 8799016 [details] [diff] [review]:
-----------------------------------------------------------------

It looks good to me. Just some nits for fixing.

::: dom/vr/VRDisplay.cpp
@@ +827,5 @@
> +  matHead.SetRotationFromQuaternion(qt);
> +  matHead.PreTranslate(pos);
> +
> +  mLeftView = matHead;
> +  mLeftView.PostTranslate(

Probably, you can try to use mLeftView.PostTranslate(-aInfo.mEyeTranslation[gfx::VRDisplayInfo::Eye_Left]) to make it more concise.

@@ +839,5 @@
> +    -aInfo.mEyeTranslation[gfx::VRDisplayInfo::Eye_Right][0],
> +    -aInfo.mEyeTranslation[gfx::VRDisplayInfo::Eye_Right][1],
> +    -aInfo.mEyeTranslation[gfx::VRDisplayInfo::Eye_Right][2]
> +  );
> +

Same here. could you use  mRightView.PostTranslate(-aInfo.mEyeTranslation[gfx::VRDisplayInfo::Eye_Right])?
Attachment #8799016 - Flags: review?(dmu) → review+
(In reply to Daosheng Mu[:daoshengmu] from comment #11)
> Comment on attachment 8799016 [details] [diff] [review]
> Bug 1306415 - Part 1:  Implement VRFrameData and VRDisplay.getFrameData,
> r=smaug
> 
> Review of attachment 8799016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks good to me. Just some nits for fixing.
> 
> ::: dom/vr/VRDisplay.cpp
> @@ +827,5 @@
> > +  matHead.SetRotationFromQuaternion(qt);
> > +  matHead.PreTranslate(pos);
> > +
> > +  mLeftView = matHead;
> > +  mLeftView.PostTranslate(
> 
> Probably, you can try to use
> mLeftView.PostTranslate(-aInfo.mEyeTranslation[gfx::VRDisplayInfo::
> Eye_Left]) to make it more concise.
> 
> @@ +839,5 @@
> > +    -aInfo.mEyeTranslation[gfx::VRDisplayInfo::Eye_Right][0],
> > +    -aInfo.mEyeTranslation[gfx::VRDisplayInfo::Eye_Right][1],
> > +    -aInfo.mEyeTranslation[gfx::VRDisplayInfo::Eye_Right][2]
> > +  );
> > +
> 
> Same here. could you use 
> mRightView.PostTranslate(-aInfo.mEyeTranslation[gfx::VRDisplayInfo::
> Eye_Right])?

Good catch, thanks!  I have corrected this.
https://hg.mozilla.org/mozilla-central/rev/d3bda4876ada
https://hg.mozilla.org/mozilla-central/rev/96db3a1d6e66
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1317258
You need to log in before you can comment on or make changes to this bug.