Add try_get_* component accessors to filtered entity references and mutators
#21588
Conversation
|
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
alice-i-cecile
added
A-ECS
| if !self.access.has_component_write(component_id) { | ||
| return None; | ||
| } | ||
| self.entity.get_mut_by_id(component_id).ok() |
There was a problem hiding this comment.
Looks like this is just a formatting change? the compiler allows calling get_mut_by_id without the unsafe block here because bevy_ecs doesn't use unsafe_op_in_unsafe_fn, but it's good style to annotate unsafe function calls even within unsafe functions, since not all of the safety bounds of the parent function may have to hold for the inner call.
| pub fn try_get<T: Component>(&self) -> Result<&'w T, ComponentFetchError> { | ||
| let components = self.entity.world().components(); | ||
| let id = components.get_valid_id(TypeId::of::<T>()).ok_or( | ||
| ComponentFetchError::ComponentNotFound(ComponentId::new(usize::MAX)), |
There was a problem hiding this comment.
We might want to differentiate between ComponentNotFound on the entity and ComponentNotRegistered
There was a problem hiding this comment.
I agree, this would be a confusing error to get.
There was a problem hiding this comment.
We might want to differentiate between ComponentNotFound on the entity and ComponentNotRegistered
Would that ever be meaningful to the caller? Inserting a component onto any entity will cause it to be registered. So if it's not registered, then we do know that it's not on this entity. And the caller isn't likely to care whether it's been inserted on some other entity before.
There was a problem hiding this comment.
Here it doesn't matter much, but I can see the distinction more in try_get_by_id() where you could by mistake an incorrect ComponentId, vs requesting a component that is not present on the entity.
But I agree that this is pretty niche, it's mostly a nit
| /// Returns an error distinguishing between insufficient access and the component | ||
| /// not being present on the entity. | ||
| #[inline] | ||
| pub fn try_get_mut<T: Component<Mutability = Mutable>>( |
There was a problem hiding this comment.
not blocking, but: an unchecked version of this method as well as try_get_mut_by_id sounds useful to me as well.
| pub fn try_get<T: Component>(&self) -> Result<&'w T, ComponentFetchError> { | ||
| let components = self.entity.world().components(); | ||
| let id = components.get_valid_id(TypeId::of::<T>()).ok_or( | ||
| ComponentFetchError::ComponentNotFound(ComponentId::new(usize::MAX)), |
There was a problem hiding this comment.
I agree, this would be a confusing error to get.
|
Why introduce new methods rather than modifying the existing methods to return |
alice-i-cecile
added
S-Waiting-on-Author
5 participants
Objective
Fixes #21403
Solution
try_get_*APIs to filtered entity refs/muts to returnResult, distinguishing missing components from insufficient access.ComponentFetchErrorwithInsufficientAccessandComponentNotFound.get/get_refthatNonecan also mean no read access under filtering.Testing
cargo test -p bevy_ecs.