Improve `bevy_winit` documentation by maniwani · Pull Request #7609 · bevyengine/bevy

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: .. }

ghost

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

ghost · 2023-07-25T19:27:28Z

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

ghost · 2023-07-25T19:41:24Z

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

ghost · 2023-07-25T19:51:25Z

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

ghost

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

ghost · 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.

ghost

Alright, nothing else jumps out at me so LGTM

ghost · 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

ghost · 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.

ghost

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

Sorry for the delay.

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

ghost · 2023-07-26T17:09:57Z

@@ -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?)

ghost · 2023-07-26T19:50:35Z

-    /// 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?

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

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

Comment thread crates/bevy_winit/src/lib.rs Resolved

Comment thread crates/bevy_winit/src/lib.rs Resolved

Comment thread crates/bevy_winit/src/lib.rs Resolved

Comment thread crates/bevy_winit/src/lib.rs 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

Comment thread crates/bevy_winit/src/lib.rs 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

Cameron 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

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

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

Conversation

maniwani commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Migration Guide

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 25, 2023

Uh oh!

ghost commented Jul 25, 2023

Uh oh!

ghost commented Jul 25, 2023

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Jul 25, 2023

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 25, 2023

Uh oh!

ghost commented Jul 25, 2023

Uh oh!

Uh oh!

ghost left a comment • edited by Selene-Amanita Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

ghost Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

maniwani commented Feb 10, 2023 •

edited

Loading

ghost left a comment •

edited by Selene-Amanita

Loading