Skip to content

Various changes around null and emulated poses #1058

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
merged 6 commits into from
May 29, 2020

Conversation

Manishearth
Copy link
Contributor

@Manishearth Manishearth commented May 22, 2020

This contains changes to:

For users, the main change here is that requestReferenceSpace() might have some delay involved in resolving the promise, but on the plus side getViewerPose() will never be null.


Preview | Diff

@Manishearth Manishearth requested a review from toji May 22, 2020 22:03
@Manishearth
Copy link
Contributor Author

/agenda to discuss the changes being made here

@Manishearth
Copy link
Contributor Author

Manishearth commented May 26, 2020

In the call today there seemed to be general consensus around allowing getPose() to be null for arbitrary spaces.

It was pointed out that for handheld input controllers we may wish to encourage them to never return null when the input source is active and instead trigger oninputsourceschange events. (Worth noting, you can still hold on to a reference to an old input source's space, and that should definitely return null in getPose()). I could potentially add language to that effect somewhere, though I'd rather wait for @toji to get in an initial review.

The point about getViewerPose() did not get discussed as much, however this PR fixes the main issues that caused contention in #674, so it feels like that change is fine.

@alcooper91
Copy link
Contributor

I thought in a previous call the resolution for #674 that was discussed was to not enforce any particular behavior and to leave that to be a UA choice? (Relevant Minutes: https://www.w3.org/2020/05/12-immersive-web-minutes.html)

@Manishearth
Copy link
Contributor Author

Hmm, you're right. However, I think neither I nor @toji had a strong opinion on that, and my opinion has shifted towards Alex's original one of "getViewerPose() should never be null" now that getPose() is much more free to be null. The reason I felt that getViewerPose() should be allowed to be null was really that I felt that "populate a pose" should be null in more situations so that getPose() can be null.

cc @thetuvix

If you or others feel strongly I can revert that bit.

@toji
Copy link
Member

toji commented May 28, 2020

Sorry it's taken me a while to comment on this. Wanted to hear the call discussion and then come back. I'm OK with everything here with the exception of blocking requestReferenceSpace() resolves so that getViewerPose() is never null. (That said, I do like the forced emulation bit once tracking have been established.)

My biggest concern is that a very common pattern in many apps is to wait for a reference space before spinning up the rAF loop, but in many cases the device will have already switched over to presenting the session's content as soon as the session is created. If there's a need for some action on the user's part to help establish tracking (as is commonly the case for mobile AR, where you want to wiggle your phone around) then having that rAF loop available as a point to ping the tracking state is useful, as well as offering a logical place to draw any helper dialogs with a more reliable (viewer?) space.

@Manishearth
Copy link
Contributor Author

Oh, that's a fair point. I'll remove those changes and fix up the rest of the PR. You should leave a comment to that effect on the other issue, and perhaps we should reagenda 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.

LGTM. Thanks!

@Manishearth
Copy link
Contributor Author

@toji oh, I also added a note about hand controllers losing tracking, as discussed in the call. want to quickly check that as well?

@toji
Copy link
Member

toji commented May 29, 2020

Controller comment LGTM too!

@Manishearth
Copy link
Contributor Author

Going to merge, the only part of this PR that was not discussed fully in the call was the one I removed.

@Manishearth Manishearth merged commit 403f2e5 into immersive-web:master May 29, 2020
@Yonet Yonet added agenda Request discussion in the next telecon/FTF and removed agenda Request discussion in the next telecon/FTF labels Jun 1, 2020
@AdaRoseCannon AdaRoseCannon removed the agenda Request discussion in the next telecon/FTF label Jun 8, 2020
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.

Allow poses to be null in more situations
5 participants