report a command system origin in case of panic#2222
report a command system origin in case of panic#2222mockersf wants to merge 5 commits intobevyengine:mainfrom
Conversation
ca9467f to
4c87867
Compare
| commands: Vec<Box<dyn Command>>, | ||
| pub(crate) commands: Vec<Box<dyn Command>>, | ||
| #[cfg(feature = "command_panic_origin")] | ||
| pub(crate) system_name: Option<Cow<'static, str>>, |
There was a problem hiding this comment.
Should this be Option<_>?
inside fn init system_name is always set as Some.
There was a problem hiding this comment.
the struct CommandQueue is pub and can be built using default() (done in LightsNode and in RenderResourcesNode). In those case a None is better than an empty string I think.
Though with the field behind a feature it's less of an issue I guess
There was a problem hiding this comment.
Could we just impl Default for CommandQueue?
There was a problem hiding this comment.
Unless it's actually possible for it to be None...
Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>
|
The new feature also needs to be mentioned in the ./docs/cargo_features.md file. |
|
@mockersf .on_err(CommandErrorHandler::panic())which, I have a feeling will be few and far between. It's probably a better approach to have the struct CommandArgs<'a> {
world: &'a mut World,
system_name: Cow<'static, str>,
}
trait Command {
fn write(self: Box<Self>, args: CommandArgs);
}So that way all commands (and error handlers) can have the system name correctly propagated to them? |
I think this'll definitely to be looked at after #2241 has been merged, moved it to draft
Or having the system name in the |
Yep, it'll be there to. |
related to #1743 and #2219
Add a new feature
bevy_command_panic_origin. If it is enabled, the command system origin will be logged in case of panic when applying it.This is not meant to be a final feature with nice UX but more a stepping stone for adding proper result handling of commands. But it can be useful as is, and adding error to all commands is a much larger change #2004