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

Fix WebGL mode for Adreno GPUs #8508

Merged
merged 3 commits into from
Jun 29, 2023
Merged

Fix WebGL mode for Adreno GPUs #8508

merged 3 commits into from
Jun 29, 2023

Conversation

TotalKrill
Copy link
Contributor

Objective

Solution

  • The shader pbr_functions.wgsl, will fail in apply_fog function, trying to access values that are null on Adreno chipsets using WebGL, these devices are commonly found in android handheld devices.

@alice-i-cecile alice-i-cecile added O-Web Specific to web (WASM) builds C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen P-Crash A sudden unexpected crash O-Android Specific to the Android mobile operating system labels Apr 28, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't verify this myself, but the comments are good and the fix is reasonable.

@alice-i-cecile alice-i-cecile modified the milestones: 0.11, 0.10.2 Apr 28, 2023
//NOTE: When running bevy on Adreno GPU chipsets in WebGL, any value above 1 will result in a crash
// when loading the wgsl "pbr_functions.wgsl" in the function apply_fog.
#[cfg(feature = "webgl")]
pub const MAX_DIRECTIONAL_LIGHTS: usize = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, is this the case for all WebGL2? Or just with some Adreno GPU drivers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its actually only the case for Adreno GPU drivers, but I am not sure how to address that.

Copy link
Contributor

@Elabajaba Elabajaba Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's possible if we're using a const here, since you have to query the gpu driver at runtime.

edit: Also, is this for all Adreno GPUs, or only certain ones? Do you know if it's an issue on just Android, or does it also impact Linux?

edit2: I should've read the linked issues first...

@mockersf
Copy link
Member

so the question is, do we want to reduce functionality for all webgl2 app for a few android devices?

@mockersf mockersf added the X-Controversial There is active debate or serious implications around merging this PR label Apr 28, 2023
@Elabajaba
Copy link
Contributor

Elabajaba commented Apr 28, 2023

so the question is, do we want to reduce functionality for all webgl2 app for a few android devices?

If we make this not a const, it's possible to detect the driver at runtime using https://github.com/gfx-rs/wgpu/blob/2571af9557eeba59f00f69649ae6bcd9d5a62392/wgpu-types/src/lib.rs#L1303

@TotalKrill
Copy link
Contributor Author

TotalKrill commented Apr 28, 2023

so the question is, do we want to reduce functionality for all webgl2 app for a few android devices?

Yeah, I agree, its not the ideal solution. And getting a feature to start tweaking rendering paths depending on which adaptor is detected, will probably be the best solution.

But then again, this is at least a third of all android devices (Adreno marketshare is approx 33% from what I can tell), not really only a few. And we are talking about a hard crash on approx 1/3 of android devices, that was not there before 10.0 vs having 9 extra directional lights available for webgl targets.

I still think that speaks in favor of this. Even if not ideal. And it will not crash WebGL targets that use more directional lights.

@TotalKrill
Copy link
Contributor Author

In addition to this, I also do not think that more directional lights works as intended on WebGL regardless, from what I can tell, shadows does not seem to work at least on my computer.

@alice-i-cecile alice-i-cecile modified the milestones: 0.10.2, 0.11 Jun 2, 2023
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we make this not a const, it's possible to detect the driver at runtime using https://github.com/gfx-rs/wgpu/blob/2571af9557eeba59f00f69649ae6bcd9d5a62392/wgpu-types/src/lib.rs#L1303

I briefly poked around with this. Making it non-const is non-trivial and will require a bit of elbow grease.

I do think thats the best long-term strategy though. We should investigate it. I created #8992 to track that.

I just resolved merge conflicts.

@cart cart enabled auto-merge June 29, 2023 00:53
@cart cart added this pull request to the merge queue Jun 29, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 29, 2023
@cart cart added this pull request to the merge queue Jun 29, 2023
@mockersf mockersf removed this pull request from the merge queue due to a manual request Jun 29, 2023
Co-authored-by: François <mockersf@gmail.com>
@cart cart enabled auto-merge June 29, 2023 04:10
@cart cart added this pull request to the merge queue Jun 29, 2023
Merged via the queue into bevyengine:main with commit d90c65d Jun 29, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior O-Android Specific to the Android mobile operating system O-Web Specific to web (WASM) builds P-Crash A sudden unexpected crash X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fog crashes on certain mobile devices in WASM WASM WebGL examples crash "wgpu error: Validation Error"
6 participants