Skip to content
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

FilteredResource returns a Result instead of a simple Option #18265

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions crates/bevy_ecs/src/reflect/resource.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ use crate::{
change_detection::Mut,
component::ComponentId,
resource::Resource,
world::{unsafe_world_cell::UnsafeWorldCell, FilteredResources, FilteredResourcesMut, World},
world::{
error::ResourceFetchError, unsafe_world_cell::UnsafeWorldCell, FilteredResources,
FilteredResourcesMut, World,
},
};
use bevy_reflect::{FromReflect, FromType, PartialReflect, Reflect, TypePath, TypeRegistry};

Expand Down Expand Up @@ -52,7 +55,8 @@ pub struct ReflectResourceFns {
/// Function pointer implementing [`ReflectResource::remove()`].
pub remove: fn(&mut World),
/// Function pointer implementing [`ReflectResource::reflect()`].
pub reflect: for<'w> fn(FilteredResources<'w, '_>) -> Option<&'w dyn Reflect>,
pub reflect:
for<'w> fn(FilteredResources<'w, '_>) -> Result<&'w dyn Reflect, ResourceFetchError>,
/// Function pointer implementing [`ReflectResource::reflect_mut()`].
pub reflect_mut: for<'w> fn(FilteredResourcesMut<'w, '_>) -> Option<Mut<'w, dyn Reflect>>,
/// Function pointer implementing [`ReflectResource::reflect_unchecked_mut()`].
Expand Down Expand Up @@ -118,7 +122,7 @@ impl ReflectResource {
pub fn reflect<'w, 's>(
&self,
resources: impl Into<FilteredResources<'w, 's>>,
) -> Option<&'w dyn Reflect> {
) -> Result<&'w dyn Reflect, ResourceFetchError> {
(self.0.reflect)(resources.into())
}

Expand Down
11 changes: 11 additions & 0 deletions crates/bevy_ecs/src/world/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,14 @@ pub enum EntityMutableFetchError {
#[error("The entity with ID {0} was requested mutably more than once")]
AliasedMutability(Entity),
}

/// An error that occurs when getting a resource of a given type in a world.
#[derive(thiserror::Error, Debug, Clone, Copy, PartialEq, Eq)]
pub enum ResourceFetchError {
/// The resource does not exist in the world.
#[error("The resource does not exist in the world.")]
MissingResource,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You included ComponentId in the NoResourceAccess variant, but not in MissingResource. Would it be useful to include it there, as well? I guess it's not available in the case where the resource isn't even initialized, but you could do Option<ComponentId>, or even separate variants for "not initialized" and "not present".

/// No access to the resource with the given [`ComponentId`] in the world.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These docs / error message should be clearer. What sort of access are we talking about here?

Copy link
Contributor Author

@andristarr andristarr Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like that is better, but I think the key thing to mention is that the conflicting access is with respect to the world :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

#[error("No access to the resource with ID {0:?} in the world.")]
NoResourceAccess(ComponentId),
}
34 changes: 21 additions & 13 deletions crates/bevy_ecs/src/world/filtered_resource.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ use crate::{
};
use bevy_ptr::{Ptr, UnsafeCellDeref};

use super::error::ResourceFetchError;

/// Provides read-only access to a set of [`Resource`]s defined by the contained [`Access`].
///
/// Use [`FilteredResourcesMut`] if you need mutable access to some resources.
Expand Down Expand Up @@ -151,21 +153,27 @@ impl<'w, 's> FilteredResources<'w, 's> {
}

/// Gets a reference to the resource of the given type if it exists and the `FilteredResources` has access to it.
pub fn get<R: Resource>(&self) -> Option<Ref<'w, R>> {
let component_id = self.world.components().resource_id::<R>()?;
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()) },
})
Comment on lines +156 to 177
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very idiomatic, I like it.

}

Expand Down Expand Up @@ -419,7 +427,7 @@ impl<'w, 's> FilteredResourcesMut<'w, 's> {
}

/// Gets a reference to the resource of the given type if it exists and the `FilteredResources` has access to it.
pub fn get<R: Resource>(&self) -> Option<Ref<'_, R>> {
pub fn get<R: Resource>(&self) -> Result<Ref<'_, R>, ResourceFetchError> {
self.as_readonly().get()
}

Expand Down
3 changes: 2 additions & 1 deletion crates/bevy_scene/src/dynamic_scene_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,8 @@ impl<'w> DynamicSceneBuilder<'w> {

let resource = type_registration
.data::<ReflectResource>()?
.reflect(self.original_world)?;
.reflect(self.original_world)
.ok()?;

let resource = type_registration
.data::<ReflectFromReflect>()
Expand Down
Loading