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 apply at unexpected times with exclusive World access #14621

Closed
jrobsonchase opened this issue Aug 5, 2024 · 3 comments · Fixed by #16219
Closed

Commands apply at unexpected times with exclusive World access #14621

jrobsonchase opened this issue Aug 5, 2024 · 3 comments · Fixed by #16219
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior C-Usability A targeted 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 X-Controversial There is active debate or serious implications around merging this PR

Comments

@jrobsonchase
Copy link
Contributor

Bevy version

0.14.0 and main (c1c003d)

What you did

world.commands().add(...) from within another command. Take the following example:

let mut world = World::new();

let mut cmd = world.commands();

// Command 1
cmd.add(|world: &mut World| {
    let entity_1 = world.spawn_empty().id();

    println!("A");

    // Command 3
    world.commands().add(|world: &mut World| {
        println!("BUZZ!");
    });

    println!("B");

    world.entity_mut(entity_1).insert(A);

    let a = world.get::<A>(entity_1);

    println!("C");

    world.spawn(A);

    println!("D");

    world.flush();

    println!("E");
});

// Command 2
cmd.add(|_: &mut World| {
    println!("F");
});

world.flush();

When does BUZZ! get printed?

What went wrong

It ends up being in the place I would least expect, which is why I'm calling this a bug rather than simply lacking documentation.

Loosely ordered by where I would expect it to occur:

  • After F? If the &mut World passed to each command is the same world as the outermost, then I would expect world.commands() to always refer to the same queue, so Command 1 is added, Command 2 is added, Command 1 runs, it pushes Command 3 into the queue, Command 2 runs, Command 3 runs.
  • After D or E? If each command gets a new "nested" queue that's flushed after it finishes, then it would happen between Command 1 and Command 2. But we're also calling world.flush(), and the docs for world.commands() says they'll run after a call to World::flush. So it should be after D.
  • After A? We have exclusive World access, so Commands are a bit redundant. If we ignore the bit about World::flush in the World::commands docs, then maybe we'll guess that they simply execute immediately, and World::commands is just for convenience when you have something that accepts a Commands argument. Or maybe they flush when the Commands is immediately dropped after our add?
  • After B? Maybe there's an implicit World::flush any time we actually mutate the world or when we read the result of a mutation.
  • After C? If it's none of the above, this is the only option left, apart from "never".

And the winner is... after C! It comes down to World::flush being implicit in some methods, but not others. There isn't any mention of this in any of their docs. Even if it was called out explicitly in the docs for each method if it does a flush, I wouldn't want to have to keep that context in my head when reasoning about when a command will run.

To me, the only three possibilities for when a command should run given exclusive world access are:

  1. At the end of the outermost command queue
  2. Immediately after the parent command
  3. Immediately after being added, or at least before any other world access. No explicit flushing or "will they/won't they" implicit flushing.

Additional information

This comes out of my attempts to get lifecycle Observers to behave the way I want, and they make things even harder to reason about. Specifically, the OnAdd trigger will occur immediately upon component addition, but if you add commands to the queue in the trigger, they'll be executed whenever the command that did the component addition does a flush, which can be wildly unpredictable. For example, what if the command pre-spawns a bunch of entities, then adds components? Your observer's commands will run at the end of that command after all components have been added. What if instead, it interleaves entity spawning and component adding? Then you'll get a flush before each new entity, and your observer's commands will occur midway through the command that triggered it.

@jrobsonchase jrobsonchase added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Aug 5, 2024
@TrialDragon TrialDragon added A-ECS Entities, components, systems, and events C-Usability A targeted 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 and removed S-Needs-Triage This issue needs to be labelled labels Aug 5, 2024
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Sep 16, 2024
@alice-i-cecile
Copy link
Member

For the record, this is intended behavior, and I don't think we should change this.

world.commands().add(...) from within another command

Generally I don't think users should be doing this, precisely because of how complex it makes things to debug. I've already opened a proposal for a lint against this: TheBevyFlock/bevy_cli#77

@jrobsonchase
Copy link
Contributor Author

Generally I don't think users should be doing this, precisely because of how complex it makes things to debug. I've already opened a proposal for a lint against this

Then should there also be a lint against using Commands from within observers or component hooks? It feels like the same situation, just with more indirection.

@nakedible
Copy link
Contributor

Then should there also be a lint against using Commands from within observers or component hooks? It feels like the same situation, just with more indirection.

The only way to add or remove components from within observers and component hooks is to use Commands, as the world is a deferred world.

github-merge-queue bot pushed a commit that referenced this issue Dec 5, 2024
# Objective

- Currently adding observers spawns an entity which implicitly flushes
the command queue, which can cause undefined behaviour if the
`WorldEntityMut` is used after this
- The reason `WorldEntityMut` attempted to (unsuccessfully) avoid
flushing commands until finished was that such commands may move or
despawn the entity being referenced, invalidating the cached location.
- With the introduction of hooks and observers, this isn't sensible
anymore as running the commands generated by hooks immediately is
required to maintain correct ordering of operations and to not expose
the world in an inconsistent state
- Objective is to make command flushing deterministic and fix the
related issues
- Fixes #16212
- Fixes #14621 
- Fixes #16034

## Solution

- Allow `WorldEntityMut` to exist even when it refers to a despawned
entity by allowing `EntityLocation` to be marked invalid
- Add checks to all methods to panic if trying to access a despawned
entity
- Flush command queue after every operation that might trigger hooks or
observers
- Update entity location always after flushing command queue

## Testing

- Added test cases for currently broken behaviour
- Added test cases that flushes happen in all operations
- Added test cases to ensure hooks and commands are run exactly in
correct order when nested

---

Todo:

- [x] Write migration guide
- [x] Add tests that using `EntityWorldMut` on a despawned entity panics
- [x] Add tests that commands are flushed after every operation that is
supposed to flush them
- [x] Add tests that hooks, observers and their spawned commands are run
in the correct order when nested

---

## Migration Guide

Previously `EntityWorldMut` triggered command queue flushes in
unpredictable places, which could interfere with hooks and observers.
Now the command queue is flushed always immediately after any call in
`EntityWorldMut` that spawns or despawns an entity, or adds, removes or
replaces a component. This means hooks and observers will run their
commands in the correct order.

As a side effect, there is a possibility that a hook or observer could
despawn the entity that is being referred to by `EntityWorldMut`. This
could already currently happen if an observer was added while keeping an
`EntityWorldMut` referece and would cause unsound behaviour. If the
entity has been despawned, calling any methods which require the entity
location will panic. This matches the behaviour that `Commands` will
panic if called on an already despawned entity. In the extremely rare
case where taking a new `EntityWorldMut` reference or otherwise
restructuring the code so that this case does not happen is not
possible, there's a new `is_despawned` method that can be used to check
if the referred entity has been despawned.
ecoskey pushed a commit to ecoskey/bevy that referenced this issue Jan 6, 2025
…6219)

# Objective

- Currently adding observers spawns an entity which implicitly flushes
the command queue, which can cause undefined behaviour if the
`WorldEntityMut` is used after this
- The reason `WorldEntityMut` attempted to (unsuccessfully) avoid
flushing commands until finished was that such commands may move or
despawn the entity being referenced, invalidating the cached location.
- With the introduction of hooks and observers, this isn't sensible
anymore as running the commands generated by hooks immediately is
required to maintain correct ordering of operations and to not expose
the world in an inconsistent state
- Objective is to make command flushing deterministic and fix the
related issues
- Fixes bevyengine#16212
- Fixes bevyengine#14621 
- Fixes bevyengine#16034

## Solution

- Allow `WorldEntityMut` to exist even when it refers to a despawned
entity by allowing `EntityLocation` to be marked invalid
- Add checks to all methods to panic if trying to access a despawned
entity
- Flush command queue after every operation that might trigger hooks or
observers
- Update entity location always after flushing command queue

## Testing

- Added test cases for currently broken behaviour
- Added test cases that flushes happen in all operations
- Added test cases to ensure hooks and commands are run exactly in
correct order when nested

---

Todo:

- [x] Write migration guide
- [x] Add tests that using `EntityWorldMut` on a despawned entity panics
- [x] Add tests that commands are flushed after every operation that is
supposed to flush them
- [x] Add tests that hooks, observers and their spawned commands are run
in the correct order when nested

---

## Migration Guide

Previously `EntityWorldMut` triggered command queue flushes in
unpredictable places, which could interfere with hooks and observers.
Now the command queue is flushed always immediately after any call in
`EntityWorldMut` that spawns or despawns an entity, or adds, removes or
replaces a component. This means hooks and observers will run their
commands in the correct order.

As a side effect, there is a possibility that a hook or observer could
despawn the entity that is being referred to by `EntityWorldMut`. This
could already currently happen if an observer was added while keeping an
`EntityWorldMut` referece and would cause unsound behaviour. If the
entity has been despawned, calling any methods which require the entity
location will panic. This matches the behaviour that `Commands` will
panic if called on an already despawned entity. In the extremely rare
case where taking a new `EntityWorldMut` reference or otherwise
restructuring the code so that this case does not happen is not
possible, there's a new `is_despawned` method that can be used to check
if the referred entity has been despawned.
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-Bug An unexpected or incorrect behavior C-Usability A targeted 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 X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants