fix world.is_resource_added() panicing #2829
Conversation
|
Could you also remove the unwraps inside |
if a resource doesn't exist, would you say it is never changed? or indeed should we crash the program because the user might try to access an unregistered resource? |
|
I can see that argument, but in that case |
There was a problem hiding this comment.
I'd implement the function differently, but in a semantically equivalent way. The test looks good though
I'm not sure about is_resource_changed. I'd prefer returning false when the resource doesn't exist to panicing, but I'm less sure about changing the return type.
| let component_id = match self.components.get_resource_id(TypeId::of::<T>()) { | ||
| Some(id) => id, | ||
| None => return false, | ||
| }; | ||
| let column = match self.get_populated_resource_column(component_id) { | ||
| Some(col) => col, | ||
| None => return false, | ||
| }; | ||
| // SAFE: resources table always have row 0 | ||
| let ticks = unsafe { column.get_ticks_unchecked(0) }; | ||
| ticks.is_added(self.last_change_tick(), self.read_change_tick()) |
There was a problem hiding this comment.
This could be combined with the line using Option::and_then, avoiding one match.
I would also change it to be an if let Some(...) with an else {false} at the end
I.e. my final version would be:
| let component_id = match self.components.get_resource_id(TypeId::of::<T>()) { | |
| Some(id) => id, | |
| None => return false, | |
| }; | |
| let column = match self.get_populated_resource_column(component_id) { | |
| Some(col) => col, | |
| None => return false, | |
| }; | |
| // SAFE: resources table always have row 0 | |
| let ticks = unsafe { column.get_ticks_unchecked(0) }; | |
| ticks.is_added(self.last_change_tick(), self.read_change_tick()) | |
| if let Some(column) = self.components.get_resource_id(TypeId::of::<T>()) | |
| .and_then(|component_id| self.get_populated_resource_column(component_id)) { | |
| // SAFE: resources table always have row 0 | |
| let ticks = unsafe { column.get_ticks_unchecked(0) }; | |
| ticks.is_added(self.last_change_tick(), self.read_change_tick()) | |
| } else { | |
| false | |
| } |
There was a problem hiding this comment.
it seems a bit less readable imo, the if condition stretches over 2 lines(or occupies a really long line) and generally seems a bit crowded, the match cases(while seems a bit long) gives you a clear view of what is going on at a glance.
(and generally speaking i would get that if condition to be a variable and a short if let statement instead)
alice-i-cecile
added
A-ECS
Objective
Fixes #2828
Solution
replaced unwrap with match(and added a test to make sure everything works without crashing)