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.

Sign up for GitHub

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

Jump to bottom

Add Android support to wgpu example #215

Merged

XAMPPRocky merged 9 commits into EmbarkStudios:main from MarijnS95:android

Nov 11, 2020

Contributor

MarijnS95 commented Nov 5, 2020 •

edited

Loading

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-wgpu to 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-ash after #130 lands.

In other good news (also expected) compiling rustc_codegen_spirv on a native aarch64 device (Android phone with chroot) and using it to compile the sky-shader works 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.

MarijnS95 added 2 commits

November 5, 2020 16:45


          examples/wgpu: Handle escape to exit (same as ash example)

bb25d0b


          examples/wgpu: Set up for use on Android

0655b49

khyperia reviewed

View reviewed changes

Contributor

khyperia left a 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") {

Contributor

khyperia Nov 5, 2020

nit: !cfg! is a little gross, could we swap the true/false branches if this statement?

i.e. if cfg!(..) { None } else { Some(..) }

Contributor Author

MarijnS95 Nov 6, 2020

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 👍

examples/runners/wgpu/src/lib.rs

+                      // the resources are properly cleaned up.
+                      let _ = (&instance, &adapter, &module, &pipeline_layout);
+                      *control_flow = ControlFlow::Wait;

Contributor

khyperia Nov 5, 2020

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?

Contributor Author

MarijnS95 Nov 6, 2020 •

edited

Loading

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 with window.request_redraw().

examples/runners/wgpu/src/lib.rs

+                                  WindowEvent::KeyboardInput {
+                                      input:
+                                          KeyboardInput {
+                                              virtual_keycode: Some(VirtualKeyCode::Escape),

Contributor

khyperia Nov 5, 2020

neat, thanks~

examples/runners/wgpu/src/lib.rs Outdated

+                  #[cfg(not(target_arch = "wasm32"))]
+                  {
+                      #[cfg(not(target_os = "android"))]
+                      wgpu_subscriber::initialize_default_subscriber(None);

Contributor

khyperia Nov 5, 2020

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)

Contributor Author

MarijnS95 Nov 6, 2020

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?

Contributor Author

MarijnS95 Nov 6, 2020

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.

XAMPPRocky reviewed

View reviewed changes

examples/runners/wgpu/src/lib.rs Outdated

+                      .build(&event_loop)
+                      .unwrap();
+                  #[cfg(not(target_arch = "wasm32"))]

Member

XAMPPRocky Nov 6, 2020

I would add cfg_if and replace these blocks with that. We're already using it in the ash example.

Contributor Author

MarijnS95 Nov 6, 2020

Oh that's really nice, I didn't know cfg_if existed, thanks for the suggestion!

Member

XAMPPRocky commented Nov 6, 2020

EDIT: Do we want to build-test Android on the CI as well? We can set up something like android-nk-rs.

Definitely

Contributor Author

MarijnS95 commented Nov 6, 2020

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

That's git rename detection going bonkers. I recall it being pretty good at understanding that there was a move from main.rs => lib.rs (with similarity index 98% in the move commit 0655b49) and main.rs reintroduced as a new file, but that's not happening here (anymore?). You can however use copy detection (-C flag on show/diff) so that the output is shown as if lib.rs is a copy from main.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.

Thanks!

EDIT: Do we want to build-test Android on the CI as well?

Definitely

@XAMPPRocky I'll get it set up in the next push round 👍

MarijnS95 added 4 commits

November 6, 2020 12:01


          examples/wgpu: Convert #[cfg] blocks to cfg_if

52bf627


          examples/wgpu: Wait for events instead of busy-looping

7624cb5

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()`.


          examples/wgpu: Create swapchain in ::Resume on Android

daf30e8


          docs: Add Android to supported operating systems

59a9763

MarijnS95 force-pushed the android branch from d4e2ff8 to 91918c4 Compare

November 6, 2020 14:31

XAMPPRocky reviewed

View reviewed changes

.github/workflows/ci.yaml Outdated

@@ @@ -64,6 +63,31 @@ jobs: @@
                       shell: bash
                       run: .github/workflows/test.sh ${{ runner.os }}
+                    - if: runner.os == 'Linux'

Member

XAMPPRocky Nov 6, 2020 •

edited

Loading

Is it worth splitting this out into a separate job so that in the CI UI it says ubuntu, macOS, windows, and android?

Contributor Author

MarijnS95 Nov 6, 2020 •

edited

Loading

Yeah I wasn't sure about this. The build:

Breaks on Mac because of the wrong NDK name;
Breaks on Windows because export is 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?

Contributor Author

MarijnS95 Nov 6, 2020

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

Contributor Author

MarijnS95 Nov 7, 2020

Spacing issue should be solved now, cc rust-mobile/ndk#92 (comment).


          ci: Build test cross-compilation for Android

0726e58

MarijnS95 force-pushed the android branch from c45d9d0 to 0726e58 Compare

November 6, 2020 23:20

MarijnS95 added 2 commits

November 7, 2020 15:50


          HACK: ci: Create Android symlink without spaces

56a6343


          ci: Set legacy ANDROID_HOME because ndk-build prefers deprecated var

e72a2e1

XAMPPRocky added the s: waiting on review label

khyperia approved these changes

View reviewed changes

Contributor

khyperia left a comment

🎉

XAMPPRocky merged commit e8aef14 into EmbarkStudios:main

MarijnS95 deleted the android branch

November 11, 2020 15:32

DJMcNab added a commit to DJMcNab/rust-gpu that referenced this pull request


          Fix the output filename collision warning

7200e08

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

DJMcNab mentioned this pull request

Fix the output filename collision warning #669

Merged

khyperia pushed a commit that referenced this pull request


          Fix the output filename collision warning (#669)

e66e72b

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

s: waiting on review