Improve `bevy_winit` documentation #7609

maniwani · 2023-02-10T20:27:56Z

Redo of #7590 since I messed up my branch.

Objective

Revise docs.
Refactor event loop code a little bit to make it easier to follow.

Solution

Do the above.

Migration Guide

UpdateMode::Reactive { max_wait: .. } -> UpdateMode::Reactive { wait: .. }
UpdateMode::ReactiveLowPower { max_wait: .. } -> UpdateMode::ReactiveLowPower { wait: .. }

alice-i-cecile

These changes look good to me and clean things up, but this needs to be rebased.

github-actions · 2023-07-25T19:27:28Z

Example alien_cake_addict failed to run, please try running it locally and check the result.

github-actions · 2023-07-25T19:41:24Z

Example alien_cake_addict failed to run, please try running it locally and check the result.

github-actions · 2023-07-25T19:51:25Z

Example alien_cake_addict failed to run, please try running it locally and check the result.

IceSentry

In general this looks good to me. I can't comment on the unsafe part though.

crates/bevy_winit/src/lib.rs

maniwani · 2023-07-25T21:15:43Z

I can't comment on the unsafe part though.

@IceSentry, I changed it back to doing the SystemState::get_mut and passing those references into the function. I was doing the lifetime transmute in #9122 because it's essential with those changes, but it's not essential here.

IceSentry

Alright, nothing else jumps out at me so LGTM

ickk · 2023-07-25T21:37:23Z

I'm guessing the addition of the generic T to functions taking EventLoop<()> is to support #9122?

Otherwise in this PR I don't understand its purpose, since all the functions of note here are private, and WinitPlugin only explicitly constructs an EventLoop<()>; it adds some cognitive load. But if you're going to add it anyway in that other PR then its fine here

maniwani · 2023-07-25T21:40:06Z

I'm guessing the addition of the generic T to functions taking EventLoop<()> is #9122?

Ah, yes, that's correct. I'll change it back though. It's small enough, and it'll be justified then.

crates/bevy_winit/src/lib.rs

Selene-Amanita

I'm not sure I understood everything but I finally reviewed it.

Sorry for the delay.

I'm mostly concerned about #7609 (comment).

crates/bevy_winit/src/lib.rs

Selene-Amanita · 2023-07-26T17:09:57Z

crates/bevy_winit/src/lib.rs

@@ -244,79 +241,83 @@ struct InputEvents<'w> {
    mouse_wheel_input: EventWriter<'w, MouseWheel>,
    touch_input: EventWriter<'w, TouchInput>,
    ime_input: EventWriter<'w, Ime>,


Nitpick:

Maybe events from bevy_input should be bellow the ones from bevy_window?

(with maybe a mention of that in the comments, and the "// device events" could mention more clearly that it's a winit's DeviceEvent and the other ones are WindowEvents?)

crates/bevy_winit/src/winit_config.rs

Selene-Amanita · 2023-07-26T19:50:35Z

crates/bevy_winit/src/winit_config.rs

-    /// input, or the window being resized) is received or the time limit is reached.
+    /// The [`App`](bevy_app::App) will update in response to the following, until an
+    /// [`AppExit`](bevy_app::AppExit) event appears:
+    /// - enough time has elapsed since the previous update


Suggested change

/// - enough time has elapsed since the previous update

/// - enough time has elapsed since the previous update, see [`UpdateMode::Reactive::wait`]

Why link back to Reactive a second time here when ReactiveLowPower's wait field already has its own documentation?

crates/bevy_winit/src/winit_config.rs

Co-authored-by: Sélène Amanita <134181069+Selene-Amanita@users.noreply.github.com>

# Objective - #7609 broke Android support ``` 8721 8770 I event crates/bevy_winit/src/system.rs:55: Creating new window "App" (0v0) 8721 8769 I RustStdoutStderr: thread '<unnamed>' panicked at 'Cannot get the native window, it's null and will always be null before Event::Resumed and after Event::Suspended. Make sure you only call this function between those events.', winit-0.28.6/src/platform_impl/android/mod.rs:1058:13 ``` ## Solution - Don't create windows on `StartCause::Init` as it's too early

# Objective - bevyengine#7609 broke Android support ``` 8721 8770 I event crates/bevy_winit/src/system.rs:55: Creating new window "App" (0v0) 8721 8769 I RustStdoutStderr: thread '<unnamed>' panicked at 'Cannot get the native window, it's null and will always be null before Event::Resumed and after Event::Suspended. Make sure you only call this function between those events.', winit-0.28.6/src/platform_impl/android/mod.rs:1058:13 ``` ## Solution - Don't create windows on `StartCause::Init` as it's too early

