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 eframe example #14

Closed
wants to merge 1 commit into from
Closed

Add eframe example #14

wants to merge 1 commit into from

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Aug 12, 2022

I tried to make eframe work again after fixing compile errors in my egui branch. It currently results in rendering just a black application. I didn't spot anything wrong in eframe however.

@rib
Copy link
Member

rib commented Aug 12, 2022

ah, yeah - it would be nice to get eframe working. when I first added the egui winit/wgpu support for android I just did the bare minimum to have eframe compile while it looked like it would take a bit more refactoring to ensure the wgpu state initialization could be deferred until the first resume - and I guessed it might affect the public api.

I'll try to get a chance to look at your branch more closely at some point, but yeah I'd expect the main challenge is with deferring the painter initialization until the first time the app is resumed.

@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 12, 2022

I have had a previous eframe version working with android-ndk-rs with the wgpu backend. The Painter::set_window call is already deferred there.

@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 20, 2022

Parts of my egui branch landed in egui in emilk/egui#1900 leaving just the android-activity glue.

@rib
Copy link
Member

rib commented Aug 20, 2022

Cool, so does this work now - I had the impression this wasnt working yet?

@rib
Copy link
Member

rib commented Aug 20, 2022

just gave this a quick try, pulling eframe from git = "https://github.com/Zoxc/egui", branch = "eframe-android" but at the moment the app isn't running - haven't investigated yet though.

@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 20, 2022

I haven't found the cause of the lack of rendering, so I wouldn't expect it to work yet, but at least eframe should keep compiling now.

@rib
Copy link
Member

rib commented Aug 20, 2022

I'm currently getting a crash with this backtrace:

std::sys::unix::abort_internal::hf3f0057594b51fb7 at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/sys/unix/mod.rs:259
rust_panic at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/panicking.rs:748
std::panicking::rust_panic_with_hook::h95dad99d0f37c0dc at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/panicking.rs:716
std::panicking::begin_panic_handler::_$u7b$$u7b$closure$u7d$$u7d$::h606a10048a0e7a24 at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/panicking.rs:588
std::sys_common::backtrace::__rust_end_short_backtrace::hf0cea5e40e468662 at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/sys_common/backtrace.rs:138
rust_begin_unwind at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/panicking.rs:584
core::panicking::panic_fmt::h1b2b89cbefe20785 at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/panicking.rs:142
core::panicking::panic_display::h6bf78c3b6fcda5d6 at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/panicking.rs:72
core::panicking::panic_str::h1dcf248cb499bb76 at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/panicking.rs:56
core::option::expect_failed::h58a001997f856392 at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/option.rs:1854
core::option::Option$LT$T$GT$::expect::h5cb902943fac1100 at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/option.rs:718
winit::platform_impl::platform::EventLoop$LT$T$GT$::new::h62b58da8766f195c at C:\Users\Robert\.cargo\git\checkouts\winit-0cced2578ff7cf1c\ac94bb3\src\platform_impl\android/mod.rs:316
winit::event_loop::EventLoopBuilder$LT$T$GT$::build::ha1b4c37576b99330 at C:\Users\Robert\.cargo\git\checkouts\winit-0cced2578ff7cf1c\ac94bb3\src/event_loop.rs:114
eframe::native::run::with_event_loop::EVENT_LOOP::__init::h226ae7735043ed52 at C:\Users\Robert\.cargo\git\checkouts\egui-9a58596941f2023d\831476f\eframe\src\native/run.rs:84
eframe::native::run::with_event_loop::EVENT_LOOP::__getit::_$u7b$$u7b$closure$u7d$$u7d$::h1d0ee934ca2003eb at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/thread/local.rs:353
std::thread::local::lazy::LazyKeyInner$LT$T$GT$::initialize::h7ca74c26f53e3294 at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/thread/local.rs:809
std::thread::local::os::Key$LT$T$GT$::try_initialize::h86fcd097292233c0 at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/thread/local.rs:1119
std::thread::local::os::Key$LT$T$GT$::get::hb838fc0154142fea at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/thread/local.rs:1086
eframe::native::run::with_event_loop::EVENT_LOOP::__getit::h852bf193f90f8cdd at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/thread/local.rs:345
std::thread::local::LocalKey$LT$T$GT$::try_with::hd11ae67e907b08c1 at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/thread/local.rs:444
std::thread::local::LocalKey$LT$T$GT$::with::h4cdbb4a6dd8bb7e8 at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/thread/local.rs:421
eframe::native::run::with_event_loop::h32440aeb2b74d149 at C:\Users\Robert\.cargo\git\checkouts\egui-9a58596941f2023d\831476f\eframe\src\native/run.rs:86
eframe::native::run::wgpu_integration::run_wgpu::h56b9e2d0cbc53c25 at C:\Users\Robert\.cargo\git\checkouts\egui-9a58596941f2023d\831476f\eframe\src\native/run.rs:722
eframe::run_native::hc0367fc8d9f31eaa at C:\Users\Robert\.cargo\git\checkouts\egui-9a58596941f2023d\831476f\eframe\src/lib.rs:177
main::_main::h773189bed7b69f8c at C:\Users\Robert\src\android-activity\examples\agdk-eframe/src/lib.rs:54
android_main at C:\Users\Robert\src\android-activity\examples\agdk-eframe/src/lib.rs:64
_rust_glue_entry at C:\Users\Robert\src\android-activity\android-activity\src\game_activity/mod.rs:617
android_app_entry at C:\Users\Robert\src\android-activity\android-activity/game-activity-csrc/game-activity/native_app_glue/android_native_app_glue.c:218

