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

Immediate Execution of Commands on a &mut World #6186

Closed
wants to merge 13 commits into from
Closed

Immediate Execution of Commands on a &mut World #6186

wants to merge 13 commits into from

Conversation

Zeenobit
Copy link
Contributor

@Zeenobit Zeenobit commented Oct 6, 2022

Objective

Fixed #6184

Solution

This PR adds an Execute trait which is implemented for &mut World and &mut App. It allows the user to do this:

let mut world = World::default();
let _ = world.execute(|_world, mut commands| {
    /* ... */
});

@mockersf
Copy link
Member

mockersf commented Oct 6, 2022

As you mention in your doc comment, this is a very inefficient way to execute a command when you have a world available.

If this is only to be used for test, I would rather it being not available outside of tests

@Zeenobit
Copy link
Contributor Author

Zeenobit commented Oct 7, 2022

As you mention in your doc comment, this is a very inefficient way to execute a command when you have a world available.

If this is only to be used for test, I would rather it being not available outside of tests

I considered doing this initially. There is a couple of problems with this:

  1. We can't just gate it behind #[cfg(test)], as it would also be useful for examples. So ideally it should be locked behind a feature of its own, but that's just extra build complexity for questionable benefit.
  2. I've found this to be useful for debug UI code, such as with egui, where I want to have a button which executes a bunch of commands when I have &mut World access. So it's also useful for general diagnostics at runtime.

For these reasons, I decided keeping it available but with a warning in the comments is probably the best option.

Also, Execute is not imported with preludes. So that's an extra layer of "protection".

@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 Oct 7, 2022
@alice-i-cecile
Copy link
Member

If this is only to be used for test, I would rather it being not available outside of tests

This came up before in my other PRs for testing utilities. I think that the trait (that's not in the prelude) is a reasonable approach. I would also support the creation of a bevy_testing crate to stick this sort of thing in, which would improve discoverability.

@Zeenobit
Copy link
Contributor Author

Zeenobit commented Oct 7, 2022

This came up before in my other PRs for testing utilities. I think that the trait (that's not in the prelude) is a reasonable approach. I would also support the creation of a bevy_testing crate to stick this sort of thing in, which would improve discoverability.

I like this idea, but I would suggest naming it some more generic like bevy_diagnostics. My rationale is that a lot of stuff like this that's useful for testing would also be useful for stuff like cheats, debug tools, or automation. If we name it bevy_testing, it'd imply it's only useful for testing.

@alice-i-cecile
Copy link
Member

Well, we have bevy_diagnostic. @mockersf, I'd be okay with testing utilities going in there!

@Zeenobit
Copy link
Contributor Author

Let me know if we want to move the entire implementation into a separate file execute.rs under bevy_diagnostic.

@alice-i-cecile
Copy link
Member

Yes please :)

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.

Awesome. Can you add an example for this? Maybe in the root tests directory? I would suggest you link it from the Commands docs but we can't, because circular dependencies.

@Zeenobit
Copy link
Contributor Author

Awesome. Can you add an example for this? Maybe in the root tests directory? I would suggest you link it from the Commands docs but we can't, because circular dependencies.

I added an example under how_to_test_commands.rs. It's hard to come up with a good use case for using a custom command without really explaining a system which might need it. I figured navigation might the best example, but let me know if you have better ideas. And for all regular commands, most of them are accessible through &mut World anyways.

Also, I'm not sure if it was just me, but an issue I discovered while testing:

world.spawn(()) seems to panic with "index out of bounds: the len is 0 but the index is 0". But commands.spawn(()) seems fine.

@alice-i-cecile alice-i-cecile added the C-Testing A change that impacts how we test Bevy or how users test their apps label Oct 10, 2022
@Zeenobit Zeenobit closed this by deleting the head repository Dec 13, 2022
@Zeenobit
Copy link
Contributor Author

Today I realized I accidentally deleted the repository for this. 😢
I'll open a new PR in future.

Zeenobit added a commit to Zeenobit/bevy that referenced this pull request Jan 20, 2023
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-Testing A change that impacts how we test Bevy or how users test their apps 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.

Immediate Execution of Commands on &mut World
3 participants