# Objective - bevyengine/bevy#7609 broke Android support ``` 8721 8770 I event crates/bevy_winit/src/system.rs:55: Creating new window "App" (0v0) 8721 8769 I RustStdoutStderr: thread '<unnamed>' panicked at 'Cannot get the native window, it's null and will always be null before Event::Resumed and after Event::Suspended. Make sure you only call this function between those events.', winit-0.28.6/src/platform_impl/android/mod.rs:1058:13 ``` ## Solution - Don't create windows on `StartCause::Init` as it's too early

maniwani changed the title ~~refactoring~~ Improve bevy_winit documentation Feb 10, 2023

maniwani mentioned this pull request Feb 10, 2023

Change WinitPlugin defaults to limit game update rate when window is not visible #7611

Merged

alice-i-cecile reviewed Jun 8, 2023

View reviewed changes

maniwani force-pushed the bevy-winit-refactoring branch 2 times, most recently from f1a0efb to 45ac905 Compare July 25, 2023 19:23

maniwani force-pushed the bevy-winit-refactoring branch 2 times, most recently from b664432 to dfe3bea Compare July 25, 2023 19:37

alice-i-cecile approved these changes Jul 25, 2023

View reviewed changes

alice-i-cecile requested a review from Selene-Amanita July 25, 2023 19:39

maniwani force-pushed the bevy-winit-refactoring branch from dfe3bea to 36c49c1 Compare July 25, 2023 19:44

IceSentry reviewed Jul 25, 2023

View reviewed changes

crates/bevy_winit/src/lib.rs Outdated Show resolved Hide resolved

crates/bevy_winit/src/lib.rs Show resolved Hide resolved

crates/bevy_winit/src/lib.rs Show resolved Hide resolved

crates/bevy_winit/src/lib.rs Show resolved Hide resolved

maniwani force-pushed the bevy-winit-refactoring branch 2 times, most recently from a716fce to 10a8ea6 Compare July 25, 2023 21:10

refactoring

b194c60

maniwani force-pushed the bevy-winit-refactoring branch from 10a8ea6 to b194c60 Compare July 25, 2023 21:17

Update lib.rs

6fa1263

IceSentry approved these changes Jul 25, 2023

View reviewed changes

misc

fc1dc5e

ickk approved these changes Jul 25, 2023

View reviewed changes

crates/bevy_winit/src/lib.rs Outdated Show resolved Hide resolved

I forgot finished_and_setup_done

4f09919

maniwani force-pushed the bevy-winit-refactoring branch from 57ac11a to 4f09919 Compare July 25, 2023 22:03

Selene-Amanita reviewed Jul 26, 2023

View reviewed changes

maniwani and others added 3 commits July 30, 2023 23:11

Apply suggestions from code review

aac0f17

Co-authored-by: Sélène Amanita <134181069+Selene-Amanita@users.noreply.github.com>

fix Reactive and docs

2627ac8

clippy

6808d75

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 31, 2023

alice-i-cecile added this pull request to the merge queue Jul 31, 2023

Merged via the queue into bevyengine:main with commit c0510c8 Jul 31, 2023
21 checks passed

maniwani deleted the bevy-winit-refactoring branch August 5, 2023 17:38

mockersf mentioned this pull request Sep 3, 2023

don't create windows on winit StartCause::Init event #9684

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `bevy_winit` documentation #7609

Improve `bevy_winit` documentation #7609

maniwani commented Feb 10, 2023 •

edited

Loading

alice-i-cecile left a comment

github-actions bot commented Jul 25, 2023

github-actions bot commented Jul 25, 2023

github-actions bot commented Jul 25, 2023

IceSentry left a comment

maniwani commented Jul 25, 2023

IceSentry left a comment

ickk commented Jul 25, 2023

maniwani commented Jul 25, 2023

Selene-Amanita left a comment •

edited

Loading

Selene-Amanita Jul 26, 2023

Selene-Amanita Jul 26, 2023

maniwani Jul 31, 2023

	/// - enough time has elapsed since the previous update
	/// - enough time has elapsed since the previous update, see [`UpdateMode::Reactive::wait`]

Improve bevy_winit documentation #7609

Improve bevy_winit documentation #7609

Conversation

maniwani commented Feb 10, 2023 • edited Loading

Objective

Solution

Migration Guide

alice-i-cecile left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 25, 2023

github-actions bot commented Jul 25, 2023

github-actions bot commented Jul 25, 2023

IceSentry left a comment

Choose a reason for hiding this comment

maniwani commented Jul 25, 2023

IceSentry left a comment

Choose a reason for hiding this comment

ickk commented Jul 25, 2023

maniwani commented Jul 25, 2023

Selene-Amanita left a comment • edited Loading

Choose a reason for hiding this comment

Selene-Amanita Jul 26, 2023

Choose a reason for hiding this comment

Selene-Amanita Jul 26, 2023

Choose a reason for hiding this comment

maniwani Jul 31, 2023

Choose a reason for hiding this comment

Improve `bevy_winit` documentation #7609

Improve `bevy_winit` documentation #7609

maniwani commented Feb 10, 2023 •

edited

Loading

Selene-Amanita left a comment •

edited

Loading