-
Notifications
You must be signed in to change notification settings - Fork 31
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 IDL and more timing details based on discussion in issues #14
Conversation
explainer.md
Outdated
}; | ||
|
||
partial interface XRSession { | ||
Promise<FrozenArray<XRHitResult>> requestHitTest(Float32Array origin, Float32Array direction, XRFrameOfReference frameOfReference); |
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 OK for now, but we'll want to make sure the ray input format here tracks whatever decision is made in immersive-web/webxr#339
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.
Will do - i'll keep tabs on that issue and update as appropriate.
explainer.md
Outdated
|
||
`hitTest` parameters | ||
* origin - the origin of the ray as [x, y, z] | ||
* direction - the direction of the ray as [x, y, z], normalized |
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.
Normalization is a trivial thing to do, and relying on the user to input a normalized direction is going to be error prone. Best case scenario is they'll get a bad result and not understand why, worst case we'll have to check if the direction is normalized and error if not, in which case we might as well just normalize it ourselves. So I would advocate for speccing that we will normalize any non-zero-length direction vectors that are passed to the API. Here or anywhere else in WebXR for that matter.
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.
Done.
explainer.md
Outdated
* origin - the origin of the ray as [x, y, z] | ||
* direction - the direction of the ray as [x, y, z], normalized | ||
* Note: We'll start with an origin-direction pair instead of a pose as a pose overspecifies a ray and has the danger of the developer creating a malformed matrix | ||
* frameOfReference - the frame of reference the ray origin/direction and hit results should be relative to. The call needs to include the frame of reference so the underlying system understands how to perform the calculation. |
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 suggest making this a XRCoordinateSystem instead. The exact structure is still in flux but using XRCoordinateSystem here would signal that you would expect to be able to accept both frames of reference and anchors. (Unless you have a reason for not wanting that, in which case it should probably be noted.)
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.
Done.
Left a few comments on minor points, but overall this looks good to me. Would love to hear input from other vendors. |
explainer.md
Outdated
* direction - the direction of the ray as [x, y, z], normalized | ||
* Note: We'll start with an origin-direction pair instead of a pose as a pose overspecifies a ray and has the danger of the developer creating a malformed matrix | ||
* frameOfReference - the frame of reference the ray origin/direction and hit results should be relative to. The call needs to include the frame of reference so the underlying system understands how to perform the calculation. | ||
* To enable feature detection of possible future versions of the API with additional parameters, an error is thrown if additional arguments are given to the function. |
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.
Very smart idea.
This PR adds a discussion to the explainer about the async promise resolution semantics and establishes IDL for the hit-test API. It also contains some additional discussion of how anchors relate to hit-testing.