@rib
Copy link
Member

rib commented Aug 20, 2022

I'm sort of half expecting there to still be a bit of work to do in eframe for Android too. Even though eframe knows to call Painter::set_window on Resume for android, I guess eframe still tries to initialize wgpu and the render surface up-front instead of deferring this until the first Resume.

@rib
Copy link
Member

rib commented Aug 20, 2022

ooh, just realised I haven't pushed my latest winit backend branch too

@rib
Copy link
Member

rib commented Aug 20, 2022

oh, the panic is from let android_app = attributes.android_app.as_ref().expect("An AndroidAppas passed to android_main() is required to create anEventLoop on Android"); - though it looked like your eframe branch did have an option for the android_app which I guess already passes this to winit.

@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 20, 2022

That crash seems to be because NativeOptions::run_and_return is not false. That's a new feature which is on by default.

@rib
Copy link
Member

rib commented Aug 20, 2022

That crash seems to be because NativeOptions::run_and_return is not false. That's a new feature which is on by default.

okey, yeah, as a hack I've set run_and_return = false

@rib
Copy link
Member

rib commented Aug 20, 2022

okey, yeah, so now I get the black screen like you originally described.

My guess, as mentioned above is that eframe somehow needs to be able to defer graphics state initialization until the first resume.

One of the changes recently landed in winit was to ensure that Resumed events are now delivered consistently on all platforms, so it may be possible to get eframe to handle its initialization consistently based on Resumed events: rust-windowing/winit#2331

@rib
Copy link
Member

rib commented Aug 20, 2022

btw for reference I've pushed a version of your branch here: https://github.com/rib/android-activity/tree/eframe

I've did a bit of renaming of the example from agdkegui to agdkeframe just so it doesn't clash with the other example for now, and updated the Cargo.toml to point at git branches instead of local paths plus the hack for run_and_return.

@rib
Copy link
Member

rib commented Aug 20, 2022

Yeah this WpuWinitApp::new() code in eframe/src/native/run.rslooks like it probably needs re-working:

