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

Added support for angular and linear velocity #1182

Merged
merged 3 commits into from
Mar 14, 2021
Merged

Added support for angular and linear velocity #1182

merged 3 commits into from
Mar 14, 2021

Conversation

cabanier
Copy link
Member

@cabanier cabanier commented Mar 12, 2021

Closes #619

Also silences a couple of bikeshed warnings.


Preview | Diff

@cabanier
Copy link
Member Author

@Manishearth why can I not add reviewers?

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 after one correction.

index.bs Outdated Show resolved Hide resolved
@toji
Copy link
Member

toji commented Mar 13, 2021

GitHub had a hangover this morning, which is why you couldn't add reviewers. I think it's generally better now.

@cabanier cabanier requested a review from toji March 13, 2021 04:42
@cabanier
Copy link
Member Author

GitHub had a hangover this morning, which is why you couldn't add reviewers. I think it's generally better now.

I still can't add reviewers.

@cabanier
Copy link
Member Author

I was looking into a possible implementation and there are some spaces where velocity will never be reported.
For instance XRJointSpaces and XRTargetRaySpace with a XRTargetRayMode different than POINTING.

Should we add those exceptions to the spec, or leave it up to the UA?

@toji
Copy link
Member

toji commented Mar 13, 2021

I think it's best to leave that up to the UA. For example, I can see that OpenXR has an extension for hand joint velocity and I suspect that the tracking method (ie: optical vs sensors) will make a big difference in what velocities are available. I wouldn't want to prematurely prevent future advances in tracking.

If we're concerned about developers needing to know what spaces can and can't report velocities, it would probably be best to report that as a property of the space (hasLinearVelocity/hasAngularVelocity booleans, maybe?) but I don't know that we can do that consistently and reliably on all platforms, and I believe even spaces that CAN report velocities may have moments where it lapses, such after the first frame or two of tracking (depending on the tracking method). As a result, developers will need to be able to handle null velocities no matter what.

We should probably just leave it as "If the user agent can't populate this, it's allowed to return null."

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 for fixing the angular velocity definition, by the way!

@cabanier
Copy link
Member Author

@toji thanks for approving! Can you merge it for me? I'm not able to.

@toji
Copy link
Member

toji commented Mar 14, 2021

If you don't mind I'd like to make sure that @Manishearth has had a chance to look before I do? That should be Tues at the latest, since we have our editors sync then. Normally I wouldn't be too concerned but given this is a change to a largely "shipped" spec I feel a smidge of extra caution is warranted.

I doubt there's going to be any concerns, though.

@cabanier
Copy link
Member Author

I see. That sounds good! There's no hurry to get it in.

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.

Seems good now.

One note: angular velocity can be obtained from the other attributes; and we tend towards not having APIs that expose information that can be calculated by the author, but I think it's fine in this case.

@Manishearth Manishearth merged commit 10a4f07 into immersive-web:main Mar 14, 2021
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.

Add Linear/Angular Acceleration/Velocity for XRInputSource
3 participants