bevy
bevy copied to clipboard
Cleanup bevy_winit
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
WindowAndInputEventWritersstruct with direct calls toWorld::send_event. This is done through a new private extension trait calledAppSendEvent. Now, instead of usingevent_writers.specific_events.send(SpecificEvent { … }), it directly doesapp.send_event(SpecificEvent { … }). Looking at thesend_eventimplementation, 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 theWindowAndInputEventWritersstruct - Rename
window_entitytowindow. This allows constructing mostbevy_windowevents in a single line, rather than being split on several lines byrustfmt. This is also more consistent, since theEntityfield ofbevy_windowevents is calledwindow, it makes sense to re-use the same name to designate the same thing. This removes a lot of boilerplate. - Explicitly name the
CreateWindowParamsas a type alias instead of copy/pasting it about everywhere. Increate_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'sentityfield has been renamed towindow
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
This will conflict with #11245: opinions on merge order?
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
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.
Merging #11245 now; rebase and ping me then I'll merge this in immediately.
@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.
Once we've checked that there's no new problems on Android / iOS this is good to go.
blocked by #11281 and #11282 for testing
Merging later today. Ping me if I forget.
Oops misclicked on mobile
Merging later today. Ping me if I forget.
Has it been tested on enough platforms?
Hmm. We should resolve conflicts and do another round of testing.
#11489 completes this.