-
Notifications
You must be signed in to change notification settings - Fork 210
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
feat(webgl): Pass through canvas in WebGLDevice.attach() #1931
Conversation
Can you use gl.canvas? I mean why pass it in if you already have the context? |
@ibgreen I'm not sure where you mean exactly by "use", I've pushed a change that uses In general I find it a bit strange that we accept both |
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 sure where you mean exactly by "use", I've pushed a change that uses gl.canvas in the constructor instead.
Yes... maybe my comment was a bit tardy, I was on a flight.
In general I find it a bit strange that we accept both gl and canvas in DeviceProps, is there ever a case where we would have one and not the other?
The idea is that you can
- provide a canvas and the device creates a context for that canvas
- provide a gl context and the Device wraps that context. In this case there is only one canvas that can be used, the canvas that this context was created for.
@@ -52,6 +53,8 @@ export class WebGLCanvasContext extends CanvasContext { | |||
* See http://webgl2fundamentals.org/webgl/lessons/webgl-resizing-the-canvas.html | |||
*/ | |||
resize(options?: {width?: number; height?: number; useDevicePixels?: boolean | number}): void { | |||
if (!this.device.gl) return; |
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.
This should not be possible (i.e. a WebGLDevive should always have a gl context), unless this is some mid-initialization issue with function being called before Device is full constructed.
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.
Yes, is it due to the initialization sequence. Creating a new WebGLCanvasContext
invokes update()
and thus resize()
, which expects there to be a gl
context present on the device. Later the creation of the gl
context can depend on a canvasContext
already being available, so there is a circular dependency.
Merging this for now, and will try to followup with a better solution
@@ -192,7 +192,8 @@ ${device.info.vendor}, ${device.info.renderer} for canvas: ${device.canvasContex | |||
} | |||
|
|||
// Create and instrument context | |||
this.canvasContext = new WebGLCanvasContext(this, props); | |||
const canvas = props.canvas || props.gl?.canvas; |
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.
We might even change the order here, if there is a gl context it should not be possible to override the canvas. Or even throw an error
Background
When attaching to an existing WebGL context
WebGLDevice.attach()
doesn't pass through the canvas, leading tonew WebGLDevice()
to create another canvas, even if the WebGL context already has one (as is commonly the case). For example affects: visgl/deck.gl#8442Change List
canvas
inWebGLDevice.attach()