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

refactor: Simplify lifetimes for Commands and related types #11445

Merged
merged 8 commits into from
Jan 22, 2024

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Jan 21, 2024

Objective

It would be convenient to be able to call functions with Commands as a parameter without having to move your own instance of Commands. Since this struct is composed entirely of references, we can easily get an owned instance of Commands by shortening the lifetime.

Solution

Add Commands::reborrow, EntiyCommands::reborrow, and Deferred::reborrow, which returns an owned version of themselves with a shorter lifetime.

Remove unnecessary lifetimes from EntityCommands. The 'w and 's lifetimes only have to be separate for Commands because it's used as a SystemParam -- this is not the case for EntityCommands.


Changelog

Added Commands::reborrow. This is useful if you have &mut Commands but need Commands. Also added EntityCommands::reborrow and Deferred:reborrow which serve the same purpose.

Migration Guide

The lifetimes for EntityCommands have been simplified.

// Before (Bevy 0.12)
struct MyStruct<'w, 's, 'a> {
     commands: EntityCommands<'w, 's, 'a>,
}

// After (Bevy 0.13)
struct MyStruct<'a> {
    commands: EntityCommands<'a>,
}

The method EntityCommands::commands now returns Commands rather than &mut Commands.

// Before (Bevy 0.12)
let commands = entity_commands.commands();
commands.spawn(...);

// After (Bevy 0.13)
let mut commands = entity_commands.commands();
commands.spawn(...);

@JoJoJet JoJoJet changed the title Add a method to turn &mut Commands into Commands refactor: Simplify lifetimes for Commands and related types Jan 21, 2024
@JoJoJet JoJoJet added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jan 21, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Really nice! The logic makes sense to me, the docs on reborrow are unusually helpful, and the simplified lifetimes are great.

Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

Just a nit about the migration guide. Definitely nice not to see 3 lifetimes on EntityCommands anymore.

crates/bevy_ecs/src/system/commands/mod.rs Show resolved Hide resolved
@hymm hymm 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 22, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 22, 2024
Merged via the queue into bevyengine:main with commit 7d69d31 Jan 22, 2024
22 checks passed
@JoJoJet JoJoJet deleted the command-reborrow branch January 22, 2024 17:40
@eidloi
Copy link
Contributor

eidloi commented Feb 18, 2024

FYI this gave me a mental breakdown. It is no longer possible to create a unified extension over both commands and entity commands, which means I have to throw away like ~10kloc and start over the UI widget library I have been building for the past 3 months. On that note, I find it hard to believe the lifetimes were that much of a problem. They were complicated to use when writing some extensions, yes, but they did let you write stuff you would have needed the world otherwise for. Cheers.

@hymm
Copy link
Contributor

hymm commented Feb 18, 2024

@eidloi I'd be fine with reverting the lifetime change. Could you give a small example of code that doesn't compile anymore with this change?

@eidloi
Copy link
Contributor

eidloi commented Feb 18, 2024

No, @hymm, it is fine, meltdown's over, moving forward - I'll drop supporting EntityCommands entirely. This was my original code:

use bevy::{
    ecs::{
        bundle::Bundle,
        entity::Entity,
        system::{Commands, EntityCommands},
    },
    hierarchy::BuildChildren,
};

pub struct UiBuilder<'w, 's, 'a> {
    commands: &'a mut Commands<'w, 's>,
    entity: Option<Entity>,
}

impl<'w, 's> UiBuilder<'w, 's, '_> {
    pub fn id(&self) -> Option<Entity> {
        self.entity
    }

    pub fn commands(&mut self) -> &mut Commands<'w, 's> {
        self.commands
    }

    pub fn entity_commands<'a>(&'a mut self) -> Result<EntityCommands<'w, 's, 'a>, &'static str> {
        if let Some(entity) = self.entity {
            Ok(self.commands().entity(entity))
        } else {
            Err("No entity set for UiBuilder")
        }
    }

    pub fn spawn<'a>(&'a mut self, bundle: impl Bundle) -> EntityCommands<'w, 's, 'a> {
        let mut new_entity = Entity::PLACEHOLDER;

        if let Some(entity) = self.id() {
            self.commands().entity(entity).with_children(|parent| {
                new_entity = parent.spawn(bundle).id();
            });
        } else {
            new_entity = self.commands().spawn(bundle).id();
        }

        self.commands().entity(new_entity)
    }
}

pub trait UiBuilderExt<'w, 's, 'a> {
    fn ui_builder(&'a mut self) -> UiBuilder<'w, 's, 'a>;
}

impl<'w, 's, 'a> UiBuilderExt<'w, 's, 'a> for EntityCommands<'w, 's, 'a> {
    fn ui_builder(&'a mut self) -> UiBuilder<'w, 's, 'a> {
        let entity = self.id();
        let mut builder = self.commands().ui_builder();
        builder.entity = entity.into();

        builder
    }
}

impl<'w, 's, 'a> UiBuilderExt<'w, 's, 'a> for Commands<'w, 's> {
    fn ui_builder(&'a mut self) -> UiBuilder<'w, 's, 'a> {
        UiBuilder::<'w, 's, 'a> {
            commands: self,
            entity: None,
        }
    }
}

Which let me do things like:

