[Merged by Bors] - impl SystemParam for Option<Res<T>> / Option<ResMut<T>>#1494
[Merged by Bors] - impl SystemParam for Option<Res<T>> / Option<ResMut<T>>#1494inodentry wants to merge 2 commits into
Conversation
TheRawMeatball
left a comment
There was a problem hiding this comment.
Overall, I really like this
| if resources.contains::<T>() { | ||
| Some(Some(Res::new( | ||
| resources.get_unsafe_ref::<T>(ResourceIndex::Global), | ||
| ))) | ||
| } else { | ||
| Some(None) | ||
| } |
There was a problem hiding this comment.
I'm not sure if this is cleaner or not, but it's worth considering
| if resources.contains::<T>() { | |
| Some(Some(Res::new( | |
| resources.get_unsafe_ref::<T>(ResourceIndex::Global), | |
| ))) | |
| } else { | |
| Some(None) | |
| } | |
| Some( | |
| resources | |
| .contains::<T>() | |
| .then(|| Res::new(resources.get_unsafe_ref::<T>(ResourceIndex::Global))), | |
| ) |
There was a problem hiding this comment.
I prefer the imperative style over the functional style.
So i think the original is clearer and more readable.
(and probably also easier for the compiler)
|
I think this needs an example and a mention in the |
|
Added doc comments. I can't come up with a meaningful way to add it to the |
DJMcNab
left a comment
There was a problem hiding this comment.
This is useful functionality to expse.
Also this will almost certainly need to be changed for the ecs pr when that arrives, since the SystemParam trait will be changing.
| _world: &'a World, | ||
| resources: &'a Resources, | ||
| ) -> Option<Self::Item> { | ||
| if resources.contains::<T>() { |
There was a problem hiding this comment.
This double lookup is not good - ideally we'd just have a get_unsafe_ref which returns an Option<NonNull<T>>.
(I think the most likely response from cart will be to point this out)
|
I'm in favor of these changes. Lets just update this when #1525 is merged. |
|
Updated for ECS v2. This time, thanks to the functionality being split across 2 separate traits, I could implement it more cleanly by wrapping an underlying There is also no extra check, because the resource fetch API returns an |
|
bors r+ |
This allows users to write systems that do not panic if a resource does not exist at runtime (such as if it has not been inserted yet). This is a copy-paste of the impls for `Res` and `ResMut`, with an extra check to see if the resource exists. There might be a cleaner way to do it than this check. I don't know.
|
Pull request successfully merged into main. Build succeeded: |
This allows users to write systems that do not panic if a resource does not exist at runtime (such as if it has not been inserted yet).
This is a copy-paste of the impls for
ResandResMut, with an extra check to see if the resource exists.There might be a cleaner way to do it than this check. I don't know.