-
Notifications
You must be signed in to change notification settings - Fork 200
Add WebXR #1744
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
Add WebXR #1744
Conversation
|
Thanks for the PR. I won't review as I know nothing about WebXR. Do you know who is a subject matter expert and could help us review this? This would ideally be someone who not only knows about WebXR, but someone who actually uses it as a web dev. |
|
I documented WebXR on MDN, I proposed to split this into modules, and I will review this :) |
Elchi3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review. Many dist files weren't generated due to using yaml (not yml), I filed #1768 for that. Also, I don't think we need compute_from everywhere.
Made a first pass to provide descriptions. Happy to write the remaining as well.
Co-authored-by: Florian Scholz <fs@florianscholz.com>
Co-authored-by: Florian Scholz <fs@florianscholz.com>
Co-authored-by: Florian Scholz <fs@florianscholz.com>
Co-authored-by: Florian Scholz <fs@florianscholz.com>
Elchi3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this from a feature composition point of view. Would probably be good if someone reviews my descriptions, though.
ddbeck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As requested, description (and name) reviews. This is a partial review, through depth sensing (I have to step out and this is a big PR).
I'm sensing some themes here, if you'd like to get ahead of my follow up though. Thank you!
features/webxr-camera.yml
Outdated
| @@ -0,0 +1,10 @@ | |||
| name: WebXR Raw Camera Access | |||
| description: Direct access to the camera allowing pose-synchronized camera images within a WebXR session. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a sentence—an active verb would really help here. What does this thing do? I suspect it's something like…
The [mumble mumble] interface reads from …
But I have no idea what goes here and there's no XRCamera docs to look at.
ddbeck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More suggestions, finishing the rest of the features. Thank you!
Co-authored-by: Daniel D. Beck <daniel@ddbeck.com>
Co-authored-by: Daniel D. Beck <daniel@ddbeck.com>
Co-authored-by: Daniel D. Beck <daniel@ddbeck.com>
Co-authored-by: Daniel D. Beck <daniel@ddbeck.com>
Co-authored-by: Daniel D. Beck <daniel@ddbeck.com>
Co-authored-by: Daniel D. Beck <daniel@ddbeck.com>
Co-authored-by: Daniel D. Beck <daniel@ddbeck.com>
Co-authored-by: Daniel D. Beck <daniel@ddbeck.com>
Co-authored-by: Daniel D. Beck <daniel@ddbeck.com>
Co-authored-by: Daniel D. Beck <daniel@ddbeck.com>
|
One more description to figure out. Also, @queengooborg, can you re-run dist generation for this PR to pass? TY! |
Elchi3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is now ready to land.
ddbeck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Nice to land a big chunk of coverage here.
This PR adds the WebXR feature set as a Baseline feature.