-
Notifications
You must be signed in to change notification settings - Fork 977
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
Lower max_color_attachments limit for GL to 4 #6994
base: trunk
Are you sure you want to change the base?
Lower max_color_attachments limit for GL to 4 #6994
Conversation
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.
For reference: https://registry.khronos.org/OpenGL-Refpages/es3.0/html/glGet.xhtml also says the minimum value is 4 for GLES 3.0, so this change makes sense in any case! 👍
@adrian17 Please add a line to the changelog under bugfixes |
I tested this on a device that had this problem (Raspberry Pi 4). The error is gone and the program (ruffle) is starting with no more errors. |
597c900
to
efb65e7
Compare
Added changelog. Should I assume that the wasm CI failures are expected? Looking at logs, they didn't seem related to my changes. |
So Wasm is failing, but apparently the logging config we're using casues there to be 41MB of logs... I'll take a look at this on monday |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
It's still failing somehow on wasm :c Here's the first relevant part of the CI logs, I think?
|
Yeah this means gl.GetError is returning something - you unfortunately need to run the tests in the browser to get the full error. I can do this if it's a bother. |
I just ran the suite locally and they passed :(
I also tried with again pretending that I don't understand how this test can be failing now; if the entire CI environment had My best guess is that this has something to do with this hardcoded constant, which I didn't know whether I should have updated or not: https://github.com/gfx-rs/wgpu/blob/trunk/wgpu-hal/src/lib.rs#L298 |
Connections
#6986
Description
Wgpu 23 broke support for older devices that only supported GLES and
max_color_attachments == 4
(which is the minimum mandated by 3.1 spec). The limit technically used to be 8 for much longer, but according to @cwfitzgerald it wasn't enforced before. This change aims to revert this regression.Only downlevel limits are changed, not default limits.
Note that I can't prove that this is the only thing that needs changing to make wgpu work on such old devices again, because I'm only relying on user reports and can't check it myself.
Testing
Ran
cargo test --doc
only.Only tested manually with a hack, since I don't have access to such a device:
let max_color_attachments = unsafe { gl.get_parameter_i32(glow::MAX_COLOR_ATTACHMENTS) .min(gl.get_parameter_i32(glow::MAX_DRAW_BUFFERS)) .min(crate::MAX_COLOR_ATTACHMENTS as i32) as u32 }; + let max_color_attachments = 4;
Checklist
cargo fmt
.taplo format
.cargo clippy
. If applicable, add:--target wasm32-unknown-unknown
--target wasm32-unknown-emscripten
cargo xtask test
to run tests.CHANGELOG.md
. See simple instructions inside file.