Skip to content

Commit 7a748e6

Browse files
alice-i-cecileMiniaczQ
authored andcommitted
Make system param validation rely on the unified ECS error handling via the GLOBAL_ERROR_HANDLER (#18454)
There are two related problems here: 1. Users should be able to change the fallback behavior of *all* ECS-based errors in their application by setting the `GLOBAL_ERROR_HANDLER`. See #18351 for earlier work in this vein. 2. The existing solution (#15500) for customizing this behavior is high on boilerplate, not global and adds a great deal of complexity. The consensus is that the default behavior when a parameter fails validation should be set based on the kind of system parameter in question: `Single` / `Populated` should silently skip the system, but `Res` should panic. Setting this behavior at the system level is a bandaid that makes getting to that ideal behavior more painful, and can mask real failures (if a resource is missing but you've ignored a system to make the Single stop panicking you're going to have a bad day). I've removed the existing `ParamWarnPolicy`-based configuration, and wired up the `GLOBAL_ERROR_HANDLER`/`default_error_handler` to the various schedule executors to properly plumb through errors . Additionally, I've done a small cleanup pass on the corresponding example. I've run the `fallible_params` example, with both the default and a custom global error handler. The former panics (as expected), and the latter spams the error console with warnings 🥲 1. Currently, failed system param validation will result in endless console spam. Do you want me to implement a solution for warn_once-style debouncing somehow? 2. Currently, the error reporting for failed system param validation is very limited: all we get is that a system param failed validation and the name of the system. Do you want me to implement improved error reporting by bubbling up errors in this PR? 3. There is broad consensus that the default behavior for failed system param validation should be set on a per-system param basis. Would you like me to implement that in this PR? My gut instinct is that we absolutely want to solve 2 and 3, but it will be much easier to do that work (and review it) if we split the PRs apart. `ParamWarnPolicy` and the `WithParamWarnPolicy` have been removed completely. Failures during system param validation are now handled via the `GLOBAL_ERROR_HANDLER`: please see the `bevy_ecs::error` module docs for more information. --------- Co-authored-by: MiniaczQ <[email protected]>
1 parent e7fd0d5 commit 7a748e6

File tree

14 files changed

+130
-194
lines changed

14 files changed

+130
-194
lines changed

Cargo.toml

+1
Original file line numberDiff line numberDiff line change
@@ -2203,6 +2203,7 @@ wasm = false
22032203
name = "fallible_params"
22042204
path = "examples/ecs/fallible_params.rs"
22052205
doc-scrape-examples = true
2206+
required-features = ["configurable_error_handler"]
22062207

22072208
[package.metadata.example.fallible_params]
22082209
name = "Fallible System Parameters"

crates/bevy_ecs/src/error/handler.rs

+13-1
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,13 @@ pub enum ErrorContext {
1515
/// The last tick that the system was run.
1616
last_run: Tick,
1717
},
18+
/// The error occurred in a run condition.
19+
RunCondition {
20+
/// The name of the run condition that failed.
21+
name: Cow<'static, str>,
22+
/// The last tick that the run condition was evaluated.
23+
last_run: Tick,
24+
},
1825
/// The error occurred in a command.
1926
Command {
2027
/// The name of the command that failed.
@@ -39,6 +46,9 @@ impl Display for ErrorContext {
3946
Self::Observer { name, .. } => {
4047
write!(f, "Observer `{}` failed", name)
4148
}
49+
Self::RunCondition { name, .. } => {
50+
write!(f, "Run condition `{}` failed", name)
51+
}
4252
}
4353
}
4454
}
@@ -49,7 +59,8 @@ impl ErrorContext {
4959
match self {
5060
Self::System { name, .. }
5161
| Self::Command { name, .. }
52-
| Self::Observer { name, .. } => name,
62+
| Self::Observer { name, .. }
63+
| Self::RunCondition { name, .. } => name,
5364
}
5465
}
5566

@@ -61,6 +72,7 @@ impl ErrorContext {
6172
Self::System { .. } => "system",
6273
Self::Command { .. } => "command",
6374
Self::Observer { .. } => "observer",
75+
Self::RunCondition { .. } => "run condition",
6476
}
6577
}
6678
}

crates/bevy_ecs/src/lib.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ pub mod prelude {
9393
Command, Commands, Deferred, EntityCommand, EntityCommands, In, InMut, InRef,
9494
IntoSystem, Local, NonSend, NonSendMut, ParamSet, Populated, Query, ReadOnlySystem,
9595
Res, ResMut, Single, System, SystemIn, SystemInput, SystemParamBuilder,
96-
SystemParamFunction, WithParamWarnPolicy,
96+
SystemParamFunction,
9797
},
9898
world::{
9999
EntityMut, EntityRef, EntityWorldMut, FilteredResources, FilteredResourcesMut,

crates/bevy_ecs/src/observer/runner.rs

+9-1
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use crate::{
77
observer::{ObserverDescriptor, ObserverTrigger},
88
prelude::*,
99
query::DebugCheckedUnwrap,
10-
system::{IntoObserverSystem, ObserverSystem},
10+
system::{IntoObserverSystem, ObserverSystem, SystemParamValidationError},
1111
world::DeferredWorld,
1212
};
1313
use bevy_ptr::PtrMut;
@@ -416,6 +416,14 @@ fn observer_system_runner<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
416416
);
417417
};
418418
(*system).queue_deferred(world.into_deferred());
419+
} else {
420+
error_handler(
421+
SystemParamValidationError.into(),
422+
ErrorContext::Observer {
423+
name: (*system).name(),
424+
last_run: (*system).get_last_run(),
425+
},
426+
);
419427
}
420428
}
421429
}

crates/bevy_ecs/src/schedule/executor/mod.rs

+2-34
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,7 @@ mod tests {
314314
use crate::{
315315
prelude::{IntoScheduleConfigs, Resource, Schedule, SystemSet},
316316
schedule::ExecutorKind,
317-
system::{Commands, Res, WithParamWarnPolicy},
317+
system::Commands,
318318
world::World,
319319
};
320320

@@ -346,8 +346,7 @@ mod tests {
346346
// This system depends on a system that is always skipped.
347347
(|mut commands: Commands| {
348348
commands.insert_resource(R2);
349-
})
350-
.warn_param_missing(),
349+
}),
351350
)
352351
.chain(),
353352
);
@@ -358,35 +357,4 @@ mod tests {
358357

359358
#[derive(SystemSet, Hash, Debug, PartialEq, Eq, Clone)]
360359
struct S1;
361-
362-
#[test]
363-
fn invalid_condition_param_skips_system() {
364-
for executor in EXECUTORS {
365-
invalid_condition_param_skips_system_core(executor);
366-
}
367-
}
368-
369-
fn invalid_condition_param_skips_system_core(executor: ExecutorKind) {
370-
let mut world = World::new();
371-
let mut schedule = Schedule::default();
372-
schedule.set_executor_kind(executor);
373-
schedule.configure_sets(S1.run_if((|_: Res<R1>| true).warn_param_missing()));
374-
schedule.add_systems((
375-
// System gets skipped if system set run conditions fail validation.
376-
(|mut commands: Commands| {
377-
commands.insert_resource(R1);
378-
})
379-
.warn_param_missing()
380-
.in_set(S1),
381-
// System gets skipped if run conditions fail validation.
382-
(|mut commands: Commands| {
383-
commands.insert_resource(R2);
384-
})
385-
.warn_param_missing()
386-
.run_if((|_: Res<R2>| true).warn_param_missing()),
387-
));
388-
schedule.run(&mut world);
389-
assert!(world.get_resource::<R1>().is_none());
390-
assert!(world.get_resource::<R2>().is_none());
391-
}
392360
}

crates/bevy_ecs/src/schedule/executor/multi_threaded.rs

+21-2
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,11 @@ use tracing::{info_span, Span};
1414

1515
use crate::{
1616
archetype::ArchetypeComponentId,
17-
error::{BevyError, ErrorContext, Result},
17+
error::{default_error_handler, BevyError, ErrorContext, Result},
1818
prelude::Resource,
1919
query::Access,
2020
schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
21-
system::ScheduleSystem,
21+
system::{ScheduleSystem, SystemParamValidationError},
2222
world::{unsafe_world_cell::UnsafeWorldCell, World},
2323
};
2424

