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

Add egui example #48

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Add egui example #48

wants to merge 12 commits into from

Conversation

zeerooth
Copy link
Contributor

@zeerooth zeerooth commented Oct 25, 2021

This example is (partially) functional right now, at least on Android, but there are a couple of bugs and QOL stuff that I think need to be addressed before merging this PR:

  • It is currently borked on desktop (unresponsive and resize fails with "Dropped frame with error: The underlying surface has changed, and therefore the swap chain must be updated")
  • iOS tests (I don't have such device, can somebody with one test this, pretty please? :3)
  • Virtual keyboard not being displayed on text fields (may need to be fixed upstream) (update: yeah, to handle the virtual keyboard input, composition events for Android need to be implemented in winit first, therefore this is currently unfixable)
  • Weird workaround with waiting for NativeActivity
  • Weird resolution at the start, which is fixed when rotating the screen (edit: I found out that the resolution is actually set correctly, but the scale_factor of winit::Window is incorrectly reported as 1.0 at the start. This was fixed in this PR for winit but we still need to wait for a full winit release and for new winit version to be changed in egui_winit soo untill that I guess this PR is blocked :x ) (fixed!)
  • Crash when leaving the app in the background
  • Status bar overlays the app which leads to some content not being interactable (fixed by enabling fullscreen by default)
  • Update egui_wgpu_backend version when a new one is published with egui 0.15

@zeerooth zeerooth added status: upstream It's not our fault for once type: enhancement Wouldn't this be the coolest? labels Nov 2, 2021
@zeerooth zeerooth added status: needs review A maintainer must review this code and removed status: upstream It's not our fault for once labels Jan 3, 2022
@zeerooth
Copy link
Contributor Author

zeerooth commented Jan 3, 2022

I think this PR is ready to be merged now - I still haven't tested this on iOS (because I lack the capacity to) and the virtual keyboard is not working, but it's dependant or future changes in winit. However, the rest of the functionalities in the demo seem to be working fine!

@zeerooth zeerooth marked this pull request as ready for review January 3, 2022 20:29
Copy link
Contributor

@francesca64 francesca64 left a comment

Choose a reason for hiding this comment

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

Thanks for all the great work! This all looks good, so I'll test this on Android and iOS and report back.

@@ -3,6 +3,7 @@
<!-- Base application theme. -->
<style name="AppTheme" parent="android:Theme.Material.Light.DarkActionBar">
<!-- Customize your theme here. -->
<item name="android:windowFullscreen">true</item>
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's so that apps are in fullscreen by default and the status bar doesn't overlay the app's content (which I've seen was also an issue in other examples, such as the wgpu one)

@francesca64
Copy link
Contributor

iOS tests (I don't have such device, can somebody with one test this, pretty please? :3)

Here I go!

Oh...

error: Only Windows, Mac OS, Linux, *BSD and Haiku and Wasm32 are currently supported
   --> /Users/francesca/.cargo/registry/src/github.com-1ecc6299db9ec823/webbrowser-0.5.5/src/lib.rs:232:1
    |
232 | compile_error!("Only Windows, Mac OS, Linux, *BSD and Haiku and Wasm32 are currently supported");
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

It doesn't look there's any upstream traction on this. If you'd like, you can just mark iOS as unsupported with a comment explaining why:

#[serde(default = "default_true")]
supported: bool,

@zeerooth
Copy link
Contributor Author

zeerooth commented Feb 4, 2022

iOS tests (I don't have such device, can somebody with one test this, pretty please? :3)

Here I go!

Oh...

error: Only Windows, Mac OS, Linux, *BSD and Haiku and Wasm32 are currently supported
   --> /Users/francesca/.cargo/registry/src/github.com-1ecc6299db9ec823/webbrowser-0.5.5/src/lib.rs:232:1
    |
232 | compile_error!("Only Windows, Mac OS, Linux, *BSD and Haiku and Wasm32 are currently supported");
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

It doesn't look there's any upstream traction on this. If you'd like, you can just mark iOS as unsupported with a comment explaining why:

#[serde(default = "default_true")]
supported: bool,

I managed to fix that by disabing the default features for egui-winit (I seems they are not required for the demo so it should be safe to do so) and the build for aarch64-apple-ios managed to pass that step

Can you please try again now @francesca64 ?

@francesca64
Copy link
Contributor

Now it builds fine, but crashes immediately:

-[MTLCompileOptionsInternal setPreserveInvariance:]: unrecognized selector sent to instance 0x2811be040

This originates down in the metal-rs crate: https://github.com/gfx-rs/metal-rs/blob/140c8f4e39001ae154f153ffc767da6c0c9d7f06/src/library.rs#L572-L575

The device I'm testing on is running iOS 13.1.2, which should support that, as far as I can tell.

This was added to wgpu quite recently: gfx-rs/wgpu#2372

The equivalent logic in MoltenVK dynamically checks if the selector is supported, as opposed to checking the version like in the wgpu PR, which could be significant: https://github.com/KhronosGroup/MoltenVK/blob/101ea9eec158717c38c448d313d3c6452f8b327c/MoltenVK/MoltenVK/GPUObjects/MVKDevice.mm#L3781-L3783

@ArthurKValladares can you try to run this too? I'm curious if this is device-dependent.

@ArthurKValladares
Copy link
Contributor

Runs fine on my device on iOS 15.2.1.

@zeerooth
Copy link
Contributor Author

Now it builds fine, but crashes immediately:

-[MTLCompileOptionsInternal setPreserveInvariance:]: unrecognized selector sent to instance 0x2811be040

This originates down in the metal-rs crate: https://github.com/gfx-rs/metal-rs/blob/140c8f4e39001ae154f153ffc767da6c0c9d7f06/src/library.rs#L572-L575

The device I'm testing on is running iOS 13.1.2, which should support that, as far as I can tell.

This was added to wgpu quite recently: gfx-rs/wgpu#2372

The equivalent logic in MoltenVK dynamically checks if the selector is supported, as opposed to checking the version like in the wgpu PR, which could be significant: https://github.com/KhronosGroup/MoltenVK/blob/101ea9eec158717c38c448d313d3c6452f8b327c/MoltenVK/MoltenVK/GPUObjects/MVKDevice.mm#L3781-L3783

@ArthurKValladares can you try to run this too? I'm curious if this is device-dependent.

Thanks for investigating this, I definitely wouldn't have figured out the root cause of that issue.

What can we do about it though? Since that fix has been added to wgpu recently, could you please try using the wgpu from the master branch instead of the latest 0.12 release on creates.io that doesn't contain that fix and see if the template works with it?

@CutestNekoAqua
Copy link

this work is probably already out of date at this point, but can I help get this merged? as I need it for my project anyways and Id so like to contribute back to this project!

@zeerooth
Copy link
Contributor Author

this work is probably already out of date at this point, but can I help get this merged? as I need it for my project anyways and Id so like to contribute back to this project!

Oh wow, I didn't think somebody would need this :D

I don't know if you're aware but this project isn't really maintained much anymore, you're probably better off using https://github.com/tauri-apps/tauri-mobile which is a more active fork. If you need egui I can port this PR over there and try to get it merged. Just give me a day or two.

@CutestNekoAqua
Copy link

Oh wow, I didn't think somebody would need this :D

Yes! My autism wants to create a mobile app in Rust and egui is what we already are used to and can use efficiently

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs review A maintainer must review this code type: enhancement Wouldn't this be the coolest?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants