Simplify commands a bit #268
Conversation
| @@ -3,7 +3,9 @@ | |||
| use bevy::{input::common_conditions::input_just_pressed, prelude::*}; | |||
|
|
|||
| use super::Screen; | |||
| use crate::{asset_tracking::LoadResource, audio::Music, demo::level::SpawnLevel}; | |||
| use crate::{ | |||
| asset_tracking::LoadResource, audio::Music, demo::level::spawn_level as spawn_level_command, | |||
There was a problem hiding this comment.
I don't think the type alias is helpful here. A comment would be better.
There was a problem hiding this comment.
It's needed because we also have function called spawn_level in this file. Imo both of these "deserve" the name, and qualifying this one with level::spawn_level does not show what the difference between them is.
There was a problem hiding this comment.
A point in favor of level::level naming convention 🤐
src/demo/level.rs
Outdated
| world.flush_commands(); | ||
| } | ||
| /// A [`Command`] to spawn the level. | ||
| /// Exclusive systems, i.e. systems that only accept `&mut World`, implement [`Command`]. |
There was a problem hiding this comment.
This is not exactly true: https://docs.rs/bevy_ecs/0.14.1/src/bevy_ecs/system/commands/mod.rs.html#1173-1175
Also I don't think this comment is necessary.
There was a problem hiding this comment.
Considering how much newcomers were struggling with custom commands, I think it would be even more confusing to have two completely different ways of defining them without explanation.
There was a problem hiding this comment.
I thought the struggle with custom commands was that they're unergonomic which makes them look unwieldy, not confusing?
There was a problem hiding this comment.
Better? 70a688d (#268)
There was a problem hiding this comment.
@benfrankel IME everything concerning exclusive access to World is spooky. In general, I also don't expect newcomers to know that this kind of fn implements Command, considering they probably have not ever touched a custom command or seen how a Command is implemented.
As such, I strongly feel that we should have some comment here until the day where custom Commands are accepted by newcomers as a common thing to do.
There was a problem hiding this comment.
Better?
70a688d(#268)
Accurate now, yes. I still think the doc comment calling the function a ['Command'] is sufficient.
There was a problem hiding this comment.
@alice-i-cecile, could you act as a tie-breaker here?
There was a problem hiding this comment.
I think erring on the side of more verbose here is good.
Resolves #265
Resolves #266