KEMBAR78
Refactor XRLayers by cabanier · Pull Request #88 · immersive-web/layers · GitHub
Skip to content

Conversation

@cabanier
Copy link
Member

No description provided.

@cabanier cabanier requested review from Manishearth and toji April 17, 2020 20:50
@cabanier cabanier marked this pull request as ready for review April 17, 2020 22:07
webxrlayers-1.bs Outdated

<pre class="idl">
interface XRLayer {
interface XRGenericLayer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this name, but that's a very minor concern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of this either, but after reading through immersive-web/webxr#999 I at least understand why we need a different name. (Although shouldn't this be extended from XRLayer as defined in the core spec now?)

AS far as naming goes, OpenXR refers to all layers as "XrCompositionLayer_____" so maybe XRCompositionLayer or XRCompositorLayer could work as alternatives here? Other ideas off the top of my head: XRLayerBase (May be kind of confusing since XRLayer is actually the base one.) XRBindingsLayer since they come from and XR____Binding object? Or we could go the Microsoft route and define an XRLayerEx 😆

If we end up keeping XRGenericLayer it's fine, it just sounds slightly awkward to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other option, I guess, is that since the amount of shared values here is limited and there's certain attributes that we're sharing across all but one layer type already, maybe we just skip having this intermediate layer type all together and have each layer copy these into their own interface.

It's not the most elegant thing ever, but there's not a whole lot that this structure concretely buys us anyway besides preventing you from passing an XRWebGLLayer into get[View]SubImage(). But even that will need to make sure that the layer was not created by a different XR____Binding object in it's algorithm anyway (because we don't want WebGL bindings to return images for WebGPU-created layers.) So maybe i wouldn't be a terrible things to drop this type from the hierarchy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we end up keeping XRGenericLayer it's fine, it just sounds slightly awkward to me.

I like the name XRCompositionLayer. I will update the PR with that name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other option, I guess, is that since the amount of shared values here is limited and there's certain attributes that we're sharing across all but one layer type already, maybe we just skip having this intermediate layer type all together and have each layer copy these into their own interface.

There's still quite a bit of common interface between the composition layers so I think we should keep a common base class.

@cabanier
Copy link
Member Author

@Manishearth I moved the check to earlier. Otherwise the pending render state was updated.
I also updated the layers spec to deal with this.

@cabanier cabanier requested a review from Manishearth April 20, 2020 21:03
Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@cabanier cabanier merged commit c5b6cbf into immersive-web:master Apr 20, 2020
@cabanier cabanier deleted the xrlayer-rename branch April 20, 2020 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants