Cleanup bevy_winit #11257

nicopap · 2024-01-08T15:26:10Z

Objective

The bevy_winit crate has a very large lib.rs, the size is daunting and discourages both contributors and reviewers. Beside, more code implies more ways to accidentally introduce bugs.

Solution

This PR does a few things to reduce the line count, without changing logic:

Move the winit event handler to a standalone function. This reduces the amount of indentation, and isolates relevant code.
Replace the WindowAndInputEventWriters struct with direct calls to World::send_event. This is done through a new private extension trait called AppSendEvent. Now, instead of using event_writers.specific_events.send(SpecificEvent { … }), it directly does app.send_event(SpecificEvent { … }). Looking at the send_event implementation, I didn't see anything that would lead me to believe this is more costly or leads to different behaviors. With this change, we both reduce boilerplate on event sending and delete the WindowAndInputEventWriters struct
Rename window_entity to window. This allows constructing most bevy_window events in a single line, rather than being split on several lines by rustfmt. This is also more consistent, since the Entity field of bevy_window events is called window, it makes sense to re-use the same name to designate the same thing. This removes a lot of boilerplate.
Explicitly name the CreateWindowParams as a type alias instead of copy/pasting it about everywhere. In create_windows, instead of accepting each param field, accept the whole param. This removes a lot of boilerplate as well.

The combination of all those changes leads to a reduction of 200 lines.

Notes to reviewers

You should really use the "Hide whitespaces" diff display mode.

The trickiest bit is probably the scale factor handling. Because we now directly access the world, I had to move event-sending code around, to avoid breaking mutual exclusion rules. I'm fairly confident it's the same behavior.

Another thing that deserves looking-at is non-linux plateform handling. I've only compiled this for linux targets, hence it might fail to compile on other plateforms.

Changelog

bevy::window::WindowMoved's entity field has been renamed to window

Migration Guide

bevy::window::WindowMoved's entity field has been renamed to window. This is to be more consistent with other windowing events.

Consider changing usage:

-window_moved.entity
+window_moved.window

alice-i-cecile · 2024-01-08T21:45:48Z

This will conflict with #11245: opinions on merge order?

mockersf · 2024-01-09T04:31:55Z

This will conflict with #11245: opinions on merge order?

My opinion is that this PR should be tested on enough platforms (at least one native, WebGL2 and WebGPU, and iOS and Android) and that some of those won't work until #11245 is merged

nicopap · 2024-01-09T12:00:23Z

This set of changes is relatively trivial. Unlike #11227, which was a pretty fundamental change in the way the event loop gets updated, this just changes the way we send event, and extracts common code in a type alias.

I'm fine if we merge #11245 first. At this point, I'm comfortable with rebasing. What I really do not want to happen is for this PR to stall like #9768. I'd be happy if this gets reviewed as soon as #11245 is merged and this rebased.

doonv

Looks good, but it looks like WindowTitleCache is never used. So maybe remove that too?

doonv · 2024-01-09T12:39:50Z

crates/bevy_winit/src/lib.rs

+            let msg = "Bevy must be setup with the #[bevy_main] macro on Android";
+            event_loop_builder.with_android_app(ANDROID_APP.get().expect(msg).clone());


I don't really understand why you are separating the msg into a separate variable. If you wanted to reduce indentation I think this will work better:

Suggested change

let msg = "Bevy must be setup with the #[bevy_main] macro on Android";

event_loop_builder.with_android_app(ANDROID_APP.get().expect(msg).clone());

let app = ANDROID_APP

.get()

.expect("Bevy must be setup with the #[bevy_main] macro on Android")

.clone();

event_loop_builder.with_android_app(app);

It's just a personal style choice. I prefer to avoid expanding the method chain into several lines, it occupies vertical space and you can see less things on screen at the same time.

But it's purely personal preference. If the community prefers the expanded version, I'll switch back.

Alright that's fine by me.

alice-i-cecile · 2024-01-09T15:42:16Z

Merging #11245 now; rebase and ping me then I'll merge this in immediately.

The `bevy_winit` crate has a very large `lib.rs`, the size is daunting and discourages both contributors and reviewers. Beside, more code implies more ways to accidentally introduce bugs. This PR does a few things to reduce greatly the line count, without really changing much logic: - Move the winit event handler to a standalone function. This reduces the amount of indentation, and isolates relevant code. - Replace the `WindowAndInputEventWriters` struct with direct calls to `World::send_event`. This is done through a new private extension trait called `AppSendEvent`. Now, instead of using `event_writers.specific_events.send(SpecificEvent { … })`, it directly does `app.send_event(SpecificEvent { … })`. Looking at the `send_event` implementation, I didn't see anything that would lead me to believe this is more costly or leads to different behaviors. With this change, we both reduce boilerplate on event sending and delete the `WindowAndInputEventWriters` struct - Rename `window_entity` to `window`. This allows constructing most `bevy_window` events in a single line, rather than being split on several lines by `rustfmt`. This is also more consistent, since the `Entity` field of `bevy_window` events is called `window`, it makes sense to re-use the same name to designate the same thing. This removes a lot of boilerplate. - Explicitly name the `CreateWindowParams` as a type alias instead of copy/pasting it about everywhere. In `create_windows`, instead of accepting each param field, accept the whole param. This removes a lot of boilerplate as well. The combination of all those changes leads to a reduction of 200 lines. **Notes to reviewers** You should really use the "Hide whitespaces" diff display mode. The trickiest bit is probably the scale factor handling. Because we now directly access the world, I had to move event-sending code around, to avoid breaking mutual exclusion rules. I'm fairly confident it's the same behavior. Another thing that deserves looking-at is non-linux plateform handling. I've only compiled this for linux targets, hence it might fail to compile on other plateforms.

nicopap · 2024-01-09T18:32:22Z

@alice-i-cecile should be good to go.

Also TIL that you can use -Xignore-all-space to rebase while using a whitespace-ignoring diff for merge resolution. Without it, it would have been much harder to accomplish.

Vrixyz · 2024-01-09T21:35:10Z

crates/bevy_winit/src/lib.rs

+impl AppSendEvent for App {
+    fn send_event<E: bevy_ecs::event::Event>(&mut self, event: E) {
+        self.world.send_event(event);
+    }
 }


I like it, and I wouldn't block on it, but this might be better as a usability method within bevy_ecs maybe?

I tried to add it earlier and it was controversial IIRC 😅 Private helper is fine though.

Vrixyz

I like it 👍 tested a few examples on windows native + firefox

alice-i-cecile · 2024-01-09T22:07:56Z

Once we've checked that there's no new problems on Android / iOS this is good to go.

mockersf · 2024-01-10T07:56:49Z

blocked by #11281 and #11282 for testing

alice-i-cecile · 2024-01-15T15:59:31Z

Merging later today. Ping me if I forget.

mockersf · 2024-01-15T16:59:01Z

Oops misclicked on mobile

Merging later today. Ping me if I forget.

Has it been tested on enough platforms?

alice-i-cecile · 2024-01-15T17:49:07Z

Hmm. We should resolve conflicts and do another round of testing.

# Objective Get #11257 changes merged. I rewrote them one by one checking each to ensure correctness. In particular, the window rescale logic changes to accomodate mut app access are double checked. Not all changes have been included as some of bevy_winit has since changed, and i am not confident including them. Namely, the `run_app_update_if_should` change. ### Notes to reviewers Review commits individually, use the "Hide whitespaces" diff display mode. ## Changelog * `bevy::window::WindowMoved`'s `entity` field has been renamed to `window` ## Migration Guide `bevy::window::WindowMoved`'s `entity` field has been renamed to `window`. This is to be more consistent with other windowing events. Consider changing usage: ```diff -window_moved.entity +window_moved.window ``` --------- Co-authored-by: François <mockersf@gmail.com>

nicopap · 2024-01-29T11:06:25Z

#11489 completes this.

# Objective Get bevyengine#11257 changes merged. I rewrote them one by one checking each to ensure correctness. In particular, the window rescale logic changes to accomodate mut app access are double checked. Not all changes have been included as some of bevy_winit has since changed, and i am not confident including them. Namely, the `run_app_update_if_should` change. ### Notes to reviewers Review commits individually, use the "Hide whitespaces" diff display mode. ## Changelog * `bevy::window::WindowMoved`'s `entity` field has been renamed to `window` ## Migration Guide `bevy::window::WindowMoved`'s `entity` field has been renamed to `window`. This is to be more consistent with other windowing events. Consider changing usage: ```diff -window_moved.entity +window_moved.window ``` --------- Co-authored-by: François <mockersf@gmail.com>

nicopap added A-Windowing Platform-agnostic interface layer to run your app in C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jan 8, 2024

alice-i-cecile requested review from Vrixyz and mockersf January 8, 2024 21:45

doonv approved these changes Jan 9, 2024

View reviewed changes

nicopap force-pushed the cleanup-bevy_winit branch from 59b6f1e to 70c36bf Compare January 9, 2024 18:28

alice-i-cecile added the P-High This is particularly urgent, and deserves immediate attention label Jan 9, 2024

Vrixyz reviewed Jan 9, 2024

View reviewed changes

Vrixyz approved these changes Jan 9, 2024

View reviewed changes

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 Jan 9, 2024

nicopap added the S-Blocked This cannot move forward until something else changes label Jan 10, 2024

matiqo15 removed the S-Blocked This cannot move forward until something else changes label Jan 14, 2024

mockersf enabled auto-merge January 15, 2024 16:57

mockersf disabled auto-merge January 15, 2024 16:57

alice-i-cecile removed 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 Jan 22, 2024

atlv24 mentioned this pull request Jan 23, 2024

Cleanup bevy winit #11489

Merged

nicopap closed this Jan 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup bevy_winit #11257

Cleanup bevy_winit #11257

nicopap commented Jan 8, 2024

alice-i-cecile commented Jan 8, 2024

mockersf commented Jan 9, 2024

nicopap commented Jan 9, 2024

doonv left a comment

doonv Jan 9, 2024

nicopap Jan 9, 2024

doonv Jan 9, 2024

alice-i-cecile commented Jan 9, 2024

nicopap commented Jan 9, 2024

Vrixyz Jan 9, 2024

alice-i-cecile Jan 9, 2024

Vrixyz left a comment

alice-i-cecile commented Jan 9, 2024

mockersf commented Jan 10, 2024

alice-i-cecile commented Jan 15, 2024

mockersf commented Jan 15, 2024

alice-i-cecile commented Jan 15, 2024

nicopap commented Jan 29, 2024

		let msg = "Bevy must be setup with the #[bevy_main] macro on Android";
		event_loop_builder.with_android_app(ANDROID_APP.get().expect(msg).clone());

-            let msg = "Bevy must be setup with the #[bevy_main] macro on Android";
-            event_loop_builder.with_android_app(ANDROID_APP.get().expect(msg).clone());
+            let app = ANDROID_APP
+                .get()
+                .expect("Bevy must be setup with the #[bevy_main] macro on Android")
+                .clone();
+            event_loop_builder.with_android_app(app);

Cleanup bevy_winit #11257

Cleanup bevy_winit #11257

Conversation

nicopap commented Jan 8, 2024

Objective

Solution

Notes to reviewers

Changelog

Migration Guide

alice-i-cecile commented Jan 8, 2024

mockersf commented Jan 9, 2024

nicopap commented Jan 9, 2024

doonv left a comment

Choose a reason for hiding this comment

doonv Jan 9, 2024

Choose a reason for hiding this comment

nicopap Jan 9, 2024

Choose a reason for hiding this comment

doonv Jan 9, 2024

Choose a reason for hiding this comment

alice-i-cecile commented Jan 9, 2024

nicopap commented Jan 9, 2024

Vrixyz Jan 9, 2024

Choose a reason for hiding this comment

alice-i-cecile Jan 9, 2024

Choose a reason for hiding this comment

Vrixyz left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Jan 9, 2024

mockersf commented Jan 10, 2024

alice-i-cecile commented Jan 15, 2024

mockersf commented Jan 15, 2024

alice-i-cecile commented Jan 15, 2024

nicopap commented Jan 29, 2024