-
Notifications
You must be signed in to change notification settings - Fork 387
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
Detail magic window support #237
Conversation
Updated this pull request after some conversations with @NellWaliczek to drop the concept of a "Tracking only" session for the moment. While I'd like to discuss that further in the future it's the least important part of this proposal and I don't care for it to complicate things. Very keen to get feedback, since I consider this to be one of the last big issues to work out prior to TAG review and starting on the spec proper. |
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 core concepts seem pretty solid at first glance,
There's a few things I'd like to look into a bit more, but I think we're trending in a good direction.
// VRDevice.requestSession must be called within a user gesture event | ||
// like click or touch when requesting exclusive access. | ||
vrDevice.requestSession({ exclusive: isExclusive }) |
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.
It seems a bit odd to pass the exclusive entry explicitly in the supportsSession() sample code and then skip it here. Was there a specific reason for that?
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.
Agreed that it's a bit confusing, reverting for now. I also think that we need to push a change to supportsSession
soon, as you've been suggesting, and perhaps at that point it'll be easier to demonstrate effectively.
explainer.md
Outdated
@@ -123,7 +123,7 @@ function BeginVRSession(isExclusive) { | |||
``` | |||
Once the session has started, some setup must be done to prepare for rendering. | |||
- A `VRFrameOfReference` must be created to define the coordinate system in which the `VRDevicePose` objects will be defined. See the Advanced Functionality section for more details about frames of reference. | |||
- The depth range of the session should be set to something appropriate for the application. This range will be used in the construction of the projection matricies provided by `VRPresentationFrame`. | |||
- The depth range of the session should be set to something appropriate for the application. This range will be used in the construction of the projection matricies provided by `VRFrame`. |
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 naming change perhaps isn't as relevant with the removal of the non-visual sessions?
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.
Fair point. I'll revert these for now. (I think Input might push them away from the Presentation
verbiage again, though.)
explainer.md
Outdated
@@ -336,6 +325,62 @@ if(frameOfRef.bounds) { | |||
} | |||
``` | |||
|
|||
### Mirroring |
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.
Should this go in the advanced functionality section? On mobile devices, this won't be particularly useful and on Windows Mixed Reality headsets, there is automatic mirroring in the Mixed Reality Portal.
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 in the advanced functionality section, actually. (Not always easy to tell with the sparse diffs)
explainer.md
Outdated
|
||
When mirroring only one eye's content will be shown, and it should be shown undistorted. The UA may choose to crop the image shown, display it at a lower resolution than originally rendered, and the mirror may be multiple frames behind the image shown in the headset. The mirror may include or exclude elements added by the underlying VR system (such as visualizations of room boundaries) at the UA's discretion. Pages should not rely on a particular timing or presentation of mirrored content, it's really just for the benefit of bystanders or demo operators. | ||
|
||
On devices without an external display to mirror to, such as mobile or all-in-one systems, the `outputCanvas` will be ignored. |
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.
Are you operating under the assumption that external display == need to mirror? What if a user agent/OS is already doing its own mirroring (such as the Windows Mixed Reality Portal)? It seems like a better end user experience might come from not mirroring a second time. It might be a bit heavy handed to say that a UA can only ignore the outputCanvas
if it doesn't have an external display.
Of course with the current design the page has no way of distinguishing this situation. We'd be likely to see pages always supplying an outputCanvas and then there might be a big gaping hole in the middle of the page in some UA. Perhaps it's worth indicating the need for mirroring (as a attribute off an object returned by calls to supportsSession?) There is still the risk that developers will ignore it though. Hrmmmm....
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 could probably find a way to highlight this better, but the intent is not so much "external display == need to mirror" but "no external display == pointless to mirror". UA should still have the option to ignore a requested mirror when it's appropriate, even if the system indicates that it has an external display.
explainer.md
Outdated
|
||
These scenarios can make use of non-exclusive sessions to render tracked content to the page. Similar to mirroring, to make use of this mode an `outputCanvas` is provided at session creation time, as well as the `exclusive: false` flag. Just like with mirroring the `outputCanvas` must have a `ImageBitmapRenderingContext`. At that point content rendered to the `VRSession.baseLayer` will be rendered to the `outputCanvas`. In the future, if multiple `VRLayers` are used their composited result will be what is displayed in the `outputCanvas`. Given that the session is tied to a specific output canvas it's permissible to have multiple non-exclusive sessions active at once to handle multiple output surfaces. | ||
|
||
Exclusive and non-exclusive sessions use the same render loop code, but there are some differences in behavior to be aware of. The sessions may run their render loops at at different rates. During exclusive sessions the UA runs the rendering loop at the `VRDevice`'s native refresh rate. During non-exclusive sessions the UA runs the rendering loop at the refresh rate of page (aligned with `window.requestAnimationFrame`.) Also, when rendering with an exclusive session the projection matrices provided each frame are based on the device optics, while non-exclusive sessions provide projection matrices based on the output canvas dimensions. |
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.
That last sentence might be accidentally a bit too restrictive. The "punch-through" scenarios we've discussed also take into account the headpose the same as I imagine zSpace would.
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.
Expanded the language in upcoming commit.
explainer.md
Outdated
|
||
Exclusive and non-exclusive sessions use the same render loop code, but there are some differences in behavior to be aware of. The sessions may run their render loops at at different rates. During exclusive sessions the UA runs the rendering loop at the `VRDevice`'s native refresh rate. During non-exclusive sessions the UA runs the rendering loop at the refresh rate of page (aligned with `window.requestAnimationFrame`.) Also, when rendering with an exclusive session the projection matrices provided each frame are based on the device optics, while non-exclusive sessions provide projection matrices based on the output canvas dimensions. | ||
|
||
Most instances of non-exclusive sessions will only provide a single `VRView` to be rendered, but UA may request multiple views be rendered if, for example, it's detected that that output medium of the page supports stereo rendering. As a result pages should always draw every `VRView` provided by the `VRFrame` regardless of what type of session has been requested. |
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.
An example here might help to clarify a lot :)
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 guess I could, but it would literally be a copy/paste of a previous render loop example with a comment saying: "This is literally a copy/paste of the previous render loop example." ;)
explainer.md
Outdated
|
||
UAs may have different restrictions on non-exclusive contexts that don't apply to exclusive contexts. For instance, a different set of `VRFrameOfReference` types may be available with a non-exclusive session versus an exclusive session. | ||
|
||
The UA may reject requests for a non-exclusive sessions for a variety of reasons, such as the inability of the underlying hardware to provide tracking data without actively rendering to the device. Pages should be designed to robustly handle the inability to aquire non-exclusive sessions. |
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.
Did I miss it, or are we calling out somewhere that no user initiated action is required for non-exclusive sessions?
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.
It's called out in the "Detecting and advertising VR mode" section, but didn't show up in this diff because it was made as part of #236, which I (accidentally) made the upstream for this patch. Hm... We'll have to sort that out.
@@ -578,10 +623,12 @@ interface VRDevice : EventTarget { | |||
|
|||
dictionary VRSessionCreateParametersInit { | |||
boolean exclusive = true; | |||
HTMLCanvasElement outputCanvas = null; |
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.
Out of curiosity, why HTMLCanvasElement rather than the ImageBitmapRenderingContext given the hard requirement listed above?
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.
Primarily because the intent is to create a link to a specific canvas on the page, but I'm flexible on this point. Probably want to address @RafaelCintron's comments about the ImageBitmapRenderingContext
before we make a decision here, though.
Here's what
Calling The problem with using If we introduce spec language that says “transferFromImageBitmap is disabled while we’re mirroring” then I think we’re better off just making our own “mirroring” rendering context type that doesn’t have this function. At that point, we can introduce |
Made several updates based on @NellWaliczek's feedback, haven't yet taken into account @RafaelCintron's comments because I need some more time to consider them. |
This change removes any concepts that assume a device may only have a single active session at a time. This allows multiple non-exclusive VRSessions to be active at once, which will be important for enabling various interesting magic window scenarios in the future. With this change starting an exclusive session suspends the non-exclusive ones, but doesn’t end them. IDL Changes: - Remove VRDevice.activeSession - VRDevice.onsessionchange -> VRSession.onended
Wanted to explicitly state the differences between the two modes and why you’d want to use one or the other.
Some security discussion has been deferred to a follow up PR for the sake of readability.
Following discussion on implementor call, for the moment we want to just remove elements of the explainer that would explicitly block multiple non-exclusive sessions at once (like the activeSession attribute) while not explicitly stating that multiple are support just yet. (Might be a 2.1 feature? We’ll see.)
Described how magic window mode works with the `outputCanvas` proposal. Also, because they’re natural variants of the same mechanism, described mirroring and tracking only behavior. Follows the proposal in #224. Also changed VRPresentationFrame -> VRFrame because if we moved forward with the tracking only portion of this there would be scenarios in which the VRPresentationFrames had no Presentation component.
It’s the least important and yet most contentious part of this proposal AFAICT, so I don’t mind dropping it to help move things along.
Described how magic window mode works with the
outputCanvas
proposal.Also, because they’re natural variants of the same mechanism, described
mirroring and tracking only behavior. Follows the proposal in #224.
Also changed VRPresentationFrame -> VRFrame because if we moved forward
with the tracking only portion of this there would be scenarios in
which the VRPresentationFrames had no Presentation component.