commands.entity(root_entity).ui_builder().column(
        ColumnConfig {
            width: Val::Percent(100.),
            background_color: Color::rgb(0.7, 0.7, 0.7),
            ..default()
        },
        |column| {
            column.row(
                RowConfig {
                    height: Val::Px(50.),
                    background_color: Color::rgb(0.8, 0.8, 0.8),
                },
                |row| {
                    row.column(
                        ColumnConfig {
                            width: Val::Px(100.),
                            background_color: Color::CYAN,
                        },
                        |_| {},
                    )
                    .insert((
                        Interaction::default(),
                        ChangeableBackgroundColor,
                        ChangeableBorderSize,
                        GenerateContextMenu::default(),
                    ));

                    row.checkbox(String::from("Panel").into())
                        .insert((ToggleDisplay(floating_panel_id_2),));
                    row.checkbox(Option::<String>::None);
                    row.slider(
                        SliderConfig::horizontal(Some("Slider"), 0., 100., 50., true).into(),
                    );
                    row.radio_group(vec!["Draggable", "Resizable", "Both"], false)
                        .insert(PanelFeatureControl(floating_panel_id));
                },
            );           
        },
    );

Notice the inserts in the chain there? Bottom line is that the callbacks inside would receive a builder, but would return the EntityCommands of the corresponding frame entity, so you could decorate it with whatever. This was a necessary addition to 1) be able to grab the commands to spawn entities next to each other while still being able to return the EntityCommands of the external one (something which a ChildBuilder wouldn't let you do) and 2) to have a common struct that can be extended by end users as well and not just from Bevy. An example of 1):

pub trait UiScrollViewExt<'w, 's> {
    fn scroll_view<'a>(
        &'a mut self,
        restrict_to: Option<ScrollAxis>,
        spawn_children: impl FnOnce(&mut UiBuilder),
    ) -> EntityCommands<'w, 's, 'a>;
}

impl<'w, 's> UiScrollViewExt<'w, 's> for UiBuilder<'w, 's, '_> {
    fn scroll_view<'a>(
        &'a mut self,
        restrict_to: Option<ScrollAxis>,
        spawn_children: impl FnOnce(&mut UiBuilder),
    ) -> EntityCommands<'w, 's, 'a> {
        let mut viewport = Entity::PLACEHOLDER;
        let mut content_container = Entity::PLACEHOLDER;
        let mut horizontal_scroll_id: Option<Entity> = None;
        let mut horizontal_scroll_handle_id: Option<Entity> = None;
        let mut vertical_scroll_id: Option<Entity> = None;
        let mut vertical_scroll_handle_id: Option<Entity> = None;

        let mut scroll_view = self.container(ScrollView::frame(), |frame| {
            let scroll_axes = if let Some(restrict_to) = restrict_to {
                vec![restrict_to]
            } else {
                vec![ScrollAxis::Horizontal, ScrollAxis::Vertical]
            };

            let scroll_view_id = frame.id().unwrap();
            viewport = frame
                .container(
                    (
                        ScrollView::viewport(),
                        ScrollViewViewport {
                            scroll_view: scroll_view_id,
                        },
                    ),
                    |viewport| {
                        content_container = viewport
                            .container(
                                ScrollView::content(scroll_view_id, restrict_to),
                                spawn_children,
                            )
                            .id();
                    },
                )
                .id();

            frame.container(ScrollView::scroll_bar_container(), |scroll_bar_container| {
                for axis in scroll_axes.iter() {
                    let mut handle_id = Entity::PLACEHOLDER;
                    let mut scroll_bar = scroll_bar_container.container(
                        ScrollView::scroll_bar(*axis),
                        |scroll_bar| {
                            handle_id = scroll_bar
                                .spawn((
                                    ScrollView::scroll_bar_handle(*axis),
                                    ScrollBarHandle {
                                        axis: *axis,
                                        scroll_view: scroll_view_id,
                                    },
                                ))
                                .id();
                        },
                    );
                    scroll_bar.insert(ScrollBar {
                        axis: *axis,
                        scroll_view: scroll_view_id,
                        handle: handle_id,
                    });
                    match axis {
                        ScrollAxis::Horizontal => {
                            horizontal_scroll_id = scroll_bar.id().into();
                            horizontal_scroll_handle_id = handle_id.into();
                        }
                        ScrollAxis::Vertical => {
                            vertical_scroll_id = scroll_bar.id().into();
                            vertical_scroll_handle_id = handle_id.into();
                        }
                    }
                }
            });
        });

        scroll_view.insert(ScrollView {
            viewport,
            content_container,
            horizontal_scroll_bar: horizontal_scroll_id,
            horizontal_scroll_bar_handle: horizontal_scroll_handle_id,
            vertical_scroll_bar: vertical_scroll_id,
            vertical_scroll_bar_handle: vertical_scroll_handle_id,
            ..default()
        });

        scroll_view
    }
}

The scroll view is a tricky set, but not unique in the sense that you would normally populate a node deep in a stack of nodes.

Anyway, this change to the lifetimes also meant that EntityCommands now behave more like the ChildBuilder with an entirely useless access to it's commands (for any serious purposes I mean) so I could no longer meaningfully chain things due to the reduced lifetime. It might seem you get access to an owned Commands in it, but in reality it's the same original Commands with a heavily scoped lifetime so Rust will just stop you from using it.

That said, I decided to ditch the extension on EntityCommands entirely but implement my own insert on the builder. This will make it clear that you are in a UI building chain. while still letting you add your own components. Users would still be able to grab the ID and use the commands they already have access to to grab the EntityCommands if need be.

@eidloi
Copy link
Contributor

eidloi commented Feb 18, 2024

Oh, and of course UI elements can be both root nodes and children of other entities, hence the need to support Commands and EntityCommands under the umbrella.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants