Update XRRenderStateInit with layers sequence #999

cabanier · 2020-04-13T22:15:32Z

No description provided.

Manishearth

We should also null out layers when baseLayer is set, below

index.html

Manishearth · 2020-04-13T22:33:17Z

index.bs

@@ -642,6 +642,7 @@ When this method is invoked, the user agent MUST run the following steps:

  1. Let |session| be the target {{XRSession}}.
  1. If |session|'s [=ended=] value is <code>true</code>, throw an {{InvalidStateError}} and abort these steps.
+  1. If |newState|'s {{XRRenderStateInit/layers}}'s value is not <code>null</code>, throw an {{InvalidStateError}} and abort these steps.


I was suggesting that you have something like

1. If |newState|'s {{XRRenderStateInit/layers}}'s value is not <code>null</code>If |newState|'s {{XRRenderStateInit/layers}}'s value is not <code>null</code>, run [=update layer state=] with [=pending render state=]

and put it right before the depthNear line

and in the definition for that algorithm, just throw an InvalidStateError with a note that the layers spec will introduce new semantics.

OK. take a look and let me know what you think.

index.bs

Manishearth

It needs to operate on the init state, it can get the pending state off the session

index.bs

cabanier · 2020-04-13T23:39:09Z

@Manishearth If you think this is good to merge, can you do the PR?

Manishearth · 2020-04-13T23:40:00Z

What PR? This is the PR? :)

But I want to wait for Brandon.

cabanier · 2020-04-13T23:46:22Z

What PR? This is the PR? :)

Sorry, I mistyped :-)

But I want to wait for Brandon.

ok. @toji , let me know what you think. My layers PR is gated on this landing first.

toji · 2020-04-13T23:50:52Z

/agenda to confirm with the WG that this is the pattern we want (need?)

