Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly specify how the views array is populated #614

Merged
merged 2 commits into from
May 1, 2019

Conversation

Manishearth
Copy link
Contributor

@Manishearth Manishearth commented Apr 27, 2019

Moved from #610 which got accidentally closed due to a branch being deleted

fixes #565

r? @NellWaliczek

Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good! Left a few minor comments that I would appreciate if you could address, then I'm happy to merge it.

Reading through this does highlight to me that a structural change to the spec that @NellWaliczek previously suggested could help this read better. I'm happy to address that after this lands, though, since it would mostly involve moving text around rather than changing any verbiage you've got here.

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@Manishearth
Copy link
Contributor Author

Updated!

(I'm okay with re-splitting out the initialize algorithm, or splitting out just the loop part if y'all want it)

Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll give Nell a chance to approve prior to merging, but it looks good to me.

Copy link
Member

@NellWaliczek NellWaliczek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just nits left. I'm approving under the assumption that the nits are addressed :)

index.bs Outdated
@@ -389,6 +389,8 @@ enum XREnvironmentBlendMode {

Each {{XRSession}} has a <dfn for="XRSession">mode</dfn>, which is one of the values of {{XRSessionMode}}.

The {{XRSession/viewerSpace}} on an {{XRSession}} has a <dfn for="XRSession/viewerSpace">list of views</dfn>, which is a [=/list=] of [=view=]s corresponding to the views provided by the underlying device.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

underlying device

Don't we have a variable that defines this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

1. Initialize |xrview|'s {{XRView/eye}} to |view|'s [=view/eye=]
1. Initialize |xrview|'s {{XRView/projectionMatrix}} to |view|'s [=view/projection matrix=]
1. Let |offset| be an {{XRRigidTransform}} equal to the [=view offset=] of |view|
1. Set |xrview|'s {{XRView/transform}} property to the result of [=multiply transforms|multiplying=] the |offset| transform by the {{XRViewerPose}}'s {{XRPose/transform}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone not me should confirm this shouldn't be an inverse...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@toji or @klausw ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna go out on a limb here and say that I'm pretty sure that this shouldn't be an inverse (unlike the handling of the bounds geometry, which did need to be.)

Gonna merge this now, and accept public shaming from @klausw if I'm wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm more concerned about the multiplication order than whether it should be an inverse, but I'm reasonably sure about both.

index.bs Show resolved Hide resolved
@toji toji added this to the April 2019 milestone May 1, 2019
@toji toji merged commit 1010984 into immersive-web:master May 1, 2019
@Manishearth Manishearth deleted the views branch May 1, 2019 00:22
@AdaRoseCannon AdaRoseCannon added the ed:spec Include in newsletter, spec change label May 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ed:spec Include in newsletter, spec change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getViewerPose()s use of XRSpace doesn't quite specify how views work
4 participants