FilteredResource returns a Result instead of a simple Option #18265
Conversation
crates/bevy_ecs/src/world/error.rs
Outdated
| /// The resource does not exist in the world. | ||
| #[error("The resource does not exist in the world.")] | ||
| MissingResource, | ||
| /// No access to the resource with the given [`ComponentId`] in the world. |
There was a problem hiding this comment.
These docs / error message should be clearer. What sort of access are we talking about here?
There was a problem hiding this comment.
Hmmm, how about something like "Cannot get access to resource as it conflicts with an on going operation"?
Something along this line, I am not a native speaker so feel free to suggest something that is better!
There was a problem hiding this comment.
Something like that is better, but I think the key thing to mention is that the conflicting access is with respect to the world :)
There was a problem hiding this comment.
What sort of access are we talking about here?
The main way to use FilteredResources is with a SystemParamBuilder, so the answer is usually that you needed to request it in the FilteredResourcesParamBuilder.
(The other way to get one is to convert from &World or &mut World, but those will have access to all resources and shouldn't ever return this variant.)
alice-i-cecile
added
C-Usability
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
| pub fn get<R: Resource>(&self) -> Result<Ref<'w, R>, ResourceFetchError> { | ||
| let component_id = self | ||
| .world | ||
| .components() | ||
| .resource_id::<R>() | ||
| .ok_or(ResourceFetchError::MissingResource)?; | ||
| if !self.access.has_resource_read(component_id) { | ||
| return None; | ||
| return Err(ResourceFetchError::NoResourceAccess(component_id)); | ||
| } | ||
|
|
||
| // SAFETY: We have read access to this resource | ||
| unsafe { self.world.get_resource_with_ticks(component_id) }.map(|(value, ticks, caller)| { | ||
| Ref { | ||
| // SAFETY: `component_id` was obtained from the type ID of `R`. | ||
| value: unsafe { value.deref() }, | ||
| // SAFETY: We have read access to the resource, so no mutable reference can exist. | ||
| ticks: unsafe { Ticks::from_tick_cells(ticks, self.last_run, self.this_run) }, | ||
| // SAFETY: We have read access to the resource, so no mutable reference can exist. | ||
| changed_by: unsafe { caller.map(|caller| caller.deref()) }, | ||
| } | ||
| let (value, ticks, caller) = unsafe { self.world.get_resource_with_ticks(component_id) } | ||
| .ok_or(ResourceFetchError::MissingResource)?; | ||
|
|
||
| Ok(Ref { | ||
| // SAFETY: `component_id` was obtained from the type ID of `R`. | ||
| value: unsafe { value.deref() }, | ||
| // SAFETY: We have read access to the resource, so no mutable reference can exist. | ||
| ticks: unsafe { Ticks::from_tick_cells(ticks, self.last_run, self.this_run) }, | ||
| // SAFETY: We have read access to the resource, so no mutable reference can exist. | ||
| changed_by: unsafe { caller.map(|caller| caller.deref()) }, | ||
| }) |
There was a problem hiding this comment.
very idiomatic, I like it.
alice-i-cecile
added
S-Needs-Review
|
I think this is ready for a final review to be merged |
alice-i-cecile
added
S-Ready-For-Final-Review
|
Technically needed a second approval. But I can do that for you :) |
Objective
FilteredResource::get should return a Result instead of Option
Fixes #17480
Migration Guide
Users will need to handle the different return type on FilteredResource::get, FilteredResource::get_id, FilteredResource::get_mut as it is now a Result not an Option.