@@ -536,6 +536,7 @@ impl ExecutorState {
536536
world: UnsafeWorldCell,
537537
) -> bool {
538538
let mut should_run = !self.skipped_systems.contains(system_index);
539+
let error_handler = default_error_handler();
539540

540541
for set_idx in conditions.sets_with_conditions_of_systems[system_index].ones() {
541542
if self.evaluated_sets.contains(set_idx) {
@@ -582,6 +583,14 @@ impl ExecutorState {
582583
// - `update_archetype_component_access` has been called for system.
583584
let valid_params = unsafe { system.validate_param_unsafe(world) };
584585
if !valid_params {
586+
error_handler(
587+
SystemParamValidationError.into(),
588+
ErrorContext::System {
589+
name: system.name(),
590+
last_run: system.get_last_run(),
591+
},
592+
);
593+
585594
self.skipped_systems.insert(system_index);
586595
}
587596
should_run &= valid_params;
@@ -767,6 +776,8 @@ unsafe fn evaluate_and_fold_conditions(
767776
conditions: &mut [BoxedCondition],
768777
world: UnsafeWorldCell,
769778
) -> bool {
779+
let error_handler = default_error_handler();
780+
770781
#[expect(
771782
clippy::unnecessary_fold,
772783
reason = "Short-circuiting here would prevent conditions from mutating their own state as needed."
@@ -779,6 +790,14 @@ unsafe fn evaluate_and_fold_conditions(
779790
// required by the condition.
780791
// - `update_archetype_component_access` has been called for condition.
781792
if !unsafe { condition.validate_param_unsafe(world) } {
793+
error_handler(
794+
SystemParamValidationError.into(),
795+
ErrorContext::System {
796+
name: condition.name(),
797+
last_run: condition.get_last_run(),
798+
},
799+
);
800+
782801
return false;
783802
}
784803
// SAFETY:

crates/bevy_ecs/src/schedule/executor/simple.rs

+21-1
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,11 @@ use tracing::info_span;
88
use std::eprintln;
99

1010
use crate::{
11-
error::{BevyError, ErrorContext},
11+
error::{default_error_handler, BevyError, ErrorContext},
1212
schedule::{
1313
executor::is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule,
1414
},
15+
system::SystemParamValidationError,
1516
world::World,
1617
};
1718

@@ -88,6 +89,16 @@ impl SystemExecutor for SimpleExecutor {
8889
let system = &mut schedule.systems[system_index];
8990
if should_run {
9091
let valid_params = system.validate_param(world);
92+
if !valid_params {
93+
error_handler(
94+
SystemParamValidationError.into(),
95+
ErrorContext::System {
96+
name: system.name(),
97+
last_run: system.get_last_run(),
98+
},
99+
);
100+
}
101+
91102
should_run &= valid_params;
92103
}
93104

@@ -153,6 +164,8 @@ impl SimpleExecutor {
153164
}
154165

155166
fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut World) -> bool {
167+
let error_handler = default_error_handler();
168+
156169
#[expect(
157170
clippy::unnecessary_fold,
158171
reason = "Short-circuiting here would prevent conditions from mutating their own state as needed."
@@ -161,6 +174,13 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W
161174
.iter_mut()
162175
.map(|condition| {
163176
if !condition.validate_param(world) {
177+
error_handler(
178+
SystemParamValidationError.into(),
179+
ErrorContext::RunCondition {
180+
name: condition.name(),
181+
last_run: condition.get_last_run(),
182+
},
183+
);
164184
return false;
165185
}
166186
__rust_begin_short_backtrace::readonly_run(&mut **condition, world)

crates/bevy_ecs/src/schedule/executor/single_threaded.rs

+20-1
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,9 @@ use tracing::info_span;
88
use std::eprintln;
99

1010
use crate::{
11-
error::{BevyError, ErrorContext},
11+
error::{default_error_handler, BevyError, ErrorContext},
1212
schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
13+
system::SystemParamValidationError,
1314
world::World,
1415
};
1516

@@ -94,6 +95,15 @@ impl SystemExecutor for SingleThreadedExecutor {
9495
let system = &mut schedule.systems[system_index];
9596
if should_run {
9697
let valid_params = system.validate_param(world);
98+
if !valid_params {
99+
error_handler(
100+
SystemParamValidationError.into(),
101+
ErrorContext::System {
102+
name: system.name(),
103+
last_run: system.get_last_run(),
104+
},
105+
);
106+
}
97107
should_run &= valid_params;
98108
}
99109

@@ -196,6 +206,8 @@ impl SingleThreadedExecutor {
196206
}
197207

198208
fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut World) -> bool {
209+
let error_handler: fn(BevyError, ErrorContext) = default_error_handler();
210+
199211
#[expect(
200212
clippy::unnecessary_fold,
201213
reason = "Short-circuiting here would prevent conditions from mutating their own state as needed."
@@ -204,6 +216,13 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W
204216
.iter_mut()
205217
.map(|condition| {
206218
if !condition.validate_param(world) {
219+
error_handler(
220+
SystemParamValidationError.into(),
221+
ErrorContext::RunCondition {
222+
name: condition.name(),
223+
last_run: condition.get_last_run(),
224+
},
225+
);
207226
return false;
208227
}
209228
__rust_begin_short_backtrace::readonly_run(&mut **condition, world)

0 commit comments

Comments
 (0)