-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(bevy_ecs): add Severity metadata to BevyError
#22201
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
base: main
Are you sure you want to change the base?
Conversation
|
To add to the future work section, I do feel like it's a good idea to also add the With just the |
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum Severity { | ||
| /// The error can be safely ignored. | ||
| Ignore, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to have Debug here as well, for errors that are lower severity than warnings but may still need to be surfaced during debugging.
NthTensor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty much what I was envisioning, nice work! Obviously there's more to do, but I'm happy with this.
|
Thanks for the feedback! @NthTensor: I’ll add @Freyja-moth: I understand your concerns—thanks for pointing this out! The preferred default for error severity should be |
|
The idea is to have the trait be the default with the option to override it trait ErrorSeverity: Error {
fn severity(&self) -> SeverityLevel;
}
pub struct BevyError {
error: Box<dyn ErrorSeverity>,
severity: Option<SeverityLevel>
}
impl BevyError {
pub fn new(error: impl ErrorSeverity) -> Self {
Self {
error: Box::new(error),
severity: None,
}
}
pub fn with_severity(&mut self, severity: SeverityLevel) {
self.severity.replace(severity);
}
pub fn severity(&self) -> SeverityLevel {
self.severity.unwrap_or(self.error.severity())
}
} |
|
I didn’t modify the When thinking more about using The problem seems to be: What is the right default? With |
|
Yes sorry, I'm not familiar with the internals of
It wouldn't, as the error severity would default to the error severity as specified by the trait. To give an example pub struct EntityDoesNotMatchError;
impl ErrorSeverity for EntityDoesNotMatchError {
fn severity() -> SeverityLevel {
SeverityLevel::Critical
}
}
fn tester() {
let mut result = Err(BevyError::new(EntityDoesNotMatchError)); // `BevyError::severity` currently returns SeverityLevel::Critical since `EntityDoesNotMatchError::severity` returns `SeverityLevel::Critical` and `InnerBevyError.severity` is `None`
result.with_severity(SeverityLevel::Ignore); // `BevyError::severity` currently returns SeverityLevel::Ignore, since `InnerBevyError.severity` is now no longer `None` and is used instead of `EntityDoesNotMatchError::severity`
} |
|
Looking into it further doing this would introduce issues with non bevy errors as they wouldn't be able to be stored. There's currently no way to do what I'd suggested while allowing non bevy errors, but once specialization is added we should be able to add a |
Objective
As outlined in #20561, Bevy's default error handler currently panics on most errors. In some cases, this is overly strict—e.g., inserting a component into a despawned entity is harmless and could be handled more gracefully in library code.
This PR implements the idea suggested by @Freyja-moth: each
BevyErrornow carries aSeverity, and theResultSeverityExttrait allows overriding it. These changes lay the foundation for error handlers that can log, ignore, or panic based on severity.Happy to adjust based on feedback.
Solution
Severityenum.InnerBevyErrorto store aSeverity, defaulting toSeverity::Criticalto preserve existing panic-on-error behavior.BevyErrorexposesseverity()andwith_severity()methods.ResultSeverityExtprovides ergonomic overriding of error severity onResult<T, E>.Testing
bevy_ecspass (with thedebugfeature enabled); existing "should panic" tests continue to behave as expected.Future TODOs