Skip to content

Ensure platform conventions remain consistent for XRReferenceSpaces #1180

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

Merged

Conversation

bialpio
Copy link
Contributor

@bialpio bialpio commented Mar 8, 2021

Now that we have got rid of defining Y axes of reference spaces through
gravity, we should at least specify that conventions of the underlying
platforms should remain consistent for XRReferenceSpaces created within
a single XRSession. Y axis is important since it defines what
"horizontal" and "vertical" means - this will be needed for plane
detection API.


Preview | Diff

Now that we have got rid of defining Y axes of reference spaces through
gravity, we should at least specify that conventions of the underlying
platforms should remain consistent for XRReferenceSpaces created within
a single XRSession. Y axis is important since it defines what
"horizontal" and "vertical" means - this will be needed for plane
detection API.
@bialpio
Copy link
Contributor Author

bialpio commented Mar 8, 2021

@toji, @Manishearth - please take a look & let me know what you think.

Copy link
Contributor

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Makes sense. I suppose a bounded reference space may want to keep Y perpendicular to the floor, but that does seem kinda weird.

@bialpio
Copy link
Contributor Author

bialpio commented Mar 9, 2021

I think it's OK to leave this unspecified & rely on the underlying conventions of the platform (which we don't say for "bounded-floor" spaces - should we?), but if you think we should strengthen this in some way, I can address it in this PR.

index.bs Outdated
@@ -1247,6 +1247,8 @@ An {{XRReferenceSpace}} is most frequently obtained by calling {{XRSession/reque

- Passing a type of <dfn enum-value for="XRReferenceSpaceType">unbounded</dfn> creates an {{XRReferenceSpace}} instance. It represents a tracking space where the user is expected to move freely around their environment, potentially even long distances from their starting point. Tracking in an {{unbounded}} reference space is optimized for stability around the user's current position, and as such the [=native origin=] may drift over time.

Note: it is assumed that the conventions of the underlying platform regarding Y axes of the reference spaces stay consistent across different types of {{XRReferenceSpace}}s. In other words, if an XR system supports multiple reference spaces, their Y axes will be parallel to each other and point in the same direction for the duration of the {{XRSession}} in which they were created. This does not apply to {{XRReferenceSpaceType/"viewer"}} reference space, which does not rely on the conventions of the underlying platform for its orientation.
Copy link
Member

Choose a reason for hiding this comment

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

Looks good! One suggestion we came up with in the editors meeting:

Suggested change
Note: it is assumed that the conventions of the underlying platform regarding Y axes of the reference spaces stay consistent across different types of {{XRReferenceSpace}}s. In other words, if an XR system supports multiple reference spaces, their Y axes will be parallel to each other and point in the same direction for the duration of the {{XRSession}} in which they were created. This does not apply to {{XRReferenceSpaceType/"viewer"}} reference space, which does not rely on the conventions of the underlying platform for its orientation.
Note: it is assumed that the conventions of the underlying platform regarding Y axes of the reference spaces stay consistent across different types of {{XRReferenceSpace}}s. In other words, if an XR system supports multiple reference spaces, their Y axes will be parallel to each other and point in the same direction for the duration of the {{XRSession}} in which they were created. This does not apply to {{XRReferenceSpaceType/"viewer"}}, which does not rely on the conventions of the underlying platform for its orientation. {{XRReferenceSpaceType/"unbounded"}} reference spaces should align their Y axes with other reference spaces when their origins are nearby, but may deviate if the user moves over large distances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One question: all other reference spaces become practically non-functional in case the user moves far away from them (the poses must be limited algorithm ensures the poses won't be returned), so is this addition going to affect anything that is observable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, imagine if I set up an unbounded reference space while at home, and then walk a mile. The local reference spaces will have y axes that are offset from mine by approximately a minute (1/60th of a degree)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, but would that be observable through anything we expose on the API? Querying frame.getPose(local, unbounded) will give you null in that case, so any deviation between Y axes is not obtainable by the app and we do not really need to specify what happens in this case. & then when local space readjusts itself (via space reset event) to be closer to unbounded space, and any offset between Y axes of those 2 spaces disappears. I guess this addition is technically correct & it's not going to break anything, I'm just worried that it makes me go back to square one when trying to define what a "horizontal/vertical plane" is w/o also specifying a reference space through which those terms are defined.

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 think I may be able to either lean on "conventions of the underlying platform" and shift the responsibility to the platform to say what it thinks is horizontal/vertical. Alternatively, maybe I could go with sth along the lines of "horizontal plane is a plane whose normal is perpendicular to XZ plane of any reference space except 'local'", and since plane normal is obtainable via a call to getPose(), I may not need to worry about reference spaces deviating from each other across large distances. This may be workable...

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 approving the suggestion and keeping this conversation open, if you decide that the suggested addition is not needed after all, we can revert the commit. Otherwise, I'm OK to merge the PR so please merge it when you have a moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, should rely on conventions of the underlying platform.

I think we can define these things in terms of local, yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, I think we can either merge the PR as-is, or close it w/o merging, as I may not be able to lean on this note after all due to the reference space drift. I'll leave the decision up to you & @toji.

Additionally, I made a mistake, in the "(...) perpendicular to XZ plane of any reference space except 'local' 'viewer'".

Co-authored-by: Brandon Jones <[email protected]>
Copy link
Contributor

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Yeah this seems fine to me

@Manishearth Manishearth merged commit 39aad3f into immersive-web:main Mar 10, 2021
@toji
Copy link
Member

toji commented Mar 10, 2021

Post-merge LGTM here too. Thanks for taking the time to clarify this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants