bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Cleanup bevy_winit

Open nicopap opened this issue 1 year ago • 10 comments
trafficstars

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

nicopap avatar Jan 08 '24 15:01 nicopap

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

alice-i-cecile avatar Jan 08 '24 21:01 alice-i-cecile

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

mockersf avatar Jan 09 '24 04:01 mockersf

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.

nicopap avatar Jan 09 '24 12:01 nicopap

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

alice-i-cecile avatar Jan 09 '24 15:01 alice-i-cecile

@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.

nicopap avatar Jan 09 '24 18:01 nicopap

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

alice-i-cecile avatar Jan 09 '24 22:01 alice-i-cecile

blocked by #11281 and #11282 for testing

mockersf avatar Jan 10 '24 07:01 mockersf

Merging later today. Ping me if I forget.

alice-i-cecile avatar Jan 15 '24 15:01 alice-i-cecile

Oops misclicked on mobile

Merging later today. Ping me if I forget.

Has it been tested on enough platforms?

mockersf avatar Jan 15 '24 16:01 mockersf

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

alice-i-cecile avatar Jan 15 '24 17:01 alice-i-cecile

#11489 completes this.

nicopap avatar Jan 29 '24 11:01 nicopap