-
Notifications
You must be signed in to change notification settings - Fork 20
Refactor XRLayers #88
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
Integrate with main repo
webxrlayers-1.bs
Outdated
|
|
||
| <pre class="idl"> | ||
| interface XRLayer { | ||
| interface XRGenericLayer { |
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 don't like this name, but that's a very minor concern.
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'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.
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.
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.
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.
If we end up keeping
XRGenericLayerit's fine, it just sounds slightly awkward to me.
I like the name XRCompositionLayer. I will update the PR with that name.
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.
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.
|
@Manishearth I moved the check to earlier. Otherwise the |
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.
LGTM!
No description provided.