impl WgpuWinitApp {
        fn new(
            event_loop: &EventLoop<RequestRepaintEvent>,
            app_name: &str,
            native_options: &epi::NativeOptions,
            app_creator: epi::AppCreator,
        ) -> Self {
            let storage = epi_integration::create_storage(app_name);
            let window_settings = epi_integration::load_window_settings(storage.as_deref());

            let window = epi_integration::window_builder(native_options, &window_settings)
                .with_title(app_name)
                .build(event_loop)
                .unwrap();

            // SAFETY: `window` must outlive `painter`.
            #[allow(unsafe_code, unused_mut, unused_unsafe)]
            let painter = unsafe {
                let mut painter = egui_wgpu::winit::Painter::new(
                    wgpu::Backends::PRIMARY | wgpu::Backends::GL,
                    wgpu::PowerPreference::HighPerformance,
                    wgpu::DeviceDescriptor {
                        label: None,
                        features: wgpu::Features::default(),
                        limits: wgpu::Limits::default(),
                    },
                    wgpu::PresentMode::Fifo,
                    native_options.multisampling.max(1) as _,
                );
                #[cfg(not(target_os = "android"))]
                painter.set_window(Some(&window));
                painter
            };

            let wgpu_render_state = painter.render_state();

@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 20, 2022

Do you see anything wrong? I've tested that you can create the window and painter prior to the resumed event.

@rib
Copy link
Member

rib commented Aug 20, 2022

there is no SurfaceView / NativeWindow before resume so most of the eframe setup needs to wait until the first resume. e.g. where eframe will try and query the supported texture size, depends on a correctly initialized graphics context, which itself requires a render surface to have been provided, which requires a SurfaceView on android.

So essentially I think the Epi Integration and App need to be first created when the app first resumes.

I started taking a look at this but also finding that vscode + rust-analyzer really seems to struggle with the egui repo which makes it kinda frustrating. :/

@rib
Copy link
Member

rib commented Aug 20, 2022

making sure rust-analyzer isn't cross compiling for android seems to help a lot with the egui repo

@rib
Copy link
Member

rib commented Aug 20, 2022

Okey, I've just pushed a first draft update for eframe that defers render initialization until the Resumed event: https://github.com/rib/egui/tree/eframe-android

With that branch I'm seeing that this eframe example now runs on Android

@rib
Copy link
Member

rib commented Aug 20, 2022

For the sake of only tackling one thing at a time I commented out the "glow" support in eframe, which basically needs to be updated in more-or-less the same way. What's slightly more awkward with Glow is that the Glutin API seems to tightly couple creating a window and a GL context which is overly restrictive and means that using Glutin on Android will end up recreating the whole GL context every time it suspends and resumes which isn't necessary, but it doesn't look like Glutin lets you create a context without a window.

@rib
Copy link
Member

rib commented Aug 20, 2022

Just for reference here I ended up filing this issue with Glutin to try and ask for some pointers about how the API can be used to create a context that isn't tightly coupled to the window surface, so that it's possible to destroy and create windows over the lifetime of a single context: rust-windowing/glutin#1442

This question is currently blocking me from cleaning up the eframe changes for Android because I'm not sure atm how to do the equivalent with the Glutin API.

I guess if I can't find a nice solution for glow/glutin I may be able to leave the existing glow backend for eframe essentially untouched, but I'm not sure if that might make it more awkward to get a PR accepted since the eframe code for glow vs wgpu will diverge quite significantly while the backends are currently almost identical.

@rib
Copy link
Member

rib commented Aug 20, 2022

For reference here I just got a pointer to a WIP PR for Glutin that will actually address the design issue mention above by introducing a separate Surface API that will make it possible to manage surfaces separately from the context. I don't know of any ETA for that work at the moment - since it still needs several backends to finished - but it's promising at least that there's a plan to evolve the Glutin API in a direction that would help here.

Will have to think about how to handle the eframe glow backend in the short term though.

Update: forgot to actually add the link :) rust-windowing/glutin#1435

@rib
Copy link
Member

rib commented Aug 20, 2022

For reference, I've cleaned up the egui/eframe changes I made here: https://github.com/rib/egui/tree/eframe-android including updating the glow integration (though glow still won't support suspend/resume on Android).

@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 21, 2022

I tried your egui branch and on my phone it works initially, but now turns black if I switch to another app and back.

@rib
Copy link
Member

rib commented Aug 21, 2022

Ah, okey, will need to investigate that. I didn't test the code much myself so far, so hopefully just somthing silly wrong with the logic for dropping and recreating the window + surface state on suspend / resume.

@rib
Copy link
Member

rib commented Aug 21, 2022

I wonder if there might also be some other general design issues with eframe atm that could interfere with Android re-creating the window on resume, e.g other platforms don't currently support creating multiple windows: emilk/egui#1918

@rib
Copy link
Member

rib commented Aug 21, 2022

oh at the very least it looks like the drop_window function isn't being used as expected currently - probably after the cleanup re-factor I did:

#[cfg(target = "android")]
        fn drop_window(&mut self) {
            tracing::debug!("WgpuWinit: dropping window");
            self.window = None;
            if let Some(running) = &mut self.running {
                tracing::debug!("WgpuWinit: painter dropping window");
                unsafe {
                    self.painter.set_window(None);
                }
            }
        }

There is no self.painter any more so this wouldn't even compile

@rib
Copy link
Member

rib commented Aug 21, 2022

urgh, I had written #[cfg(target = "android")] instead of #[cfg(target_os = "android")] in various places :-/

It's slightly disappointing that the Rust compiler can't do any kind of sanity check for that kind of mistake

@rib
Copy link
Member

rib commented Aug 21, 2022

just force pushed some fixes for s/target/target_os/ and smoke testing I'm seeing that I can switch back and forth to the app and it continues working.

I do see that input isn't currently working properly though, like it's not handling touch input well. I see that the check box updates as if a mouse was hovering over it but it doesn't actually toggle. Its like it's treating touch input like pointer movement and not emulating click events.

@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 21, 2022

The checkbox is not supposed to toggle.

@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 21, 2022

I've tested that touch works and switching to and from also works now. Getting the keyboard to show up would be the next step. Not sure which crate should be responsible for that.

