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

Ergonomic tools for working with Commands from &mut World #3096

Closed
alice-i-cecile opened this issue Nov 9, 2021 · 8 comments
Closed

Ergonomic tools for working with Commands from &mut World #3096

alice-i-cecile opened this issue Nov 9, 2021 · 8 comments
Labels
A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use S-Needs-Design This issue requires design work to think about how it would best be accomplished

Comments

@alice-i-cecile
Copy link
Member

What problem does this solve or what need does it fill?

Even when working with exclusive world access, work often needs to be deferred to satisfy soundness.

This occurs most commonly when iterating over a query or in other looping contexts.

What solution would you like?

Add World::commands(), which fetches a fresh instance of Commands.

Add World::apply_commands(), for a simple

What alternative(s) have you considered?

This is a partial, tightly-scoped alternative to some of the frustrations that inspired bevyengine/rfcs#42.

Additional context

World::apply_commands can currently be achieved with this snippet:

use bevy::prelude::*;
use bevy::ecs::system::SystemState;

let world = World::new();

let mut system_state = SystemState::<Commands>::new(&mut world);

// Manually adding a command to the list to verify that this works
let mut commands = system_state.get_mut();
commands.spawn();

// Applies all accumulated commands to the world, causing them to take immediate effect
system_state.apply(&mut world);
@alice-i-cecile alice-i-cecile added C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use and removed C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels Nov 9, 2021
@cart
Copy link
Member

cart commented Nov 9, 2021

Quick callout that Commands requires an &Entities reference, which is owned by World. This would block direct mutable Word accesses, which means by itself it isn't a suitable alternative to bevyengine/rfcs#42

@alice-i-cecile
Copy link
Member Author

Quick callout that Commands requires an &Entities reference, which is owned by World. This would block direct mutable Word accesses, which means by itself it isn't a suitable alternative to bevyengine/rfcs#42

Hmm, right 🤔 I'm running into this with my naive impl.

@Davier
Copy link
Contributor

Davier commented Nov 9, 2021

Another note: this used to be possible in bevy 0.4, thanks to entity reservation being behind a mutex.

@cart
Copy link
Member

cart commented Nov 10, 2021

Another note: this used to be possible in bevy 0.4, thanks to entity reservation being behind a mutex.

Actually not quite. Entity reservation is thread safe due to how it uses atomics (both in 0.4 and now), but it was never behind a mutex. We just shared &Entities references using what can only be described as "disgusting disregard for everything Rust works so hard to prevent". (I'm using this uncharitable phrasing because I wrote the code).

#[derive(Debug)]

@cart
Copy link
Member

cart commented Nov 10, 2021

The current implementation plays by the rules, but also makes it impossible to do what we did in 0.4.

@alice-i-cecile alice-i-cecile added the S-Needs-Design This issue requires design work to think about how it would best be accomplished label Nov 10, 2021
@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Nov 10, 2021

Playing with a draft of this, there are two serious problems that must be overcome.

  1. As discussed above, we need a &Entities to initialize a new Commands. This is required to verify that an entity exists before operating on it with EntityCommands, as well as to reserve entities for Commands.spawn.
  2. We need to store a CommandQueue somewhere. Naively adding it as a field to World won't work, because then we can't mutate the world as that would be self-referential.

When Commands is used as a system parameter, the second problem is solved by storing the CommandQueue in the Fetch of the system parameter, which is stored in the FunctionSystem, which is then owned by a stage and ultimately a schedule (which lives outside the world).

We could get around the second, less serious issue by forcing users to create a new CommandQueue in their exclusive system, and then manually applying that. This is the solution currently taken by the test cases that use commands. I'm not a huge fan: it's verbose and exposes CommandQueue to the user, when that should largely be an implementation detail.

@alice-i-cecile
Copy link
Member Author

#3946 makes significant progress towards this; see the included example.

@alice-i-cecile
Copy link
Member Author

Closed in favor of #6184.

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 S-Needs-Design This issue requires design work to think about how it would best be accomplished
Projects
None yet
Development

No branches or pull requests

3 participants