Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows as Entities #4947

Closed
wants to merge 32 commits into from
Closed

Conversation

Weibye
Copy link
Contributor

@Weibye Weibye commented Jun 6, 2022

This is going to be both large and controversial, so I would very much appreciate feedback and input on this initial direction so that this turns out the best it can be.

I will update the PR description with more details as I progress.

Current Status: Check todo list at the bottom.

Objective

  • Make windows be Entities as that would allow users to operate on windows the same as they would already be doing with commands, systems and components
  • Fixes Windows as entities #4530

Solution

High Level Overview

Windows in Bevy has now become entities with components. The user can query these components to read information about the windows. The user can use change detection to check if various components has changed. These components are 'readony'-ish for the user.

To mutate the windows, the user will now use WindowCommands and apply Commands to the Entity with the window. The window backend is responsible handling these commands and updating the data on the components accordingly.

bevy_window changes

Window_id

  • WindowId has been replaced by Entity throughout the entire project

Window

  • Window has been split into multiple components:
    • Window (marker)
    • WindowCursor
    • WindowCursorPosition
    • WindowHandle
    • WindowPresentation
    • WindowModeComponent
    • WindowPosition
    • WindowResolution
    • WindowTitle
    • WindowCanvas
    • WindowDecoraded (marker)
    • WindowCurrentlyFocused (marker)
    • WindowResizable (marker)
    • WindowTransparent (marker)
    • WindowMinimized (marker)
    • WindowMaximized (marker)
  • It is the responsibility of the window backend (bevy_winit) to create, update and remove these components
  • All fields on these components are private
  • The user should get data from the fields using the various getter methods exposed
  • All setter methods are named update_x_from_backend() or similar and should only be used by the window backend.

Windows

  • Windows has been removed completely
  • Getting all windows should now be done by querying for entities with the Window marker component
    • Query<Entity, With<Window>>

WindowCommandsExtension

  • Created WindowCommandsExtensions so that we can commands.window(entity) to get WindowCommands
    • Similar to how ÈntityCommands work

WindowCommand

  • Is no longer an enum
  • Has been split into structs that all impl Command
  • These commands will send themselves as events on command.write() for the window backend to handle

PrimaryWindow

  • PrimaryWindow is a new resource that the bevy_window plugin is responsible for creating when setting up the initial window.
  • It contains the Entity of the window that is considered the primary window
  • TODO: Consider if this should be a marker instead of a resource.

bevy_winit changes

  • TODO

bevy_render changes

  • TODO

bevy_text

  • Internal changes: Res -> Query

bevy_ui

Internal changes:

  • Res<Windows> -> Query
  • window.

Changelog

  • TODO: User facing changes to Window
    • Creating windows
    • Reading from windows
    • Writing / making changes to windows
  • TODO: User facing changes to Winit (This probably does not affect most users)
  • TODO: User facing changes to bevy_render

Migration Guide

  • WindowId -> Entity
  • window: Res<Window> -> windows: Query<&RelevantWindowComponent, With<Window>>
  • windows: Res<Windows> -> windows: Query<&RelevantWindowComponent, With<Window>>

Remaining Tasks

  • Changes compiles :)
  • Fix timing issues between bevy_window, bevy_winit and bevy_render when it comes to window handle creation.
  • Check for system order ambiguities in Winit systems
  • Double check that we aren't regressing any features
  • Clear out remaining TODOs
  • Write tests that covers these changes / systems
  • Add documentation
  • Write changelog
  • Write migration guide

@alice-i-cecile alice-i-cecile 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 C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR labels Jun 6, 2022
crates/bevy_winit/src/lib.rs Show resolved Hide resolved
Comment on lines 60 to 78
CoreStage::PostUpdate,
SystemSet::new()
.label(ModifiesWindows)
.with_system(update_title)
.with_system(update_window_mode)
.with_system(update_decorations)
.with_system(update_scale_factor)
.with_system(update_resizable)
.with_system(update_position)
.with_system(update_minimized)
.with_system(update_maximized)
.with_system(update_resolution)
.with_system(update_cursor_icon)
.with_system(update_cursor_lock_mode)
.with_system(update_cursor_visibility)
.with_system(update_cursor_position)
.with_system(update_resize_contraints)
.with_system(update_present_mode)
.with_system(destroy_windows), // TODO: This should probably go last?
Copy link
Contributor Author

@Weibye Weibye Jun 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splitting this into multiple systems seemed like the natural thing to do once the commands are their own events. Having many small systems instead of a big one should make the plugin more approachable to new users wanting to contribute so I hope this does not cause a lot of timing or race-condition issues

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should scan for and resolve system order ambiguities before merging this. That said, I very much appreciate this design.


/// A collection of [`Window`]s with unique [`WindowId`]s.
#[derive(Debug, Default)]
pub struct Windows {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All places that previously used Windows should now instead do a Query<Entity, With<Window>>

Comment on lines +102 to +117
#[derive(Debug)]
pub struct SetTitleCommand {
pub entity: Entity,
pub title: String,
}

impl Command for SetTitleCommand {
fn write(self, world: &mut World) {
if let Some(_) = world.get::<Window>(self.entity) {
let mut event = world.resource_mut::<Events<SetTitleCommand>>();
event.send(self);
} else {
panic!("Trying to enact window commands on an entity without a window-component");
}
}
}
Copy link
Contributor Author

@Weibye Weibye Jun 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All window commands now look like this: They are a Command that sends itself as an event that the window backend will pick up

Comment on lines +63 to +69
// TODO: Can this be solved in any other way?
/// Returns true if this commands has the entity present
pub fn has_entity(&self, entity: Entity) -> bool {
return self.entities.contains(entity);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed to be able to not grant WindowCommands on an entity that does not exist, see: https://github.com/bevyengine/bevy/pull/4947/files#diff-9e0af223dcea6a0265d880b70764a4362c0c84438fb28f5f558a369cda91021cR27

@bjorn3 bjorn3 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jun 6, 2022
crates/bevy_winit/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +113 to +131
// if not we'll regress on this
let window_descriptor = app
.world
.get_resource::<WindowDescriptor>()
.map(|descriptor| (*descriptor).clone())
.unwrap_or_default();
let mut create_window_event = app.world.resource_mut::<Events<CreateWindow>>();
create_window_event.send(CreateWindow {
id: WindowId::primary(),
descriptor: window_descriptor,
});

let window_id = app.world.spawn().id();

let mut system_state: SystemState<(Commands, ResMut<PrimaryWindow>)> = SystemState::new(&mut app.world);
let (mut commands, mut primary_window) = system_state.get_mut(&mut app.world);
primary_window.window = Some(window_id);
// create_primary_window(commands, primary_window);


let command = CreateWindowCommand { entity: window_id, descriptor: window_descriptor };
// Apply the command directly on the world
// I wonder if this causes timing issue: this will trigger a CreateWindowCommand event, but will bevy_winit exist in time to listen to the event?
command.write(&mut app.world);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Replace with systemState api

@Weibye
Copy link
Contributor Author

Weibye commented Aug 6, 2022

Status: Work has been picked up by @Aceeri over at #5589 as I have shifted focus onto other UI priorities. Will leave this up for now then we close this when #5589 get merged

@Nilirad
Copy link
Contributor

Nilirad commented Aug 6, 2022

@Weibye I think you can just close this, since you can reopen it with just a click if needed.

@Weibye
Copy link
Contributor Author

Weibye commented Aug 6, 2022

@Weibye I think you can just close this, since you can reopen it with just a click if needed.

Fair point!

@Weibye Weibye deleted the windows-as-entities branch August 18, 2022 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows as entities
4 participants