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

Commands from entities #6733

Closed
wants to merge 3 commits into from
Closed

Conversation

hymm
Copy link
Contributor

@hymm hymm commented Nov 22, 2022

Objective

  • Allow creating a command buffer that can be stored in world.

Changelog

  • Add from_entities builder to Commands

@alice-i-cecile
Copy link
Member

Very cool! I think this also fixes #6184.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use labels Nov 23, 2022
@@ -113,6 +113,10 @@ impl<'w, 's> Commands<'w, 's> {
}
}

pub fn from_entities(queue: &'s mut CommandQueue, entities: &'w Entities) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Can we alter the above constructor in the terms of this one?

@@ -190,6 +190,7 @@ Example | Description

Example | Description
--- | ---
[CommandQueue in World](../examples/ecs/command_queue_in_world.rs) | Manually apply a command Queue
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a top level example or a doc example/test?

@james7132
Copy link
Member

My only concern about this is that Commands can reserve entities without populating them until later. Does this break any safety invariant if we, for whatever reason, allow indefinitely deferring the application of commands?

@hymm
Copy link
Contributor Author

hymm commented Nov 23, 2022

Not a safety invariant, but dropping the queue probably does "leak" entity id's. Not sure there is a way to clean them up, even manually.

Comment on lines +39 to +41
fn log_entity(query: Query<Entity, With<EntityMarker>>) {
if let Ok(_entitiy) = query.get_single() {
println!("found entitiy");
Copy link
Member

@JoJoJet JoJoJet Nov 26, 2022

Choose a reason for hiding this comment

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

Suggested change
fn log_entity(query: Query<Entity, With<EntityMarker>>) {
if let Ok(_entitiy) = query.get_single() {
println!("found entitiy");
fn log_entity(query: Query<Entity, With<EntityMarker>>) {
if let Ok(entity) = query.get_single() {
println!("found {entity:?}");

Copy link
Member

Choose a reason for hiding this comment

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

Probably should use query.get_single().is_ok() if we aren't using the value.

Comment on lines +25 to +26

fn spawn_something(entities: &Entities, mut q: ResMut<MyCommandQueue>) {
Copy link
Member

Choose a reason for hiding this comment

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

Entities is a fairly niche system param, so I think this example could use a quick explanation of what it is.

@@ -113,6 +113,10 @@ impl<'w, 's> Commands<'w, 's> {
}
}

pub fn from_entities(queue: &'s mut CommandQueue, entities: &'w Entities) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed that Commands already has the function new_from_entities(&'s mut CommandQueue, &'w Entities). Can you just use that instead?

@hymm
Copy link
Contributor Author

hymm commented Dec 23, 2022

going to close this as pointed out the function already exists. I don't think that using a command queue as in the example is a good idea at present, since it'll "leak" entities if the command queue isn't applied. So there needs to be a way of cleaning up the entities when the queue is dropped.

@hymm hymm closed this Dec 23, 2022
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-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants