-
Notifications
You must be signed in to change notification settings - Fork 413
add support for XRVisibilityMaskChangeEvent #1412
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
Conversation
index.bs
Outdated
| constructor(DOMString type, XRVisibilityMaskChangeEventInit eventInitDict); | ||
| [SameObject] readonly attribute XRSession session; | ||
| readonly attribute XREye eye; | ||
| readonly attribute FrozenArray<DOMPointReadOnly>? polygon; |
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-is this would generally be expected to fire once per view, right? I'm wondering if it would be beneficial to developers to provide the mask for all views in a single event, so that if they need to do things like buffer allocation they have all the information needed in once shot.
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.
OpenXR fires an event for each eye and I think it's possible that a change applies to only one eye (ie if only the left eye is cast).
I could combine all masks but would still fire them for each mask change event.
|
|
||
| NOTE: situations where {{XRReferenceSpaceEvent/referenceSpace}} or {{XRReferenceSpaceEventInit/referenceSpace}} can be when the headset was doffed and donned between 2 seperate locations. In such cases, if the experience relies on world-locked content, it should warn the user and reset the scene. | ||
|
|
||
| XRVisibilityMaskChangeEvent {#xrvisibilitymaskchangeevent-interface} |
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 discussed in the other thread, we must have code in `requestSession() (step 5.4.11) that guarantees that this event is fired on session creation. Otherwise there is no way to get the initial mask.
Also, if we aren't going to have an attribute for this I would like docs to prominently state that this is the behavior everywhere.
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.
The mask is not known at session creation (at least on Quest) so we can't fire that event.
Event the event for controllers is only fired if the controllers were detected during startup.
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.
Perhaps what is needed then is at least a note to indicate that the events will always fire after the session request is resolved, so users that want to ensure that they get the event should always listen for it in the resolve callback.
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.
|
|
||
| The <dfn attribute for="XRVisibilityMaskChangeEvent">polygon</dfn> attribute is an optional [=/list=] of {{DOMPointReadOnly}} vertices that encompass the region of the eye's {{XRView}} that SHOULD be drawn. If this attribute is `null`, the whole region of the {{XRView}} SHOULD be drawn. The <code>Z</code> coordinate of each {{DOMPointReadOnly}} vertex in {{XRVisibilityMaskChangeEvent/polygon}} MUST be <code>-1</code>. | ||
|
|
||
| The area MUST be drawn using the {{XRViewGeometry/projectionMatrix}} of the {{XRView}} of {{XRVisibilityMaskChangeEvent/eye}} and a default {{XRRigidTransform}}. |
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.
Not a comment on this proposal, just a general observation: I didn't realize that OpenXR's visibility mask required transformation by the projection matrix! I would have thought that reporting the points in NDC would be easier all around. Wonder why they went that route.
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.
Maybe this made it easier to integrate into rendering pipelines? Maybe @thetuvix knows since he's listed as one of the authors of this extension.
toji
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.
A few comments, but broadly optimistic about the direction this is going!
| constructor(DOMString type, XRVisibilityMaskChangeEventInit eventInitDict); | ||
| [SameObject] readonly attribute XRSession session; | ||
| readonly attribute XREye eye; | ||
| readonly attribute unsigned long index; |
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.
Is the intent that the eye and the index together make a unique key, or is the index alone enough to uniquely identify the view? The added prose above suggested that the index is only unique per eye.
If so, I'm not really clear on what the benefit is to doing it that way? Seems like simply using a unique index/ID would be easier for devs using it to cache things like meshes.
Alternatively, maybe I'm just reading it wrong and there's a benefit to having the eye explicitly given here that I'm not aware of? (Especially since you wouldn't be able to get the eye otherwise till a ViewPose was queried.)
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.
Yes, I made it unique per eye. I can change it to drop that so we match OpenXR.
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 updated the PR. Please take another look
|
|
||
| [SecureContext, Exposed=Window] interface XRView { | ||
| readonly attribute XREye eye; | ||
| readonly attribute unsigned long index; |
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.
index suggests to me that this would be analogous to the index of the XRViewerPose.views[] array that this view is located at, and even if that wasn't explicit I can see implementations naturally having them line up anyway, giving the impression that they're the same.
Feels like if this is guaranteed to always align with the array index then we should let it be implicit. If it's not intended to align with the array index then we should call it something like viewId and potentially state that they're intended to be unique across the entire session. Maybe even enforce that they can't have an ID of 0 so that there's no ambiguity about the index alignment.
(Also, if what I was wondering about below is true and it's a per-eye index, the name should reflect that. eyeViewIndex or similar.)
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.
Having it match the index in the views array sgtm.
I'll update the spec.
c948a91 to
09a5e34
Compare
|
Current version LGTM! Thanks for iterating on it. PR is in a good spot to present to the rest of the WG as see if anyone else has feedback. |
|
/agenda to show to WG (reposting after bot fix) |
|
PR was discussed during bi-weekly group meeting and there were no objections. |
SHA: f9c8adf Reason: push, by cabanier Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
SHA: f9c8adf Reason: push, by cabanier Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This introduces an event that is fired when the visibility mask changes. See https://registry.khronos.org/OpenXR/specs/1.0/html/xrspec.html#XR_KHR_visibility_mask for the OpenXR equivalent.
Preview | Diff