To be honest I'm not thrilled with this pattern. I'm concerned that this type of pattern means that our modules can't stay very modular, and that the core WebXR spec will over time become a pile of unimplemented stubs and empty interfaces. It'll also make it more difficult for UAs to implement the core spec over time because they have to add stubs for a lot of modules they may not intend to support in order to be compliant, and it removes some options for feature detection (if the presence of the interface was going to be the primary indicator, which it's not in this case.)

But... at the same time I don't know if there's a lot that WebIDL let's us do about it, and that's more of a issue on their side than ours. If this is the only way we can structure the WebIDL so that the various UAs can consume it cleanly then so be it.

I'm sympathetic to Rik wanting to land this ASAP, but I'd feel better if we talked about it with the wider working group first.

cabanier · 2020-04-14T00:45:28Z

I agree it's weird to have to update the main spec.
If we can't do it another way, maybe we need a new update call that takes a new dictionary that includes layers.

Hopefully we can discuss it in tomorrow's call.

Manishearth · 2020-04-14T03:49:35Z

I think we still have a benefit to maintaining this separation, as long as we only need a couple hooks we still can stay pretty modular. I think the benefits of maintaining stuff separate have been pretty clear for me so far, I would never have been able to keep up if layers were a part of the same spec (and if it were it would likely be more tightly hooked in)

cabanier · 2020-04-14T05:40:03Z

I think we still have a benefit to maintaining this separation, as long as we only need a couple hooks we still can stay pretty modular. I think the benefits of maintaining stuff separate have been pretty clear for me so far, I would never have been able to keep up if layers were a part of the same spec (and if it were it would likely be more tightly hooked in)

If you were replying to me, that's not what I meant.
What I wanted to say is that maybe the layers spec should define a completely new Init structure (without baselayer and with a sequence of XRLayers/WebGLLayer) and a new API entry on session that takes that init structure.

That way we won't have to make any changes or add hooks to the WebXR spec.

FWIW it's really unfortunate that we don't have a way to extend a dictionary in WebIDL...

Manishearth · 2020-04-14T06:22:32Z

Oh, no, I was replying to Brandon, and I k kw he isn't quite making that point either, he's just suggesting we discuss it in a call, which I wholeheartedly agree with!

I understand what you were suggesting. I would prefer to avoid that API if possible since you still have to patch algorithms to make it work. But yeah it's an option if we can't do this.

Partial dictionaries still don't fix the issue of needing to patch algorithms at certain steps (instead of replacing hooks, which is very straightforward and less fragile)

index.bs

kearwood · 2020-04-16T21:25:42Z

I have pinged some DOM folks at Mozilla that may be opinionated on the pattern here.

kearwood · 2020-04-16T21:34:50Z

One alternate idea... Would it make sense to have a separate function for adding/changing the layers sequence, and provide a separate state dictionary for each layer type? The method for updating the state of a layer could be added to the interface of the layer object itself.

This would allow feature detection by checking for the presence of the insertLayer or such call, as well as enabling each layer to have its own dictionary schema without relying on partial dictionary webidl.

cabanier · 2020-04-16T21:40:02Z

So add a function (eq setLayers(sequence<XRGenericLayer> layers) which is called to update the layers in the renderState of the session?

We would still have to define that you can't set a baseLayer if layers was enabled.

kearwood · 2020-04-16T21:56:51Z

Could we specify that any baseLayer set is rendered first in a painters-algorithm ordering. Inherently, this would result in the baseLayer being obscured by any fully opaque layer added by the WebXR layers spec. The UA could then optimize away the rendering of this baseLayer as it sees fit.

If the baseLayer was set, and the layers sequence included something like a quad layer attached to a video element, I would expect that the site could continue to use its current rendering path for the WebGL based scene and more easily implement the layers module support on top of that. This would still allow a new layer type to fully replace XRWebGLLayer by describing it as "fully opaque" in the Layers spec.

This would allow both the feature detection, while not changing any existing behavior.

Manishearth · 2020-04-16T22:13:36Z

@kearwood we could do this, the interaction with baseLayer gets weird, and ideally we'd have an API where these things stay on renderState. But yes, this is an option available to us.

kearwood · 2020-04-16T22:13:45Z

Without changing the algorithms in the Core spec, it would also effectively limit XRRenderStateInit and XRRenderState to be descriptors of the XRWebGLLayer, specified with baseLayer.

Could we specify that any baseLayer set is rendered first in a painters-algorithm ordering. Inherently, this would result in the baseLayer being obscured by any fully opaque layer added by the WebXR layers spec. The UA could then optimize away the rendering of this baseLayer as it sees fit.

If the baseLayer was set, and the layers sequence included something like a quad layer attached to a video element, I would expect that the site could continue to use its current rendering path for the WebGL based scene and more easily implement the layers module support on top of that. This would still allow a new layer type to fully replace XRWebGLLayer by describing it as "fully opaque" in the Layers spec.

This would allow both the feature detection, while not changing any existing behavior.

cabanier · 2020-04-16T22:15:52Z

From an implementor's standpoint, that would be harder to implement and I suspect it's also more confusing to users.
Moreover, once the layers feature is supported, most sites will use it with projection layers just to have access to more efficient texture array.

kearwood · 2020-04-16T22:16:35Z

@kearwood we could do this, the interaction with baseLayer gets weird, and ideally we'd have an API where these things stay on renderState. But yes, this is an option available to us.

I'm less opinionated about such a solution vs the proposal in the current PR. I'm hoping to get feedback soon from out colleagues more familiar with the current state of WebIDL design patterns.

kearwood · 2020-04-16T23:10:57Z

I have had the opportunity to review this with peers at Mozilla. It seems that there are no concerns about the feature detection with this schema, nor problems with having differing algorithms within updateRenderState when a page has feature detected new functionality and passed in new parameters (eg, XRRenderState.layers).

We would like to avoid adding the XRGenericLayer type if possible.

I would like to propose landing this PR with two small changes:

Make XRWebGLLayer extend XRLayer.
Remove XRGenericLayer

cabanier · 2020-04-16T23:18:28Z

I have had the opportunity to review this with peers at Mozilla. It seems that there are no concerns about the feature detection with this schema, nor problems with having differing algorithms within updateRenderState when a page has feature detected new functionality and passed in new parameters (eg, XRRenderState.layers).

That's great news!

We would like to avoid adding the XRGenericLayer type if possible.

I would like to propose landing this PR with two small changes:

Make XRWebGLLayer extend XRLayer.

XRWebGLLayer currently does not have the attributes that XRLayer has. Should we update the WebXR spec so it does?

cabanier · 2020-04-17T16:19:13Z

@Manishearth how can we get the change proposed by @kearwood in?

Manishearth · 2020-04-17T17:59:59Z

nor problems with having differing algorithms within updateRenderState when a page has feature detected new functionality and passed in new parameters (eg, XRRenderState.layers).

Note that this change to the core spec doesn't affect feature detection since it only affects a dict, and also there is no change of functionality since the layers feature will not be enabled.

(Just wanted to make sure this was clear)

Make XRWebGLLayer extend XRLayer.

XRWebGLLayer currently does not have the attributes that XRLayer has. Should we update the WebXR spec so it does?

We can instead have XRLayer be an empty interface, and the layers spec can define an intermediate XRSomethingLayer interface with all the fields that XRLayer has right now.

Alternatively we can make everything inherit from an emptyXRGenericLayer, and the layers array is an array of XRGenericLayer, and XRLayer is only defined in the layers spec.

cabanier · 2020-04-17T19:40:59Z

We can instead have XRLayer be an empty interface, and the layers spec can define an intermediate XRSomethingLayer interface with all the fields that XRLayer has right now.

Alternatively we can make everything inherit from an emptyXRGenericLayer, and the layers array is an array of XRGenericLayer, and XRLayer is only defined in the layers spec.

OK. I will make that change. This will have some testable side effects but it's extremely unlikely it will break sites.

cabanier · 2020-04-17T20:09:44Z

@Manishearth let me know if this looks good.
If so, can you merge? I will update the layers spec.

Manishearth

Doesn't XRWebGLLayer need to inherit from XRLayer here?

Which of the two options from my comment are you moving forward with?

(also I only want to merge once @toji has also rubber stamped)

index.bs

cabanier · 2020-04-17T21:56:30Z

Doesn't XRWebGLLayer need to inherit from XRLayer here?

Yes. I will fix

Which of the two options from my comment are you moving forward with?

The former.
The layers spec will introduce a new type XRGenericLayer that has the common attributes and derives from XRLayer

Manishearth · 2020-04-17T22:04:25Z

I think it shouldn't be called XRGenericLayer in the layers spec, but that's fine, we can figure out a better name there.

Also the name will never be used by client code so shrug

cabanier · 2020-04-17T22:07:53Z

@Manishearth immersive-web/layers#88 has the changes to the layers spec

Manishearth · 2020-04-17T23:27:55Z

index.bs

@@ -646,6 +655,7 @@ When this method is invoked, the user agent MUST run the following steps:
  1. If |newState|'s {{XRRenderStateInit/inlineVerticalFieldOfView}} is set and |session| is an [=immersive session=], throw an {{InvalidStateError}} and abort these steps.
  1. Let |activeState| be |session|'s [=active render state=].
  1. If |session|'s [=pending render state=] is <code>null</code>, set it to a copy of |activeState|.
+  1. If |newState|'s {{XRRenderStateInit/layers}}'s value is not <code>null</code>, run [=update the pending layers state=] with |session| and |newState|.


I just realized: we want the patch to be able to null out layers when baseLayer is set, right? in that case this step should be unconditional, and the update the pending layers state patch in the layers spec should handle "layers set" and "layers not set" differently

Yes, we should always call that function so when we patch it in the layers spec, we can do the conditional checking.

@Manishearth I just updated the patch.
Hard to keep my head around a patch of a patch of a patch :-)

Manishearth · 2020-04-18T05:52:19Z

index.bs

@@ -634,6 +634,15 @@ Each {{XRSession}} has a <dfn>minimum inline field of view</dfn> and a <dfn>maxi

 Each {{XRSession}} has a <dfn>minimum near clip plane</dfn> and a <dfn>maximum far clip plane</dfn>, defined in meters. The values MUST be determined by the user agent and MUST be non-negative. The [=minimum near clip plane=] SHOULD be less than <code>0.1</code>. The [=maximum far clip plane=] SHOULD be greater than <code>1000.0</code> (and MAY be infinite).

+<div class="algorithm" data-algorithm="update-layers-state">
+
+When the user agent will <dfn>update the pending layers state</dfn> with {{XRSession}} <var ignore>session</var> and {{XRRenderStateInit}} |newState|, it must run the following steps:


TIL <var ignore>. Nifty!

Manishearth · 2020-04-18T05:52:59Z

(still waiting on Brandon to merge, but LGTM)

toji

Sorry for the delayed review. I definitely feel much more comfortable with this pattern vs. what we had previously. Thanks all for contributing to a more elegant solution!

cabanier · 2020-04-20T17:29:51Z

Thanks! If you or @Manishearth could merge it, I can make sure that all the dfn linking from the Layers spec work.

* Update XRRenderStateInit with layers sequence * removed index.html * refactored the layers algorithm * updated algorithm per Manish's suggestions * updated algorithm per Manish's suggestions * changed layers inheritance following feedback from Kip * removed bogus file * Made XRWebGLLayer inherit from XRLayer * switch type of exception * fix typo * removed check from updateRenderState * fix bikeshed warning + moved to an earlier step * removed bogus file Co-authored-by: Henricus Cabanier <cabanier@fb.com>

Update XRRenderStateInit with layers sequence

d5a0c56

cabanier mentioned this pull request Apr 13, 2020

new interface for `updateRenderState' immersive-web/layers#24

Closed

cabanier marked this pull request as draft April 13, 2020 22:24

cabanier marked this pull request as ready for review April 13, 2020 22:24

Manishearth reviewed Apr 13, 2020

View reviewed changes

cabanier added 2 commits April 13, 2020 15:35

removed index.html

108a1f2

refactored the layers algorithm

a1b3907

Manishearth reviewed Apr 13, 2020

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

updated algorithm per Manish's suggestions

8fbb78d

Manishearth approved these changes Apr 13, 2020

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

updated algorithm per Manish's suggestions

4326419

Manishearth approved these changes Apr 13, 2020

View reviewed changes

cabanier commented Apr 14, 2020

View reviewed changes

index.bs Outdated Show resolved Hide resolved

Manishearth added this to the Pre-CR milestone Apr 17, 2020

cabanier added 2 commits April 17, 2020 13:04

changed layers inheritance following feedback from Kip

8de29c8

removed bogus file

c8b2b3f

Manishearth reviewed Apr 17, 2020

View reviewed changes

index.bs Outdated Show resolved Hide resolved

Made XRWebGLLayer inherit from XRLayer

8f4b71d

Manishearth approved these changes Apr 17, 2020

View reviewed changes

Manishearth requested a review from toji April 17, 2020 22:04

cabanier added 2 commits April 17, 2020 16:11

switch type of exception

a529ebe

fix typo

a440746

Manishearth reviewed Apr 17, 2020

View reviewed changes

removed check from updateRenderState

7220558

Manishearth approved these changes Apr 18, 2020

View reviewed changes

cabanier added 2 commits April 18, 2020 11:30

fix bikeshed warning + moved to an earlier step

e957eff

removed bogus file

7a75b8a

toji approved these changes Apr 20, 2020

View reviewed changes

toji merged commit f395da3 into immersive-web:master Apr 20, 2020

cabanier deleted the update-updaterenderstate branch April 20, 2020 17:31

toji mentioned this pull request Apr 20, 2020

Refactor XRLayers immersive-web/layers#88

Merged

cabanier mentioned this pull request May 19, 2020

Change to "run an animation frame"? immersive-web/layers#143

Closed

Update XRRenderStateInit with layers sequence #999

Update XRRenderStateInit with layers sequence #999

Uh oh!

Conversation

cabanier commented Apr 13, 2020

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Manishearth Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

cabanier Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cabanier commented Apr 13, 2020

Uh oh!

Manishearth commented Apr 13, 2020

Uh oh!

cabanier commented Apr 13, 2020

Uh oh!

toji commented Apr 13, 2020

Uh oh!

cabanier commented Apr 14, 2020

Uh oh!

Manishearth commented Apr 14, 2020

Uh oh!

cabanier commented Apr 14, 2020

Uh oh!

Manishearth commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kearwood commented Apr 16, 2020

Uh oh!

kearwood commented Apr 16, 2020

Uh oh!

cabanier commented Apr 16, 2020

Uh oh!

kearwood commented Apr 16, 2020

Uh oh!

Manishearth commented Apr 16, 2020

Uh oh!

kearwood commented Apr 16, 2020

Uh oh!

cabanier commented Apr 16, 2020

Uh oh!

kearwood commented Apr 16, 2020

Uh oh!

kearwood commented Apr 16, 2020

Uh oh!

cabanier commented Apr 16, 2020

Uh oh!

cabanier commented Apr 17, 2020

Uh oh!

Manishearth commented Apr 17, 2020

Uh oh!

cabanier commented Apr 17, 2020

Uh oh!

cabanier commented Apr 17, 2020

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cabanier commented Apr 17, 2020

Uh oh!

Manishearth commented Apr 17, 2020

Uh oh!

cabanier commented Apr 17, 2020

Uh oh!

Manishearth commented Apr 14, 2020 •

edited

Loading