Skip to content
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

Clarify semantics of preserveDrawingBuffer #891

Closed
asajeffrey opened this issue Oct 18, 2019 · 10 comments · Fixed by #975
Closed

Clarify semantics of preserveDrawingBuffer #891

asajeffrey opened this issue Oct 18, 2019 · 10 comments · Fixed by #975
Labels
fixed by pending PR A PR that is in review will resolve this issue. help wanted This is a good issue for anyone to pick up and work on filing a PR for.
Milestone

Comments

@asajeffrey
Copy link

https://immersive-web.github.io/webxr/#opaque-framebuffer says

The buffers attached to an opaque framebuffer MUST be cleared to the values in the table below when first created, or prior to the processing of each XR animation frame. This is identical to the behavior of the WebGL context’s default framebuffer.

Implicitly, this means ignoring preserveDrawingBuffer, but perhaps this should be made explicit? Especially because this is not identical to WebGL!

@rcabanier
Copy link
Contributor

We already defined that preserveDrawingBuffer is not supported in WebXR

@rcabanier
Copy link
Contributor

See @toji 's PR: #866

@asajeffrey
Copy link
Author

Er, doing a search for preserveDrawingBuffer in that PR doesn't find anything.

@rcabanier
Copy link
Contributor

rcabanier commented Oct 21, 2019

I thought we discussed it as part of #775 which that PR solved.
I agree that if the spec doesn't call this out, it should (just like how we call out premultipliedAlpha)

@Manishearth
Copy link
Contributor

I think the more general discussion around that was that the normal webgl context options are to be ignored, the only way to set them is through XRWebGLLayer, and if an option (like premultipliedAlpha) is not in XRWebGLLayerInit then there is no way to set it for XR. We could call this out more explicitly.

@asajeffrey
Copy link
Author

Since WebXR inherits its WebGLContext from canvas, there will be a value for preserveDrawingBuffer, we should just be explicit that webXR ignores it. This does mean there may be odd effects on pages that use it, where the web page's canvas will display different content than webXR, even if uses the same WebGL code.

@klausw
Copy link
Contributor

klausw commented Oct 21, 2019

I think a core part of the confusion is that the canvas WebGL options are a mix of general context properties together with configuration for the default framebuffer used for canvas drawing. Even apart from WebXR, an offscreen framebuffer would have independent properties such as depth/alpha that don't inherit from the canvas creation attributes, and using premultiplied alpha would be an application choice for purely-internal buffers, it only needs to be specified as metadata for external interfaces. There was more discussion in webxr-ar-module issue 14 about this.

Would it be feasible to update the canvas / WebGL spec to differentiate between general properties and configuration of the canvas-backing default framebuffer? This wouldn't require any API changes, the goal would just be a documentation update to avoid people expecting them to apply to non-default render buffers.

@Manishearth Manishearth added the help wanted This is a good issue for anyone to pick up and work on filing a PR for. label Oct 21, 2019
@Manishearth Manishearth added this to the November 2019 milestone Oct 21, 2019
@Manishearth
Copy link
Contributor

We should just be explicit that we ignore preserveDrawingBuffer here (and perhaps others?)

@rcabanier
Copy link
Contributor

I agree. The spec should say that if a canvas is provided, all unsupported properties should revert to their default state. We called this out for premultipliedAlpha but not for anything else.

@probot-label
Copy link

probot-label bot commented Mar 4, 2020

This issue is fixed by PR #975

@probot-label probot-label bot added the fixed by pending PR A PR that is in review will resolve this issue. label Mar 4, 2020
@toji toji closed this as completed in #975 Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed by pending PR A PR that is in review will resolve this issue. help wanted This is a good issue for anyone to pick up and work on filing a PR for.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants