Add Android support to wgpu example#215
Conversation
khyperia
left a comment
There was a problem hiding this comment.
For others reviewing this, github's UI is a little annoying to see the actual changes due to the moved file. If you check out this branch on your local repo, though, this will show you the diff:
git diff origin/main:examples/runners/wgpu/src/main.rs HEAD:examples/runners/wgpu/src/lib.rs
this looks hecking fantastic! pseudo-approval from me, just want to give you a chance to respond to the nits before mergify does its thing.
examples/runners/wgpu/src/lib.rs
Outdated
|
|
||
| // Wait for Resumed event on Android; the surface is only needed early to | ||
| // find an adapter that can render to this surface. | ||
| let mut surface = if !cfg!(target_os = "android") { |
There was a problem hiding this comment.
nit: !cfg! is a little gross, could we swap the true/false branches if this statement?
i.e. if cfg!(..) { None } else { Some(..) }
There was a problem hiding this comment.
Yeah I noticed it looks weird this way. I'm a fan of keeping else { None }; as the last block but that doesn't really work out here, unless using cfg!(not(target_os = "android")). That's more characters as well so I'll flip them around 👍
| // the resources are properly cleaned up. | ||
| let _ = (&instance, &adapter, &module, &pipeline_layout); | ||
|
|
||
| *control_flow = ControlFlow::Wait; |
There was a problem hiding this comment.
I have no idea what practical differences are (just what the raw functionality does), is there a reason you changed this from Poll to Wait?
There was a problem hiding this comment.
That's explained in the relevant commit making this change: 54752c2
The image currently does not change and the OS will notify us when to
redraw (ie. after window resizes). This is going to save power
especially on mobile devices.As soon as interactive or animating visuals are introduced to this
example redraws can be requested withwindow.request_redraw().
| WindowEvent::KeyboardInput { | ||
| input: | ||
| KeyboardInput { | ||
| virtual_keycode: Some(VirtualKeyCode::Escape), |
examples/runners/wgpu/src/lib.rs
Outdated
| #[cfg(not(target_arch = "wasm32"))] | ||
| { | ||
| #[cfg(not(target_os = "android"))] | ||
| wgpu_subscriber::initialize_default_subscriber(None); |
There was a problem hiding this comment.
looks like wgpu_subscriber is not used on android, could we cfg it out for android in Cargo.toml? (it's already cfg'd out for wasm32)
There was a problem hiding this comment.
This caused a crash on-device but I haven't looked at the problem yet. Would be nice to have some form of tracing when necessary I guess?
There was a problem hiding this comment.
The problem turns out to be wgpu attaching a log tracer which conflicts with the Android logger if set up. That Android logger has intentionally been left out as we do not print any logs anywhere in this example, and we'd need to wait for rust-mobile/ndk#88 to land in order to not clutter Cargo.toml with some "unused" crate.
examples/runners/wgpu/src/lib.rs
Outdated
| .build(&event_loop) | ||
| .unwrap(); | ||
|
|
||
| #[cfg(not(target_arch = "wasm32"))] |
There was a problem hiding this comment.
I would add cfg_if and replace these blocks with that. We're already using it in the ash example.
There was a problem hiding this comment.
Oh that's really nice, I didn't know cfg_if existed, thanks for the suggestion!
Definitely |
That's git rename detection going bonkers. I recall it being pretty good at understanding that there was a move from
Thanks!
@XAMPPRocky I'll get it set up in the next push round 👍 |
The image currently does not change and the OS will notify us when to redraw (ie. after window resizes). This is going to save power especially on mobile devices. As soon as interactive or animating visuals are introduced to this example redraws can be requested with `window.request_redraw()`.
.github/workflows/ci.yaml
Outdated
| shell: bash | ||
| run: .github/workflows/test.sh ${{ runner.os }} | ||
|
|
||
| - if: runner.os == 'Linux' |
There was a problem hiding this comment.
Is it worth splitting this out into a separate job so that in the CI UI it says ubuntu, macOS, windows, and android?
There was a problem hiding this comment.
Yeah I wasn't sure about this. The build:
- Breaks on Mac because of the wrong NDK name;
- Breaks on Windows because
exportis not a thing there.
The former one should be fixed now soon, the latter one I don't immediately know how to resolve (without writing separate scripts for separate os'es). Does Windows understand $HOME as used elsewhere in this file, shouldn't that be %UserProfile%?
The main reason it' stuffed in check is to leverage existing setup and build artifacts: is that okay or is there a better solution in the pipeline with the changes discussed yesterday?
There was a problem hiding this comment.
Should be fixed now: we don't need any of these ugly download steps as the GH Actions image comes with the NDK preinstalled 🎉
Still an issue with spaces in paths on Windows, which I hope to get to the bottom of here: rust-mobile/ndk#92
There was a problem hiding this comment.
Spacing issue should be solved now, cc rust-mobile/ndk#92 (comment).
This works simply by naming the binary crate anything other than the name of the lib, which is example-runner-wgpu As far as I know, the warning started in EmbarkStudios#215 Since there is only one binary crate in the package, the command (`cargo run -p example-runner-wgpu --release`) maintains the same behaviour The cargo issue is rust-lang/cargo#6313 This warning caused problems for me in testing Lokathor/bytemuck#67 since I didn't notice the warning that my patch was not applied
This works simply by naming the binary crate anything other than the name of the lib, which is example-runner-wgpu As far as I know, the warning started in #215 Since there is only one binary crate in the package, the command (`cargo run -p example-runner-wgpu --release`) maintains the same behaviour The cargo issue is rust-lang/cargo#6313 This warning caused problems for me in testing Lokathor/bytemuck#67 since I didn't notice the warning that my patch was not applied
Closes: #190
As expected cross-compiling rust-gpu for Android works just fine 🎉, tested on both 10 and 11. This PR introduces the necessary changes to
example-runner-wgputo make it deal with the (imo rather obnoxious) suspend-resume handling. But hey, at least you can switch between apps without it breaking! Don't ask for the "back" button to work, though 😅The same winit changes can be applied to
example-runner-ashafter #130 lands.In other good news (also expected) compiling
rustc_codegen_spirvon a native aarch64 device (Android phone with chroot) and using it to compile thesky-shaderworks OOTB; C++ spirv-tools didn't even make it break a sweat 🙂It is unfortunately impossible to do this from termux yet (for the portable devs) as there are no nightlies available for
aarch64-linux-android; manually attempting to compile this is a can of worms I'd rather leave closed.EDIT: Do we want to build-test Android on the CI as well? We can set up something like android-nk-rs.