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

WebGLState: Use getParameter() to detect current scissor/viewport. #21831

Merged
merged 2 commits into from
May 14, 2021

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented May 13, 2021

Related issue: Fixed #21824.

Description

As suggested in #21824 (comment).

@gkjohnson
Copy link
Collaborator

Looks good! gl.canvas is also used in the reset function here and here. Should those be changed to use gl.drawingBuffer* considering they seem to more accurately reflect the initial viewport and scissor settings anyway?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 13, 2021

Using gl.canvas should not cause issues in reset().

@gkjohnson
Copy link
Collaborator

gkjohnson commented May 14, 2021

@WestLangley

Variables sized in device pixels should be assigned values in device pixel units -- not in CSS pixel units.

canvas.width and canvas.height should reflect the current size of the render buffer (unless the device cannot support the requested size), right? CSS dimensions would be retrieved with clientWidth and clientHeight.

@Mugen87

Using gl.canvas should not cause issues in reset().

If expo is setting gl.canvas to null as the MDN docs say its allowed to then calling reset will cause an error. And it seemed to me that the drawingBuffer* values were the more correct ones to use given my test in #21824 (comment). Maybe calling reset doesn't happen often enough for this to be a noticeable issue, though.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 14, 2021

The usage of getParameter() makes sense in the initialization part of WebGLState and is an improvement.

However, IMO using the drawingBuffer properties is not the right thing to do. You have mentioned an issue in #21824 (comment) which was already discussed in #5194 and which is browser specific. I think we should not start to implement workarounds for this nor make the engine "smart". Coupling the viewport and scissor to the dimensions of the canvas seems the most natural thing to do (independently of Expo). Before edge cases like too large canvas sizes can be properly handled, browser implementations have to treat this issue in a common way.

@Mugen87 Mugen87 added this to the r129 milestone May 14, 2021
@Mugen87 Mugen87 merged commit be73fc9 into mrdoob:dev May 14, 2021
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.

Three.JS WebGLRenderer fails to create when canvas is not initialized.
2 participants