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

Respect DownLevel flag for Cube Array Textures for point_light_shadow_map_array_texture_view. #12849

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

sampettersson
Copy link
Contributor

@sampettersson sampettersson commented Apr 2, 2024

Objective

This is a follow up on my PR #12052 for fixing missing Cube Array Texture support on iOS simulator. I've since noticed that iPhone 7, iPad 6th and 7th gen also run a Metal family not supporting Cube Array Textures. This PR makes it so that we respect the DownLevel flags from WGPU through the whole chain instead of using features to pick the correct one.

Solution

  • Use render_adapter.get_downlevel_capabilities().flags.contains(DownlevelFlags::CUBE_ARRAY_TEXTURES) to identify which texture format we should use instead of relying on a compile time feature flag.

Changelog

Fixed

  • Use correct texture format for point_light_shadow_map_array_texture_view on GPUs which are missing CUBE_ARRAY_TEXTURES support.

Sam Pettersson added 2 commits April 2, 2024 16:07
# Conflicts:
#	crates/bevy_pbr/Cargo.toml
#	crates/bevy_pbr/src/render/light.rs
#	crates/bevy_pbr/src/render/mesh.rs
#	crates/bevy_pbr/src/render/mesh_view_bindings.rs
#	crates/bevy_render/src/render_resource/pipeline_cache.rs
# Conflicts:
#	crates/bevy_pbr/Cargo.toml
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen O-iOS Specific to the iOS mobile operating system labels Apr 2, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Apr 2, 2024
@james7132 james7132 requested a review from superdump April 3, 2024 01:26
@Elabajaba
Copy link
Contributor

iPhone 7, iPad 6th and 7th gen

Just noting that these are all unsupported hardware for webgpu (it's targeting iphone 8/a11 SOC as the min spec), and the 6th gen iPad especially is a massive pain to support (high res screen with only 1.4GB usable ram for both GPU and CPU means constant OOMs).

@sampettersson
Copy link
Contributor Author

@Elabajaba we've had some degree of success running in production on iPad 6th and iPad 7th after employing this change and using WgpuSettingsPriority::Compatibility using the Metal backend natively on iPadOS. These iPads are still very widely supported by Apple, they got iPadOS 17 for example, and really common unfortunately. So they are devices we feel like we need support in our application to some degree.

@alice-i-cecile alice-i-cecile added X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Testing Testing must be done before this is safe to merge labels May 16, 2024
@alice-i-cecile alice-i-cecile removed this from the 0.14 milestone May 16, 2024
Sam Pettersson added 2 commits May 23, 2024 15:29
…lag-cube-array-texture

# Conflicts:
#	crates/bevy_pbr/src/render/light.rs
#	crates/bevy_pbr/src/render/mesh.rs
#	crates/bevy_pbr/src/render/mesh_view_bindings.rs
@@ -699,8 +715,7 @@ pub fn prepare_lights(
point_light_shadow_map: Res<PointLightShadowMap>,
directional_light_shadow_map: Res<DirectionalLightShadowMap>,
mut shadow_render_phases: ResMut<ViewBinnedRenderPhases<Shadow>>,
mut max_directional_lights_warning_emitted: Local<bool>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Started to fail compilation after merging in main, I think it was due to many arguments tripping the compiler out:

error[E0599]: the method `in_set` exists for fn item `fn(Commands<'a, 'b>, ..., ..., ..., ..., ..., ..., ..., ..., ..., ..., ..., ..., ..., ..., ..., ...) {prepare_lights}`, but its trait bounds were not satisfied
   --> crates/bevy_pbr/src/lib.rs:401:26

Easiest way around it I could find was to merge these two locals into a container struct.

@BD103 BD103 added X-Contentious There are nontrivial implications that should be thought through and removed X-Uncontroversial This work is generally agreed upon labels Jun 2, 2024
@jinleili
Copy link
Contributor

jinleili commented Jun 18, 2024

FWIW, Apple provides a separate device type for the simulator, with its capabilities limited to an Apple family 2 GPU (which is the old A8 chip). This means that the simulator often supports fewer features or has more limitations compared to actual iOS devices. Here is the relevant documentation: https://developer.apple.com/documentation/metal/developing_metal_apps_that_run_in_simulator

I think it might not be necessary to modify the render code for compatibility with the A8 chip.

@sampettersson
Copy link
Contributor Author

sampettersson commented Jun 18, 2024

@jinleili this specific PR fixes issues pertaining to physical Apple devices, not the iOS simulator. We've been running this change in production for a few months and are quite stable on those devices post this change.

I can't share exact stats, but active sessions are in the thousands per day on those devices. So we are quite confident that this works. Would be great if we could get this is in, right now we are currently running of a fork with this being the only change employed.

@mockersf
Copy link
Member

This breaks support for Wasm/WebGL2:

Caused by:
    In Device::create_render_pipeline
      note: label = `pbr_opaque_mesh_pipeline`
    Error matching ShaderStages(FRAGMENT) shader requirements against the pipeline
    Shader global ResourceBinding { group: 0, binding: 2 } is not available in the pipeline layout
    View dimension Cube (is array: false) doesn't match the binding Texture { sample_type: Depth, view_dimension: CubeArray, multisampled: false }

@sampettersson
Copy link
Contributor Author

@mockersf thanks for testing ⭐ this should be resolved now.

@mockersf
Copy link
Member

works for me! I would prefer the if to be only on the changing field like in my suggested change

Copy link
Contributor

@kristoff3r kristoff3r left a comment

Choose a reason for hiding this comment

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

I haven't tested it, but the changes seem correct to me.

Is the ios_simulator feature still used after this? If not this PR should also remove it + docs and other references.

not(target_arch = "wasm32"),
feature = "webgpu"
))]
let supports_cube_array_textures = render_adapter
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason that this would not work in all cases? I feel like it should and then the cfg stuff isn’t needed which simplifies this quite a bit.

Copy link
Member

Choose a reason for hiding this comment

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

that's how the PR was before #12849 (comment), but that crashed on WebGL2

not(target_arch = "wasm32"),
feature = "webgpu"
))]
let supports_cube_array_textures = render_adapter
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Why can’t this code be used in all cases?

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Testing Testing must be done before this is safe to merge labels Jun 20, 2024
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 D-Straightforward Simple bug fixes and API improvements, docs, test and examples O-iOS Specific to the iOS mobile operating system S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

8 participants