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

android: Allow redraws and resizes when a Surface is present #3897

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

android: Allow redraws and resizes when a Surface is present #3897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/event_loop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@ impl rwh_06::HasDisplayHandle for dyn ActiveEventLoop + '_ {

/// A proxy for the underlying display handle.
///
/// The purpose of this type is to provide a cheaply clonable handle to the underlying
/// The purpose of this type is to provide a cheaply cloneable handle to the underlying
/// display handle. This is often used by graphics APIs to connect to the underlying APIs.
/// It is difficult to keep a handle to the [`EventLoop`] type or the [`ActiveEventLoop`]
/// type. In contrast, this type involves no lifetimes and can be persisted for as long as
Expand Down
37 changes: 28 additions & 9 deletions src/platform_impl/android/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,13 @@ pub struct EventLoop {
window_target: ActiveEventLoop,
redraw_flag: SharedFlag,
loop_running: bool, // Dispatched `NewEvents<Init>`
running: bool,
// /// `onStart()` - `onStop()` lifecycle, denoting when the app is visible
// TODO: Might use activity_visible for allowing proxy wakeup events?
// activity_visible: bool,
// /// `onResume()` - `onPause()` lifecycle, denoting when the app is actively interacted with
// activity_active: bool,
// TODO: This should be per-Activity (likely analogous to per-Window) state.
window_has_surface: bool,
Comment on lines +106 to +112
Copy link
Member Author

@MarijnS95 MarijnS95 Sep 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I started with these states just to see what would happen, but it's quite common to have:

  • onStart()
  • onResume()
  • NativeWindowCreated
  • NativeWindowResized
  • NativeWindowRedrawNeeded
  • WindowFocusChanged to 1
  • ...
  • WindowFocusChanged to 0
  • onPause()
  • NativeWindowDestroyed
  • onStop()
  • (SaveInstanceState...)

So keying off of NativeWindowCreated/Destroyed is our only safe bet.

pending_redraw: bool,
cause: StartCause,
ignore_volume_keys: bool,
Expand Down Expand Up @@ -145,7 +151,7 @@ impl EventLoop {
},
redraw_flag,
loop_running: false,
running: false,
window_has_surface: false,
pending_redraw: false,
cause: StartCause::Init,
ignore_volume_keys: attributes.ignore_volume_keys,
Expand Down Expand Up @@ -175,13 +181,21 @@ impl EventLoop {

match event {
MainEvent::InitWindow { .. } => {
self.window_has_surface = true;
app.can_create_surfaces(&self.window_target);
},
MainEvent::TerminateWindow { .. } => {
app.destroy_surfaces(&self.window_target);
self.window_has_surface = false;
},
MainEvent::WindowResized { .. } => {
assert!(self.window_has_surface);
resized = true
},
MainEvent::RedrawNeeded { .. } => {
assert!(self.window_has_surface);
pending_redraw = true
},
MainEvent::WindowResized { .. } => resized = true,
MainEvent::RedrawNeeded { .. } => pending_redraw = true,
MainEvent::ContentRectChanged { .. } => {
warn!("TODO: find a way to notify application of content rect change");
},
Expand Down Expand Up @@ -222,7 +236,6 @@ impl EventLoop {
},
MainEvent::Resume { .. } => {
debug!("App Resumed - is running");
self.running = true;
},
MainEvent::SaveState { .. } => {
// XXX: how to forward this state to applications?
Expand All @@ -231,7 +244,6 @@ impl EventLoop {
},
MainEvent::Pause => {
debug!("App Paused - stopped running");
self.running = false;
},
MainEvent::Stop => {
// XXX: how to forward this state to applications?
Expand Down Expand Up @@ -277,7 +289,12 @@ impl EventLoop {
app.proxy_wake_up(&self.window_target);
}

if self.running {
// The SurfaceHolder.Callback that causes the ::WindowResized event should at least fire
// once, directly after InitWindow. Despite not explicitly documented, it should never fire
// after ::TerminateWindow.
// https://cs.android.com/android/platform/superproject/main/+/main:frameworks/base/core/java/android/view/SurfaceHolder.java;l=64-108;drc=366d14f6457edfa12491de15dcad02859f6f2b7a
// https://cs.android.com/android/platform/superproject/main/+/main:frameworks/base/core/java/android/app/NativeActivity.java;l=266-271;drc=5d123b67756dffcfdebdb936ab2de2b29c799321
if self.window_has_surface {
if resized {
Comment on lines +297 to 298
Copy link
Member Author

Choose a reason for hiding this comment

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

Given the asserts above I want to get rid of this double checking, except for handling a user-triggered redraw request.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we also assert if the user triggers request_redraw while there is no surface?

Copy link
Member Author

@MarijnS95 MarijnS95 Sep 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Since it could be async, I don't want to be that harsh on them - for now.

Perhaps that changes once we wire redraws up to Choreographer (and perhaps have a default "keep automatically requesting redraws every vsync" mode).

let size = if let Some(native_window) = self.android_app.native_window().as_ref() {
let width = native_window.width() as _;
Expand Down Expand Up @@ -480,7 +497,8 @@ impl EventLoop {

self.pending_redraw |= self.redraw_flag.get_and_reset();

timeout = if self.running
// TODO: Theoretically the wake-up should also be handled when there's no surface
timeout = if self.window_has_surface
&& (self.pending_redraw || self.window_target.proxy_wake_up.load(Ordering::Relaxed))
Comment on lines +500 to 502
Copy link
Member Author

Choose a reason for hiding this comment

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

Specifically, a non-pending_redraw arbitrary wakeup.

{
// If we already have work to do then we don't want to block on the next poll
Expand Down Expand Up @@ -512,7 +530,8 @@ impl EventLoop {
// a wake up here so we can ignore the wake up if there are no events/requests.
// We also ignore wake ups while suspended.
self.pending_redraw |= self.redraw_flag.get_and_reset();
if !self.running
// TODO: The wake-up should also be handled when there's no surface
if !self.window_has_surface
|| (!self.pending_redraw
&& !self.window_target.proxy_wake_up.load(Ordering::Relaxed))
{
Expand Down
Loading