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 should be a trait, and other Bevy-builtin SystemParams. #3782

Closed
VoilaNeighbor opened this issue Jan 27, 2022 · 7 comments
Closed
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A simple quality-of-life change that makes Bevy easier to use S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged

Comments

@VoilaNeighbor
Copy link

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

Inversion-of-Control (IoC) frameworks should not be intrusive, or the code will be hard to test. Currently, quite a little functionality in Bevy are implemented directly as concrete structs. This makes unit tests with systems nearly impossible without bringing up a Bevy environment (World and Stage, at least). This also couples the gameplay code deeper with the existing implementation.

What solution would you like?

We can introduce traits for structs such as Commands, Res. Since Bevy supports generic functions, it is easy to switch to using impl Commands. This costs us a little ergonomics, but we can still use the concrete one anyway if an impl keyword means too much.

What alternative(s) have you considered?

It is possible without an API change, by transforming structs such as Commands into sum types, i.e. one with the real logic, and one with mocking implementations. In tests, users can check actions performed upon the mocks. Overhead on dispatching is possible, but I guess it being neglectable thanks to modern CPU branch-prediction.

@VoilaNeighbor VoilaNeighbor added C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels Jan 27, 2022
@alice-i-cecile alice-i-cecile added S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A simple quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled C-Enhancement A new feature labels Jan 27, 2022
@VoilaNeighbor
Copy link
Author

Wow. You guys are reacting blazingly fast!

@alice-i-cecile
Copy link
Member

There's a Discord channel that tracks new Github issues ;) And this one is particularly interesting to me.

@VoilaNeighbor
Copy link
Author

This is what I'm currently doing to remove the boilerplate.

pub fn run_once<Params, D>(systems: impl IntoIterator<Item = D>) -> World
where
	D: IntoSystemDescriptor<Params>,
{
	let mut stage = SystemStage::parallel();
	for i in systems {
		stage.add_system(i);
	}
	let mut world = World::new();
	// Insert some dummy resources for use.
	world.insert_resource(PlayerImage(Handle::default()));
	stage.run(&mut world);
	world
}

/// Queries the world. Returns an iterator. Different from World::query
/// which returns a [QueryState].
macro_rules! query {
	($world: ident, $components: ty) => {
		$world.query::<$components>().iter(&$world)
	};
}

@alice-i-cecile
Copy link
Member

So, I can definitely empathize with some of your frustrations around testing, and this issue is closely related to #2192. The inability to construct Res outside of a World is very frustrating, and working with commands when using exclusive world access is nearly impossible (#3096).

However, I suspect that you're not familiar with the absolutely wonderful SystemState API. It allows you to extract elements like Res, Query, EventWriter and more from the World, in a way that bypasses the borrow checker.

For example:

        let mut input_system_state: SystemState<(
            Option<ResMut<Input<GamepadButton>>>,
            Option<ResMut<Input<KeyCode>>>,
            Option<ResMut<Input<MouseButton>>>,
        )> = SystemState::new(self);

        let (mut maybe_gamepad, mut maybe_keyboard, mut maybe_mouse) =
            input_system_state.get_mut(self);

This is underdocumented though; I was actually just about to prepare a PR for it.

@VoilaNeighbor
Copy link
Author

This SystemState API must be very useful for debugging and testing!

@TheRawMeatball
Copy link
Member

I'm pretty sure using impl Res etc. isn't feasible given how the system API works. The inputs need to be concrete types for functions to properly coerce to systems and relevant lifetime information to exist. Especially given how the system state api seems to be covering this use case, I'm closing this.

@VoilaNeighbor
Copy link
Author

Sure. It did not cost me much time to realize that it is almost impossible to mock system parameters without actually making them.

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-Code-Quality A section of code that is hard to understand or change C-Usability A simple quality-of-life change that makes Bevy easier to use S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged
Projects
None yet
Development

No branches or pull requests

3 participants