Get raw window handle only after Resume event #3639

Gordon-F · 2022-01-11T15:38:43Z

Objective

An attempt to fix #3249.

Solution

Get raw window handle and create surface after winit Resumed event.

On my device it works only with OpenGL and disabled PBR (broken due to gfx-rs/wgpu#4455).

~~Vulkan error with 3D scene example and enabled pbr:~~ (My device doesn't supports DEPTH_STENCIL_ATTACHMENT for VK_FORMAT_D32_SFLOAT)

I/RustStdoutStderr: thread '<unnamed>' panicked at 'wgpu error: Validation Error
2022-01-11 22:36:19.301 2407-2445/rust.example.android I/RustStdoutStderr: Caused by:
2022-01-11 22:36:19.301 2407-2445/rust.example.android I/RustStdoutStderr:     In Device::create_texture
2022-01-11 22:36:19.301 2407-2445/rust.example.android I/RustStdoutStderr:       note: label = `view_depth_texture`
2022-01-11 22:36:19.301 2407-2445/rust.example.android I/RustStdoutStderr:     The texture usages RENDER_ATTACHMENT are not allowed on a texture of type Depth32Float

alice-i-cecile · 2022-01-11T15:48:34Z

Thank you for investigating this! @mockersf, this is one of the main problems we need to fix to unbreak Android right?

mockersf · 2022-01-11T18:11:17Z

Yup that looks good! Even if pbr is still failing, if 2d and UI works on Android that would be nice.

@Gordon-F to undraft that PR, could you re-add the various part of Bevy that you commented and are behind a feature (pbr, audio and gilrs I think)? Maybe setting up the android example to be like the iOS one and have its own Cargo.toml file to specify which features of Bevy are working: https://github.com/bevyengine/bevy/blob/main/examples/ios/Cargo.toml

Also you can insert the WgpuOptions resource before the DefaultPlugins in the example to be able to select the GL backend:

bevy/crates/bevy_render/src/options.rs

Lines 13 to 20 in 130953c

    
           pub struct WgpuOptions { 
        
               pub device_label: Option<Cow<'static, str>>, 
        
               pub backends: Option<Backends>, 
        
               pub power_preference: PowerPreference, 
        
               pub priority: WgpuOptionsPriority, 
        
               pub features: WgpuFeatures, 
        
               pub limits: WgpuLimits, 
        
           }

Gordon-F · 2022-01-11T19:29:53Z

Maybe setting up the android example to be like the iOS one and have its own Cargo.toml

Sure, nice idea. Something really strange with Vulkan on my device (driver bug?). 2D example flickering (on device screen image flickering faster). But now it possible to draw something. Better than panic :)

vulkan.mp4

opengl.mp4

crates/bevy_render/src/lib.rs

crates/bevy_render/src/options.rs

bjorn3 · 2022-01-12T21:14:16Z

Would it make sense to request the adapter and surface again after every Resume event? That would avoid the need for a wasm special case, would allow recovering when the surface or adapter is lost (eg because the gpu got reset to recover from a lockup, timeout or memory protection fault) and would also prepare bevy_render for allowing the user to switch between graphics adapters at runtime using a settings menu or when the window moves to a display connected to a different gpu in the future.

Gordon-F · 2022-01-12T21:47:37Z

Would it make sense to request the adapter and surface again after every Resume event?

winit executing Resumed event only for iOS and Android. On iOS you have only 1 adapter all the time. It's possible only when we will implemented lifecycle management api.

Can we remove handle_initial_window_events at all? Why it's necessary? Maybe I missed something.

https://github.com/bevyengine/bevy/blob/main/crates/bevy_winit/src/lib.rs#L39

Gordon-F · 2022-01-14T10:11:09Z

ci job will be fixed #3667. But I have no idea why build-android failed :(

crates/bevy_core_pipeline/src/lib.rs

Gordon-F · 2022-01-15T22:07:13Z

CI is happy now 🎉

DJMcNab

There's nothing obviously wrong (other than the exceedingly strange implementation of indexing), but I'm not confident to fully sign off on all these changes. It seems like some potentially risky changes for 0.6.1.

crates/bevy_render/src/renderer/mod.rs

crates/bevy_winit/src/lib.rs

alice-i-cecile · 2022-01-15T23:35:12Z

It seems like some potentially risky changes for 0.6.1.

Agreed, this needs more eyes. However, (even partially) unbreaking Android would be very nice.

mockersf · 2022-01-17T01:02:37Z

Can we remove handle_initial_window_events at all? Why it's necessary? Maybe I missed something.

It would be nice if you could clarify this. I think I remember it was needed on one of the platform, but I'm not sure.
Could you check by removing it and running on linux, macos, windows, iphone, android and web? If you're missing some of those, could someone else try?

Gordon-F · 2022-01-17T09:40:27Z

I think I remember it was needed on one of the platform, but I'm not sure.

Based on my tests (Windows, Android, WASM, MacOS, iOS), it is required for WASM target. I think initialize_renderer should be a part of the startup system, not a plugin build step. But it's out of the scope of this PR, especially if this code plans to be a part of the 0.6.1 patch.

mockersf · 2022-01-17T18:38:23Z

crates/bevy_render/src/renderer/mod.rs

+            let preferred_gpu_index = match options.power_preference {
+                wgpu::PowerPreference::LowPower => {
+                    integrated.or(other).or(discrete).or(virt).or(cpu)
+                }
+                wgpu::PowerPreference::HighPerformance => {
+                    discrete.or(other).or(integrated).or(virt).or(cpu)
+                }
+            }


this feels like recoding what https://docs.rs/wgpu/0.12.0/wgpu/struct.Instance.html#method.request_adapter should be doing

Yep, it's a copy-pasted code from wgpu. But we should enumerate adapters here to make sure that we can use default texture format and default depth format as a render target for the selected adapter (for example, on my Android device Vulkan adapter can't use Depth32Float as a render target). request adapter just return first adapter which meets requirements of RequestAdapterOptions.

that makes me sad... I think we should either:

switch from Depth32Float to Depth24Plus as the comment suggest, and which should be supported on Vulkan android (maybe with a cfg to check target architecture?)

upstream to wgpu a way to select an adapter based on the texture format it supports

switch from Depth32Float to Depth24Plus as the comment suggest, and which should be supported on Vulkan android (maybe with a cfg to check target architecture?)

As far as I know, only VK_FORMAT_D16_UNORM supported across all Vulkan powered devices (99% based on gpuinfo https://vulkan.gpuinfo.org/listoptimaltilingformats.php?platform=android)

upstream to wgpu a way to select an adapter based on the texture format it supports

request_adapter is implemented following by WebGPU spec. I don't think we can change it.

That's why wgpu-rs exposed enumerate_adapters function. It's allowed users to select adapter based on their options. Perhaps in the future, we will need to check if the adapter supports any other features.

Should we really be picking devices based on what texture / depth formats they support? That seems like a pretty trivial criteria relative to things like gpu features and performance. Wouldn't it make more sense to do runtime queries of supported formats and then pick the best one? Every device will support "something".

@cart I implemented this behavior because bevy uses Depth32Float as a default depth format and it's required for bevy_renderer. When I tested this on a real device, it have Depth32Float format on OpenGL adapter, but Vulkan adapter doesn't support it. (Android 8.0).

We can remove this part from PR. It unblocks some Android devices (maybe most of the devices, but definitely not all of them).

Yeah short term if we can move this forward without this behavior I think we should. I do acknowledge that this is a problem that needs solving, but I think it should be solved via querying supported depth formats at runtime after we've selected our device based on more substantial criteria.

Anything that currently consumes hard-coded Depth32Float constants could call something like RenderDevice::get_preferred_depth_format() instead.

Anything that currently consumes hard-coded Depth32Float constants could call something like RenderDevice::get_preferred_depth_format() instead.

Sorry, maybe I confused you. We can query depth format only from adapter (correct me, if I'am wrong). For example, on real device:

Create instance with Vulkan and GL backend

Enumerate adapters:

Vulkan (Depth32Float is not available)

OpenGL (Depth32Float is available)

What we should do in this case?

We would choose vulkan (if that is what wgpu recommends based on the wgpu config) and pick a different depth format supported by the device.

mockersf · 2022-01-17T18:42:10Z

Based on my tests (Windows, Android, WASM, MacOS, iOS), it is required for WASM target.

Could you add a comment mentioning it is needed on wasm?

ManevilleF · 2022-03-01T13:44:33Z

+1

I tried running the android example, now a 2D animated sprite, on my android phone and it works.

I also tried replacing the example code with the 3D_example code and it works fine:

Landscape:

Portrait:

I think this fix is very useful and this branch should be updated to fix the merge conflicts and merged in main

@mockersf @Gordon-F

EDIT: Small suggestion, having both 3D and 3D examples could be useful I think

ManevilleF · 2022-03-01T15:42:48Z

I tried to apply this PR changes on the main branch on this commit (branch)

But I have a black screen, and after a few seconds android telling me the app is not responding:

adb logcat:

03-02 09:10:59.898 19549 19575 I RustStdoutStderr: thread '<unnamed>' panicked at 'wgpu error: Validation Error
03-02 09:10:59.898 19549 19575 I RustStdoutStderr:
03-02 09:10:59.898 19549 19575 I RustStdoutStderr: Caused by:
03-02 09:10:59.898 19549 19575 I RustStdoutStderr:     In a RenderPass
03-02 09:10:59.898 19549 19575 I RustStdoutStderr:       note: encoder = `<CommandBuffer-(0, 1, Vulkan)>`
03-02 09:10:59.898 19549 19575 I RustStdoutStderr:     In a pass parameter
03-02 09:10:59.898 19549 19575 I RustStdoutStderr:       note: command buffer = `<CommandBuffer-(0, 1, Vulkan)>`
03-02 09:10:59.898 19549 19575 I RustStdoutStderr:     attachment's sample count 2 is invalid
03-02 09:10:59.898 19549 19575 I RustStdoutStderr:
03-02 09:10:59.898 19549 19575 I RustStdoutStderr: ', wgpu-0.12.0/src/backend/direct.rs:2273:5
03-02 09:10:59.898 19549 19575 I RustStdoutStderr: note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

So I don't know what's missing

superdump · 2022-03-06T13:28:32Z

I tried to apply this PR changes on the main branch on this commit (branch)

But I have a black screen, and after a few seconds android telling me the app is not responding:

adb logcat:

03-02 09:10:59.898 19549 19575 I RustStdoutStderr: thread '<unnamed>' panicked at 'wgpu error: Validation Error
03-02 09:10:59.898 19549 19575 I RustStdoutStderr:
03-02 09:10:59.898 19549 19575 I RustStdoutStderr: Caused by:
03-02 09:10:59.898 19549 19575 I RustStdoutStderr:     In a RenderPass
03-02 09:10:59.898 19549 19575 I RustStdoutStderr:       note: encoder = `<CommandBuffer-(0, 1, Vulkan)>`
03-02 09:10:59.898 19549 19575 I RustStdoutStderr:     In a pass parameter
03-02 09:10:59.898 19549 19575 I RustStdoutStderr:       note: command buffer = `<CommandBuffer-(0, 1, Vulkan)>`
03-02 09:10:59.898 19549 19575 I RustStdoutStderr:     attachment's sample count 2 is invalid
03-02 09:10:59.898 19549 19575 I RustStdoutStderr:
03-02 09:10:59.898 19549 19575 I RustStdoutStderr: ', wgpu-0.12.0/src/backend/direct.rs:2273:5
03-02 09:10:59.898 19549 19575 I RustStdoutStderr: note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

So I don't know what's missing

This attachment's sample count 2 is invalid looks like an MSAA bug. I think only 1x (i.e. no MSAA) and 4x are required to be supported. 2x is hit and miss.

ManevilleF · 2022-03-07T08:34:44Z

@superdump thank you that was indeed the issue !

I'll open a duplicate PR without the conflicts

Gordon-F · 2022-08-21T11:55:15Z

Closing PR, since initial android support already merged in #5130

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 11, 2022

bjorn3 reviewed Jan 12, 2022

View reviewed changes

crates/bevy_render/src/lib.rs Outdated Show resolved Hide resolved

Gordon-F mentioned this pull request Jan 12, 2022

EGL failed to create surface when all backends enabled gfx-rs/wgpu#2384

Open

Gordon-F force-pushed the android_fix branch from f887d72 to 4f6d159 Compare January 12, 2022 20:17

Gordon-F marked this pull request as ready for review January 12, 2022 20:21

bjorn3 reviewed Jan 12, 2022

View reviewed changes

crates/bevy_render/src/options.rs Outdated Show resolved Hide resolved

Gordon-F force-pushed the android_fix branch from 4f6d159 to a918343 Compare January 13, 2022 23:22

Gordon-F requested review from mockersf and bjorn3 January 14, 2022 10:09

bjorn3 reviewed Jan 14, 2022

View reviewed changes

crates/bevy_core_pipeline/src/lib.rs Outdated Show resolved Hide resolved

Gordon-F force-pushed the android_fix branch 2 times, most recently from facf222 to 11309fd Compare January 15, 2022 21:47

Gordon-F requested a review from bjorn3 January 15, 2022 22:07

alice-i-cecile added this to the Bevy 0.6.1 milestone Jan 15, 2022

DJMcNab reviewed Jan 15, 2022

View reviewed changes

crates/bevy_render/src/renderer/mod.rs Outdated Show resolved Hide resolved

crates/bevy_winit/src/lib.rs Outdated Show resolved Hide resolved

Gordon-F force-pushed the android_fix branch from 11309fd to 1ae78f0 Compare January 16, 2022 20:08

mockersf reviewed Jan 17, 2022

View reviewed changes

Gordon-F added 3 commits January 22, 2022 16:00

Move android example into its own crate

dd9cc43

Add DEFAULT_DEPTH_FORMAT constant

1b7ac51

Get raw window handle only after Resume event

178ff21

Gordon-F force-pushed the android_fix branch from 1ae78f0 to 178ff21 Compare January 22, 2022 13:14

DJMcNab added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Feb 10, 2022

alice-i-cecile removed this from the Bevy 0.6.1 milestone Feb 10, 2022

bjorn3 removed their request for review March 1, 2022 13:45

ManevilleF mentioned this pull request Mar 7, 2022

Fixed android instant crash #4139

Closed

Gordon-F closed this Aug 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get raw window handle only after Resume event #3639

Get raw window handle only after Resume event #3639

Gordon-F commented Jan 11, 2022 •

edited

Loading

alice-i-cecile commented Jan 11, 2022

mockersf commented Jan 11, 2022 •

edited

Loading

Gordon-F commented Jan 11, 2022

bjorn3 commented Jan 12, 2022

Gordon-F commented Jan 12, 2022 •

edited

Loading

Gordon-F commented Jan 14, 2022

Gordon-F commented Jan 15, 2022

DJMcNab left a comment

alice-i-cecile commented Jan 15, 2022 •

edited

Loading

mockersf commented Jan 17, 2022

Gordon-F commented Jan 17, 2022

mockersf Jan 17, 2022

Gordon-F Jan 17, 2022

mockersf Jan 17, 2022 •

edited

Loading

Gordon-F Jan 18, 2022

cart Mar 16, 2022

Gordon-F Apr 14, 2022

cart Apr 14, 2022

Gordon-F Apr 15, 2022

cart Jun 1, 2022

mockersf commented Jan 17, 2022 •

edited

Loading

ManevilleF commented Mar 1, 2022 •

edited

Loading

ManevilleF commented Mar 1, 2022 •

edited

Loading

superdump commented Mar 6, 2022

ManevilleF commented Mar 7, 2022

Gordon-F commented Aug 21, 2022

Get raw window handle only after Resume event #3639

Get raw window handle only after Resume event #3639

Conversation

Gordon-F commented Jan 11, 2022 • edited Loading

Objective

Solution

alice-i-cecile commented Jan 11, 2022

mockersf commented Jan 11, 2022 • edited Loading

Gordon-F commented Jan 11, 2022

bjorn3 commented Jan 12, 2022

Gordon-F commented Jan 12, 2022 • edited Loading

Gordon-F commented Jan 14, 2022

Gordon-F commented Jan 15, 2022

DJMcNab left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Jan 15, 2022 • edited Loading

mockersf commented Jan 17, 2022

Gordon-F commented Jan 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mockersf Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mockersf commented Jan 17, 2022 • edited Loading

ManevilleF commented Mar 1, 2022 • edited Loading

ManevilleF commented Mar 1, 2022 • edited Loading

superdump commented Mar 6, 2022

ManevilleF commented Mar 7, 2022

Gordon-F commented Aug 21, 2022

Gordon-F commented Jan 11, 2022 •

edited

Loading

mockersf commented Jan 11, 2022 •

edited

Loading

Gordon-F commented Jan 12, 2022 •

edited

Loading

alice-i-cecile commented Jan 15, 2022 •

edited

Loading

mockersf Jan 17, 2022 •

edited

Loading

mockersf commented Jan 17, 2022 •

edited

Loading

ManevilleF commented Mar 1, 2022 •

edited

Loading

ManevilleF commented Mar 1, 2022 •

edited

Loading