@rib
Copy link
Member

rib commented Aug 21, 2022

Yeah it would be good to get IME text input working with Egui on android. It's something I've been meaning to poke at but haven't had a chance.

I think the place to start here is implementing support for Winit's IME in the Android backend, which can utilize the game-input-text library that's part of AGDK.

On Android the soft keyboard is basically an IME method - handled in much the same way as input methods for languages like Chinese and Japanese for desktop window systems. One notable difference is that on Android the UI needs to be the thing that adjusts according to the space taken up by the input method - whereas on desktop window systems they inform the input method where to draw. Something else different is that on Android input methods expect to be able to manage a selection region separate from a completion region for the text being edited but the Winit abstraction currently only supports tracking a single region. (I'm guessing that neither of these issues should block us from getting at least basic input method support working on Android with the existing Winit abstraction).

@rib
Copy link
Member

rib commented Aug 21, 2022

In terms of "which crate" - I'm guessing there will be a bit of work required in both the android-activity crate plus the Winit android backend. It also sort of assumes the Egui already has some basic support for input methods - otherwise it will also need some integration in Egui too.

@rib
Copy link
Member

rib commented Aug 21, 2022

For some context, this was the PR that added IME support to Winit for 0.27: rust-windowing/winit#2243

@rib
Copy link
Member

rib commented Aug 21, 2022

The checkbox is not supposed to toggle.

Ah, okey - fair enough. It could be good if we can update this example before landing to be based on the standard demo lib that egui has - similar to the agdk-egui example.

@rib
Copy link
Member

rib commented Aug 21, 2022

See also here for a bit of context about how GameTextInput is supposed to work with GameActivity: https://developer.android.com/games/agdk/game-activity/use-text-input

Note: the android-activity crate already builds game-text-input but there's currently no public Rust API exposed for it

@rib
Copy link
Member

rib commented Aug 21, 2022

I've just created a separate issue to track the need for input method support in android-activity: https://github.com/rib/android-activity/issues/18

@rib
Copy link
Member

rib commented Aug 21, 2022

Btw, @Zoxc, I'm thinking of trying to re-work a bit the eframe stuff to avoid the need for any android-activity specific integration in egui. I think in the short term that will make it harder to try and upstream the changes for supporting lazy initialization of render state and in general I think it's best to keep the number of crates that need to directly depend on android-activity (or similarly ndk-glue) to an absolute minimum.

Due to the important requirement that there can only be a single implementation of an Android glue crate linked into an application then the fewer things that have an explicit dependency then the less risk of versioning conflicts.

I'm currently hoping that we can instead come up with an API that lets applications (optionally) provide their own Winit EventLoopBuilder to eframe. That can hopefully be a more general purpose escape hatch that will allow applications to deal with platform specific event loop configuration, such as is needed with android-activity.

@rib
Copy link
Member

rib commented Aug 21, 2022

I've just pushed an update that adds a more general mechanism (via NativeOptions) for applications to configure the event loop which allows an android-activity based application to do:

#[cfg(target_os = "android")]
#[no_mangle]
fn android_main(app: AndroidApp) {
    use winit::platform::android::EventLoopBuilderExtAndroid;

    android_logger::init_once(android_logger::Config::default().with_min_level(Level::Trace));

    let mut options = NativeOptions::default();
    options.event_loop_builder = Some(Box::new(move |builder| {
        builder.with_android_app(app);
    }));
    _main(options);
}

@rib
Copy link
Member

rib commented Aug 21, 2022

I've just rebased the eframe changes up to the last commit before 0.19 was released and pushed a branch here: https://github.com/rib/egui/tree/eframe-android-0.18

@rib
Copy link
Member

rib commented Aug 21, 2022

For reference here I've just opened this PR for egui/eframe: emilk/egui#1952

@rib
Copy link
Member

rib commented Aug 24, 2022

Okey, I've just pushed an update of this patch to add an eframe example (kept your attribution + added a co-authored-by tag).

I updated the example to run the egui_demo_lib example, similar to agdk-egui.

I also investigated the issue with the app rendering continuously which was noted here: emilk/egui#1952 (comment) and found that this was due to a bug in the Winit backend which was never clearing the flag for pending_redraw (I'll hopefully push a fix for that soon).

I wasn't quite sure about the other issue noted here about the app destructor. One thing to note is that I don't think Android guarantees that your app will get an onDestroy callback when an app is closed since the OS might just kill the process. Maybe this is what you were seeing. I think the SaveState callback should be reliable but unfortunately that's not exposed by Winit currently.

@rib rib closed this Aug 24, 2022
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.

2 participants