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

Misc fixes #8

Merged
merged 4 commits into from
Aug 11, 2022
Merged

Misc fixes #8

merged 4 commits into from
Aug 11, 2022

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Aug 9, 2022

Just various fixes which enables agdk-egui to build for me. It does not work in the Android Emulator however.

@rib
Copy link
Collaborator

rib commented Aug 10, 2022

Thanks for this, I'll aim to take a closer look at this.

One thing I'm initially unsure about from skimming the changes is the introduction of the wgpu 0.13 dependency and updating the egui dependency.

There has been a bit of a balancing act with versions recently since the raw_window_handle crate was updated with some breaking API changes which have been adopted by the ndk crate (rust-mobile/ndk@814be08) and also adopted in winit 0.27 but had not yet been picked up by wgpu - which creates an awkward kind of incompatibility schism among crates.

I had so far only brought dependencies for android-activity forward up to the point before the raw_window_handle 0.5 crate was introduced because that's a breaking API change that has a knock-on effect for lots of other crates (and at the time no PR had been opened yet to support raw_window_handle 0.5 in wgpu)

Just checking quickly it looks support has been merged into wgpu master but it sounds like this dependency bump won't now be released until around late September: gfx-rs/wgpu#2918 (comment).

The lack of new wgpu release will in turn affect what egui version we need to continue depending on. I guess we'll probably needing to stick with a git commits until wgpu 0.14 and egui 0.19/0.20 are released.

@rib
Copy link
Collaborator

rib commented Aug 10, 2022

For reference this is the PR for egui to bump the winit dependency to 0.27 which is not currently merged and is either blocked on wgpu 0.14 being released or else finding some way for egui to be able to work with winit 0.26 and 0.27

For now it looks like that implies we have to stick with a git commit for the ndk crate before it was bumped to raw_window_handle 0.5 - otherwise we also have to jump to winit 0.27 which isn't supported by egui yet.

That said I think we should still be able to bump the wgpu dependency to 0.13 as in this PR, since that still only depends on winit 0.26.

@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 10, 2022

The egui version should probably be pinned so it keeps working, currently master uses wgpu 0.13 so I changed it to match.

One thing that could be improved is to change your winit branch to point to the published android-activity and change the examples to patch it to the local version. Currently I needed to check out the winit repo and point it to the local android-activity crate. The published android-activity should work without the ndk git version if you're using game-activity right?

@rib
Copy link
Collaborator

rib commented Aug 10, 2022

Yeah, there's a bit of a tangle at the moment with versions, esp relating to winit and wgpu.

Here's an issue + PR I worked on today to try any improve this situation, which will hopefully help here too: rust-windowing/winit#2418

The branch I have for winit is also wrapped up in this issue, since that's currently also blocked from moving beyond the raw_window_handle change - considering that egui is currently stuck using winit 0.26 until wgpu becomes compatible with winit 0.27.

btw, I have an updated version of the winit branch that's been rebased but it looks like I didn't push it yet - I need to check if there was still something I need to address with the rebase - I'll give that a look now (I might have just left it to wait and see about these winit versioning issues since probably wasn't sure how quickly the issue with wgpu might be resolved)

Yeah, you're right that egui needs to be pinned in this repo. I currently just have it locked locally and forgot to pin. I think because it's in a workspace you get forced to move patches up into the workspace Cargo.toml which also makes the pinning less obvious than it ideally would be, so it may also make sense to add some README notes about all this.

@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 10, 2022

I would probably prefer android-activity-winit-0.26 and android-activity-winit-0.27 branches on your winit fork to make it clear which version they're based on and allowing patching of both versions.

@rib
Copy link
Collaborator

rib commented Aug 11, 2022

I would probably prefer android-activity-winit-0.26 and android-activity-winit-0.27 branches on your winit fork to make it clear which version they're based on and allowing patching of both versions.

My hope at the moment is that we can land rust-windowing/winit#2418 in Winit asap and then if a 0.27.2 release gets spun then I think we can just move ahead to Winit 0.27 and not be blocked by the raw_window_handle upgrade.

@rib rib merged commit b07717d into rust-mobile:main Aug 11, 2022
@rib
Copy link
Collaborator

rib commented Aug 11, 2022

Okey, even though it spawned all these other comments about the current versioning tangle we have, this PR in itself wasn't really interacting with any of that. I'll hopefully follow up based on some of the discussion, but that can be done separately.

Thanks for the PR!

@rib
Copy link
Collaborator

rib commented Aug 11, 2022

For reference here, I've now update android-activity to use ndk 0.7, ndk-sys 0.4 and winit 0.27 (with an android-activity-0.27 branch for Winit here: https://github.com/rib/winit/tree/android-activity-0.27)

My Winit branch also includes my patch from rust-windowing/winit#2418 which makes it compatible with wgpu 0.13 and the agdk-winit-wgpu example has been updated to use wgpu 0.13.

While egui hasn't yet merged it's support for Winit 0.27 into master then agdk-egui currently points at the branch for the PR that adds Winit 0.27 support.

I've smoke tested that agdk-egui and agdk-winit-wgpu both work on a Samsung Galaxy S8+ with these changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants