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.

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

Compiling on windows for android errors #1886 #2118

Closed
wants to merge 13 commits into from
Closed

Compiling on windows for android errors #1886 #2118

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
87ed5e5
Fixed android crash on startup
Kaiser1989 Jul 10, 2020
828496d
Removed event println
Kaiser1989 Jul 14, 2020
5675b1b
Fixed android, changed to local repository
Kaiser1989 Jul 14, 2020
15627c2
Fixed android crash on startup
Kaiser1989 Jul 10, 2020
00d1e3a
Removed event println
Kaiser1989 Jul 14, 2020
708f788
Fixed android, changed to local repository
Kaiser1989 Jul 14, 2020
d8a3f00
Merge branch 'master' of https://github.com/Kaiser1989/winit
Kaiser1989 Aug 6, 2020
0257d6e
Added destroy event to android (when calling native_activity::finish)
Kaiser1989 Aug 7, 2020
c72f267
Added back key, all others are none
Kaiser1989 Aug 7, 2020
d8cc2a6
Merge branch 'master' of https://github.com/rust-windowing/winit into…
Kaiser1989 Dec 28, 2021
8e32192
Fixed Destroy event
Kaiser1989 Dec 28, 2021
9e082fa
Removed duplicated run_return function
Kaiser1989 Jan 1, 2022
7ff2a42
Update android.rs
Kaiser1989 Jan 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/event_loop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,14 @@ impl<T> EventLoop<T> {
self.event_loop.run(event_handler)
}

#[inline]
pub fn run_return<F>(&mut self, event_handler: F)
Kaiser1989 marked this conversation as resolved.
Show resolved Hide resolved
where
F: FnMut(Event<'_, T>, &EventLoopWindowTarget<T>, &mut ControlFlow),
{
self.event_loop.run_return(event_handler)
}

/// Creates an `EventLoopProxy` that can be used to dispatch user events to the main event loop.
pub fn create_proxy(&self) -> EventLoopProxy<T> {
EventLoopProxy {
Expand Down
3 changes: 3 additions & 0 deletions src/platform/android.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ use crate::{
event_loop::{EventLoop, EventLoopWindowTarget},
window::{Window, WindowBuilder},
};

pub use crate::platform_impl::EventLoop as AndroidEventLoop;

use ndk::configuration::Configuration;
use ndk_glue::Rect;

Expand Down
32 changes: 30 additions & 2 deletions src/platform_impl/android/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,24 @@ impl<T: 'static> EventLoop<T> {
event::Event::Suspended
);
}
Event::Pause => self.running = false,
Event::Resume => self.running = true,
Event::Pause => {
call_event_handler!(
event_handler,
self.window_target(),
control_flow,
event::Event::Suspended
);
self.running = false;
}
Event::Resume => {
call_event_handler!(
event_handler,
self.window_target(),
control_flow,
event::Event::Resumed
);
self.running = true;
}
Comment on lines +147 to +164
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add some information about why this is necessary, maybe with a few links to the Android docs? WindowDestroyed already sends Suspended, why is that not enough?

Copy link
Author

Choose a reason for hiding this comment

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

rust-windowing/glutin#904
rust-windowing/glutin#1313

Check this comment:
rust-windowing/glutin#1313 (comment)

Android has some special windows context handling, where context cannot be resused after resuming. Data has to be reinitialized. Therefore we need to handle those events.

Old version just sets flags:

Event::Pause => self.running = false,
Event::Resume => self.running = true

Copy link
Author

Choose a reason for hiding this comment

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

More discussion on this:
rust-mobile/ndk#117

Copy link
Member

Choose a reason for hiding this comment

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

I doubt that is correct: https://developer.android.com/guide/components/activities/activity-lifecycle#onpause

Copy link
Author

@Kaiser1989 Kaiser1989 Jan 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I doubt that is correct: https://developer.android.com/guide/components/activities/activity-lifecycle#onpause

Why? We need to handle pause and resume events on android side. The point is, no one can tell how long the app was paused, maybe it was send to background for several hours... We need to free graphics resources here. If the app resumes, it's possible that original window context is not valid anymore. We cannot keep rendering to that destroyed window.

Just settings flags is not an option. These both events are the point where graphics need to be handled. I've made an consumer of these events, that is working fine:

match event {
   Event::Resumed => {
       // ANDROID: only create if native window is available
       #[cfg(target_os = "android")]
       {
           // enable immersive mode
           enable_immersive();

           // create graphics context (mabye it still exists)
           if self.device_ctx.is_none() && ndk_glue::native_window().is_some() {
               self.device_ctx = Some(DeviceContext::new(_event_loop));
               if let Some(device_ctx) = self.device_ctx.as_mut() {
                   // call create device callback
                   self.runner.create_device(&device_ctx.gl);
               }
           }
       }

       // call resume callback
       paused = false;
   }
   Event::Suspended => {
       // call pause callback
       paused = true;

       // ANDROID: only destroy if native window is available
       #[cfg(target_os = "android")]
       {
           if self.device_ctx.is_some() && ndk_glue::native_window().is_some() {
               if let Some(device_ctx) = self.device_ctx.as_mut() {
                   // call destroy device callback
                   self.runner.destroy_device(&device_ctx.gl);
               }
               self.device_ctx = None;
           }
       }
   }
   ...

Copy link
Member

Choose a reason for hiding this comment

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

The linked docs are pretty clear on this:

You can also use the onPause() method to release system resources, handles to sensors (like GPS), or any resources that may affect battery life while your activity is paused and the user does not need them. However, as mentioned above in the onResume() section, a Paused activity may still be fully visible if in multi-window mode. As such, you should consider using onStop() instead of onPause() to fully release or adjust UI-related resources and operations to better support multi-window mode.

Copy link
Member

Choose a reason for hiding this comment

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

In other words: while I do agree it might be nice to forward these events through winit, the Suspended and Resumed events are not right as they serve a different purpose.

onPause and onResume should result in a WindowEvent::Focused.

Perhaps we should rename Suspended and Resumed to more closely represent the graphics state needing to be forcibly destroyed and recreated, respectively?

Copy link
Author

@Kaiser1989 Kaiser1989 Jan 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, maybe I abused these events to get my context handling correctly. The main point is, that we must not lose important android events. We need to be able to react on window changes.
When do resourced need to be created, when do they need to be destroyed.

And yes you are right, it would be much more coherent, if these events would be of type WindowEvent. Like i'm handling the resize event:

Event::WindowEvent { event, .. } => match event {
    WindowEvent::Resized(physical_size) => {
        if let Some(device_ctx) = self.device_ctx.as_mut() {
            device_ctx.window_context.resize(physical_size);

            // call resize device callback
            self.runner.resize_device(
                &device_ctx.gl,
                physical_size.width,
                physical_size.height,
            );
        }
    }
    ....

Copy link
Author

@Kaiser1989 Kaiser1989 Jan 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I checked the android behaviour:
Focus can not be used to create the device, as i get following events when starting the main loop:

  • resume
  • resize
  • focus

There is no window create event or such. The focus event is too late to use it for creation, as resize event is already executed.

Copy link
Member

Choose a reason for hiding this comment

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

Focus can not be used to create the device

Nor is the android documentation suggesting to. Swapchain creation and deletion "must" be tied to window creation and destroying: you can do it whenever you like, as long as it is created after WindowCreated and destroyed before or during WindowDestroyed. Applications are free to perform this elsewhere, as long as they synchronize properly with the events (ie. hold on to the lock from native_window(), and destroy your resources followed by releasing this lock whenever WindowDestroyed is called).

There is no window create event or such.

At what level are you logging this? Is logging initialized early enough (ie. in ndk_glue::main)? If Android doesn't give your application a window there's nothing to render to.
My best guess is "resume" in your list is referring to event::Event::Resumed, ie. triggered on ndk_glue::Event::WindowCreated?

Event::ConfigChanged => {
let am = ndk_glue::native_activity().asset_manager();
let config = Configuration::from_asset_manager(&am);
Expand All @@ -169,6 +185,18 @@ impl<T: 'static> EventLoop<T> {
);
}
}
Event::Destroy => {
let event = event::Event::WindowEvent {
window_id: window::WindowId(WindowId),
event: event::WindowEvent::CloseRequested,
};
call_event_handler!(
event_handler,
self.window_target(),
control_flow,
event
);
}
Comment on lines +188 to +199
Copy link
Member

Choose a reason for hiding this comment

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

It seems wrong to send CloseRequested when the application is destroyed; maybe it should be the Destroyed event instead?

Copy link
Author

Choose a reason for hiding this comment

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

If I get everthing correctly, there is no direct destroy in android. There is just a request to destroy the app. It's send to background, to make reopening faster. If there are not enough resources available, the app gets destroyed, but those events are handled by OS.

I will check this again!

Copy link
Author

Choose a reason for hiding this comment

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

I checked this. I'm open to change this to WindowEvent::Destroyed. I think it's some sort of interpretation:
Is it asking for destroying or is it force destroying.
When android receives this event, render context is still alive and can be saved or released gracefully, therefore i thought it as "the window is requested to be closed". You can also see this as forced shutdown.

If this is the only problem, i will change it.

Copy link
Member

Choose a reason for hiding this comment

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

Event::Destroy happens when the entire activity is destroyed (https://developer.android.com/guide/components/activities/activity-lifecycle#ondestroy), theoretically the window should have been destroyed prior, with Event::WindowDestroyed?

This is dispatched as a winit Suspended event though. Android''s Suspend/Pause and Resume events represent something different than winit's Suspended / Resumed (specific for Android backends iirc).

Copy link
Author

@Kaiser1989 Kaiser1989 Jan 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It would be best, if we can strictly divide Event and WindowEvent. The questio is, if winit fires all those events correctly, also in android environment. Again, we must not lose any important events for graphics handling.

I will rework the complete event section for android. To make some nice mapping:

// starting app
Android: Start - Winit: ???
Android: Resume - Winit: ???
Android: InputQueueCreated - Winit: ???
Android: WindowCreated - Winit: ???
Android: WindowResized - Winit: ???
Android: ContentRectChanged - Winit: ???
Android: WindowHasFocus - Winit: ???
Android: WindowRedrawNeeded - Winit: ???

// send to background
Android: Pause - Winit: ???
Android: WindowLostFocus - Winit: ???
Android: SaveInstanceState - Winit: ???
Android: Stop - Winit: ???
Android: WindowDestroyed - Winit: ???

// close
Android: InputQueueDestroyed - Winit: ???
Android: Destroy - Winit: ???

Copy link
Member

Choose a reason for hiding this comment

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

Again, we must not lose any important events for graphics handling.

As said it'd be great if this event is handled properly in winit, but we must not diverge from winit patterns too far. It is already a stretch to have Suspended and Resumed (whether their naming is useful/accurate is still up for debate) as this makes winit applications for other platforms not immediately runnable on Android without refactoring swapchain-lifetime logic to adhere to these events.

(Tangent: theoretically it'd be nice if every platform could emit these events for swapchain creation and destruction, making the whole thing more generic. Even better: we provide the RawWindowHandle only in the Resumed [or otherwise named] (Window)Event so that crates are forced to handle this event instead of calling fn raw_window_handle at will.)

I think it's some sort of interpretation:
Is it asking for destroying or is it force destroying.

Regarding this prior question: Whenever this function/event is called, you are supposed to release all resources related to the window (again: the swapchain) and return from this function (ndk_glue does that after the lock from fn native_window() is released, presuming the caller read the documentation and is already holding on to this lock while they have resources created on the window). In that sense you could keep using and rendering to the swapchain without ever returning from on_window_destroyed, but I don't know what Android will do in this case (it's already out of focus and most likely also out of view, so it'll probably result in an ANR, and no new events will arrive).

Event::WindowHasFocus => {
call_event_handler!(
event_handler,
Expand Down