Multiple fixes to XR compatibility algorithms #921
Conversation
|
@toji Is there a draft version of the spec with these changes merged in where I could see them in context? |
|
There is not (and unfortunately it's not as trivial to do so as it should be), but I can fix that. Give me a moment. |
| <dd> Set |context|'s [=XR compatible=] boolean to <code>true</code> and [=/resolve=] |promise|. | ||
| <dt> Otherwise | ||
| <dd> [=Queue a task=] to perform the following steps: | ||
| 1. Force |context| to be lost and [=handle the context loss=] as described by the WebGL specification. |
There was a problem hiding this comment.
OK, so #913 is still outstanding, right? That's fine, since this PR is not claiming to fix that.
There was a problem hiding this comment.
That's correct. Trying to keep the reviews manageable in size.
index.bs
Outdated
| <dt> If |context| was created on a [=compatible graphics adapter=] for the [=XR/immersive XR device=] | ||
| <dd> Set |context|'s [=XR compatible=] boolean to <code>true</code> and [=/resolve=] |promise|. | ||
| <dt> Otherwise | ||
| <dd> [=Queue a task=] to perform the following steps: |
There was a problem hiding this comment.
It'd be better to say "Queue a task on the WebGL task source to ...", I think, instead of putting the task source part completely separate from this operation.
Also, this still needs to define the document for this task...
There was a problem hiding this comment.
Ah, interesting. I was primarily mimicking the pattern that I saw repeated throughout the HTML standard. For example:
- https://html.spec.whatwg.org/multipage/canvas.html#dom-canvas-toblob
- https://html.spec.whatwg.org/multipage/interactive-elements.html#details-notification-task-steps
- https://html.spec.whatwg.org/multipage/browsing-the-web.html#scroll-to-fragid:task-source
Happy to change it, though. (Also none of those examples define a document, unless I'm overlooking something, so it wasn't clear what was required in that case.)
There was a problem hiding this comment.
The HTML standard has large parts that predate the "modern" formulation of queueing tasks, and also some sections where lots of tasks get queued and it tries to coalesce the "which task source" verbiage.
In practice, this is really annoying to deal with and makes it hard to find what task source to use for tasks, and I wish the HTML standard would stop doing that.
And yes, most things don't documents at the moment, but that's being worked on. See whatwg/html#4980
There was a problem hiding this comment.
Thanks for the explanation! If you don't mind providing a bit more guidance, I've got a followup question.
In this specific case it seems like the appropriate document is the document which created the canvas element which created the WebGL context, unless the context was created by an OffscreenCanvas, in which case I'm not sure how to reason about what the appropriate document is? (It may have been created in a worker.)
There was a problem hiding this comment.
Right, if we have an element it's easy. If we don't, then I don't know enough about the relevant APIs to tell you. What does Chrome implement, since they actually do document-association of tasks?
I guess one option would be to grab the relevant global of the webgl context (which will match the relevant global of the OffscreenCanvas or the canvas element) and if that global is a Window global use that window's document. That doesn't match the behavior of using the canvas element's document if that element is adopted across documents, of course, which is why the exact behavior needs to be specified...
2aa51c7 to
f057ad1
Compare
|
I'm going to merge this now since it clearly gets us to a better place with regards to the issues @bzbarsky has raised, with the intent to follow up with the remaining issues (including the declaration of the task document) in a future PR. |
<
9296
a href="/kearwood/webxr/commit/6eec55bc05e5899c0857b5f375cba646fffe956f" class="Link--secondary">6eec55b
* Make getContext consistent with makeXRCompatible /fixes immersive-web#910 by ensuring that the logic that the getContext algorithm follows more closely matches the logic and results of the makeXRCompatible algorithm. * Remove unnecessary parallel section /fixes immersive-web#911 /fixes immersive-web#908 Removes the requirement that a part of the makeXRCompatible algorithm be performed in parallel, which both simplifies the logic and solves some concurrency issues with setting the XR compatibile boolean. * Specify the task source for makeXRCompatible task /fixes immersive-web#912 We'll also want to do a larger review of the spec to determine the task sources for any other tasks that we're queueing.
Based on feedback from @bzbarsky, this pull request contains fixes for the following issues:
/fixes #910
Ensures that the logic that the getContext algorithm follows more closely
the logic and results of the makeXRCompatible algorithm.
/fixes #911
/fixes #908
Removes the requirement that a part of the makeXRCompatible algorithm
be performed in parallel, which both simplifies the logic and
solves some concurrency issues with setting the XR compatibile boolean.
/fixes #912
Specifies the task source for makeXRCompatible task.
We'll also want to do a larger review of the spec to determine the task
sources for any other tasks that we're queueing.
In addition to the usual editors cross review, I'd especially appreciate @bzbarsky's feedback and believe that @foolip may also want to take a look.