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

report a command system origin in case of panic #2222

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ trace_chrome = ["bevy_internal/trace_chrome"]
trace = ["bevy_internal/trace"]
wgpu_trace = ["bevy_internal/wgpu_trace"]

bevy_command_panic_origin = ["bevy_internal/bevy_command_panic_origin"]

# Image format support for texture loading (PNG and HDR are enabled by default)
hdr = ["bevy_internal/hdr"]
png = ["bevy_internal/png"]
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_ecs/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ categories = ["game-engines", "data-structures"]

[features]
trace = []
command_panic_origin = []
default = ["bevy_reflect"]

[dependencies]
Expand Down
26 changes: 25 additions & 1 deletion crates/bevy_ecs/src/system/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,14 @@ use crate::{
world::World,
};
use bevy_utils::tracing::debug;
#[cfg(feature = "command_panic_origin")]
use bevy_utils::tracing::error;
use std::marker::PhantomData;
#[cfg(feature = "command_panic_origin")]
use std::{
borrow::Cow,
panic::{self, AssertUnwindSafe},
};

/// A [`World`] mutation.
pub trait Command: Send + Sync + 'static {
Expand All @@ -15,7 +22,9 @@ pub trait Command: Send + Sync + 'static {
/// A queue of [`Command`]s.
#[derive(Default)]
pub struct CommandQueue {
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>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be Option<_>?
inside fn init system_name is always set as Some.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just impl Default for CommandQueue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless it's actually possible for it to be None...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be None!

}

impl CommandQueue {
Expand All @@ -24,7 +33,22 @@ impl CommandQueue {
pub fn apply(&mut self, world: &mut World) {
world.flush();
for command in self.commands.drain(..) {
// TODO: replace feature by proper error handling from commands
// https://github.com/bevyengine/bevy/issues/2004
#[cfg(not(feature = "command_panic_origin"))]
command.write(world);
#[cfg(feature = "command_panic_origin")]
{
let may_panic = panic::catch_unwind(AssertUnwindSafe(|| {
command.write(world);
}));
if may_panic.is_err() {
if let Some(system_name) = &self.system_name {
error!("panic while applying a command from {}", system_name);
}
panic!("panic applying a command");
}
mockersf marked this conversation as resolved.
Show resolved Hide resolved
}
mockersf marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_ecs/src/system/into_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ impl SystemState {
pub fn set_non_send(&mut self) {
self.is_send = false;
}

#[cfg(feature = "command_panic_origin")]
pub fn name(&self) -> Cow<'static, str> {
self.name.clone()
}
}

/// Conversion trait to turn something into a [`System`].
Expand Down
9 changes: 7 additions & 2 deletions crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -529,8 +529,13 @@ impl<'a> SystemParam for Commands<'a> {
unsafe impl SystemParamState for CommandQueue {
type Config = ();

fn init(_world: &mut World, _system_state: &mut SystemState, _config: Self::Config) -> Self {
Default::default()
#[allow(unused)]
fn init(_world: &mut World, system_state: &mut SystemState, _config: Self::Config) -> Self {
CommandQueue {
#[cfg(feature = "command_panic_origin")]
system_name: Some(system_state.name()),
mockersf marked this conversation as resolved.
Show resolved Hide resolved
..Default::default()
}
}

fn apply(&mut self, world: &mut World) {
Expand Down
3 changes: 3 additions & 0 deletions crates/bevy_internal/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ x11 = ["bevy_winit/x11"]
# enable rendering of font glyphs using subpixel accuracy
subpixel_glyph_atlas = ["bevy_text/subpixel_glyph_atlas"]

# enable tracing system origin of commands in case of command failure
bevy_command_panic_origin = ["bevy_ecs/command_panic_origin"]

# enable systems that allow for automated testing on CI
bevy_ci_testing = ["bevy_app/bevy_ci_testing"]

Expand Down