-
-
Notifications
You must be signed in to change notification settings - Fork 4k
Better warnings about invalid parameters #15500
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
Changes from all commits
c3329c2
5f1cdb5
158de13
6e8e8c5
6bc20f0
137c53f
5b1c228
d6338be
370735c
6ed333f
1801baa
caaa623
98dc20f
c5801e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,7 @@ pub struct SystemMeta { | |
is_send: bool, | ||
has_deferred: bool, | ||
pub(crate) last_run: Tick, | ||
param_warn_policy: ParamWarnPolicy, | ||
#[cfg(feature = "trace")] | ||
pub(crate) system_span: Span, | ||
#[cfg(feature = "trace")] | ||
|
@@ -59,6 +60,7 @@ impl SystemMeta { | |
is_send: true, | ||
has_deferred: false, | ||
last_run: Tick::new(0), | ||
param_warn_policy: ParamWarnPolicy::Once, | ||
#[cfg(feature = "trace")] | ||
system_span: info_span!("system", name = name), | ||
#[cfg(feature = "trace")] | ||
|
@@ -75,6 +77,7 @@ impl SystemMeta { | |
/// Sets the name of of this system. | ||
/// | ||
/// Useful to give closure systems more readable and unique names for debugging and tracing. | ||
#[inline] | ||
pub fn set_name(&mut self, new_name: impl Into<Cow<'static, str>>) { | ||
let new_name: Cow<'static, str> = new_name.into(); | ||
#[cfg(feature = "trace")] | ||
|
@@ -108,9 +111,94 @@ impl SystemMeta { | |
|
||
/// Marks the system as having deferred buffers like [`Commands`](`super::Commands`) | ||
/// This lets the scheduler insert [`apply_deferred`](`crate::prelude::apply_deferred`) systems automatically. | ||
#[inline] | ||
pub fn set_has_deferred(&mut self) { | ||
self.has_deferred = true; | ||
} | ||
|
||
/// Changes the warn policy. | ||
#[inline] | ||
pub(crate) fn set_param_warn_policy(&mut self, warn_policy: ParamWarnPolicy) { | ||
self.param_warn_policy = warn_policy; | ||
} | ||
|
||
/// Advances the warn policy after validation failed. | ||
#[inline] | ||
pub(crate) fn advance_param_warn_policy(&mut self) { | ||
self.param_warn_policy.advance(); | ||
} | ||
|
||
/// Emits a warning about inaccessible system param if policy allows it. | ||
#[inline] | ||
pub fn try_warn_param<P>(&self) | ||
where | ||
P: SystemParam, | ||
{ | ||
self.param_warn_policy.try_warn::<P>(&self.name); | ||
} | ||
} | ||
|
||
/// State machine for emitting warnings when [system params are invalid](System::validate_param). | ||
#[derive(Clone, Copy)] | ||
pub enum ParamWarnPolicy { | ||
/// No warning should ever be emitted. | ||
Never, | ||
/// The warning will be emitted once and status will update to [`Self::Never`]. | ||
Once, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Future PR: Seems reasonable to add an /// State machine for emitting warnings when [system params are invalid](System::validate_param).
#[derive(Clone, Copy)]
pub enum ParamWarnPolicy {
/// No warning should ever be emitted.
Never,
/// The warning will be emitted once and status will update to [`Self::Never`].
Once,
/// The warning will be emitted each time the system is run.
Always,
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had it but removed it as per previous feedback.
If we get some higher level logging with editor, we can do stuff like:
|
||
} | ||
|
||
impl ParamWarnPolicy { | ||
/// Advances the warn policy after validation failed. | ||
#[inline] | ||
fn advance(&mut self) { | ||
*self = Self::Never; | ||
} | ||
|
||
/// Emits a warning about inaccessible system param if policy allows it. | ||
#[inline] | ||
fn try_warn<P>(&self, name: &str) | ||
where | ||
P: SystemParam, | ||
{ | ||
if matches!(self, Self::Never) { | ||
return; | ||
} | ||
|
||
bevy_utils::tracing::warn!( | ||
"{0} did not run because it requested inaccessible system parameter {1}", | ||
name, | ||
disqualified::ShortName::of::<P>() | ||
); | ||
} | ||
} | ||
|
||
/// Trait for manipulating warn policy of systems. | ||
#[doc(hidden)] | ||
pub trait WithParamWarnPolicy<M, F> | ||
where | ||
M: 'static, | ||
F: SystemParamFunction<M>, | ||
Self: Sized, | ||
{ | ||
/// Set warn policy. | ||
fn with_param_warn_policy(self, warn_policy: ParamWarnPolicy) -> FunctionSystem<M, F>; | ||
|
||
/// Disable all param warnings. | ||
fn never_param_warn(self) -> FunctionSystem<M, F> { | ||
self.with_param_warn_policy(ParamWarnPolicy::Never) | ||
} | ||
} | ||
|
||
impl<M, F> WithParamWarnPolicy<M, F> for F | ||
where | ||
M: 'static, | ||
F: SystemParamFunction<M>, | ||
{ | ||
fn with_param_warn_policy(self, param_warn_policy: ParamWarnPolicy) -> FunctionSystem<M, F> { | ||
let mut system = IntoSystem::into_system(self); | ||
system.system_meta.set_param_warn_policy(param_warn_policy); | ||
system | ||
} | ||
} | ||
|
||
// TODO: Actually use this in FunctionSystem. We should probably only do this once Systems are constructed using a World reference | ||
|
@@ -657,9 +745,18 @@ where | |
} | ||
|
||
#[inline] | ||
unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool { | ||
unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool { | ||
let param_state = self.param_state.as_ref().expect(Self::PARAM_MESSAGE); | ||
F::Param::validate_param(param_state, &self.system_meta, world) | ||
// SAFETY: | ||
// - The caller has invoked `update_archetype_component_access`, which will panic | ||
// if the world does not match. | ||
// - All world accesses used by `F::Param` have been registered, so the caller | ||
// will ensure that there are no data access conflicts. | ||
let is_valid = unsafe { F::Param::validate_param(param_state, &self.system_meta, world) }; | ||
if !is_valid { | ||
self.system_meta.advance_param_warn_policy(); | ||
} | ||
is_valid | ||
} | ||
|
||
#[inline] | ||
|
Uh oh!
There was an error while loading. Please reload this page.