Fetch multiple resources at once from World

[Adamkob12]

Objective

• fix Allow users to fetch multiple resources at once in get_resource APIs #2942
• Update the API to allow for fetching multiple resources at once.

Solution

• add methods get_resources, get_resources_mut and get_resources_mut_unchecked. Each allows for a slightly different kind of access to the world's resources.

---

Changelog

• Methods added in World: get_resources, get_resources_mut, and get_resources_mut_unchecked
• Methods added in UnsafeWorldCell: get_resources, get_resources_mut, and get_resources_mut_unchecked
• Public trait added: ResourceBundle
• Tests added: resource_bundle and access_conflict_in_resource_bundle

Additional context

Also, I did not "complete" the API yet, if these changes are acceptable, I will add more methods such as resources_scope etc.

[SpecificProtagonist]

Nice. What's the use case for fetching the same resource multiple times?

[Adamkob12]

Nice. What's the use case for fetching the same resource multiple times?

I doubt there is a reasonable one. Should we restrict against it?

[SpecificProtagonist]

I don't think we need to, I was just curious. Not providing for it would make the code shorter though.
Edit: Maybe we should. If fetching the same res twice is a mistake, we should catch it instead of adding code to allow it.

[SpecificProtagonist]

Since the check for conflicting accesses doesn't depend on the world, we'd ideally perform it at compile time (and throw compile errors). Unfortunately const_type_id isn't stable yet, but we should revisit this when that changes (even then it will be tricky due to other const limitations, but possible).

[Adamkob12]

Since the check for conflicting accesses doesn't depend on the world, we'd ideally perform it at compile time. Unfortunately const_type_id isn't stable yet, but we should revisit this when that changes (even then it will be tricky due to other const limitations, but possible).

Having TypeId evaluations at compile time would be amazing indeed. Thanks for letting me know about that, I'll keep that in mind.

[SpecificProtagonist]

The original issue suggested to change the World::get_resource & World::get_resource_mut methods. I think this approach (as implemented in this PR) is preferable, because it lets the caller specifically request both shared and exclusive access "in the same go" (similar to queries).

Not quite. In systems, requesting shared access to a component/resource allows multiple querries/system params to use it simultaneously.

But when operating directly on the world there is nothing to mediate simultaneous accesses. Getting exclusive access to a component/resource requires holding a mutable reference to the world. While its doing that, you can't access the world in any way. If you need mutable access to one resource, you need to access all resources you need in the meanwhile with a single call. So being able to specify that you don't need exclusive access to some of the requested resources doesn't give you any benefit as you'll be given exclusive access anyways, just without the ability to mutate. And since you generally want to destructure the tuple before using it, you can still put mut on only the bindings you want to mutate.

For querries, being able to specify exclusive/shared access is there because it's useful for the system param usecase.

Making the distinction between exclusive and pretend-shared resource access means we need to add 6 new variants of public methods to World, with the old variants being strictly less capable. We could still deprecate the old ones, but that means users need to express themselves tautologically: resources_mut::<&mut Foo>() instead of resources_mut::<Foo>(). Simply expanding the old functions to multiple resources avoids that.

[Adamkob12]

Thank you for your input, I implemented a lot of your suggestions.

Now that World::get_resource is a "sub-set" of World::get_resources
(you can call both World::get_resource::<Foo>() and World::get_resources::<Foo>() and get the same result, but World::get_resources is more powerful)
Should we consider deprecating World::get_resource-style methods with their World::get_resources equivalent? similar to how App::add_plugin was deprecated in favor of App::add_plugins.

[SpecificProtagonist]

👍

Should we consider deprecating World::get_resource-style methods with their World::get_resources equivalent? similar to how App::add_plugin was deprecated in favor of App::add_plugins.

I'd say so. Other examples of the same are spawn & add_systems.

So, the follow up work (that doesn't need to happen in this PR):

• Deprecate get_resource & co.
• resources/resources_ref/resources_mut
• resource_scope

[Adamkob12]

I settled on ReadOnlySmartRefAccess to keep it consistent with #11545

Also there's currently no way to get ResMut from World (getting Res was only added a week ago). That should probably be an option, I'll open an issue. edit: #11765 is the issue

[Adamkob12]

We should wait until #11776 is merged. Then replace the 'Mut's with 'ResMut's.

[Adamkob12]

update thee doc comment here

[james7132]

As with #10908 and #11545, I'll be the one to play devils' advocate here. The typical way one would fetch multiple resources right now is SystemState<(Res<A>, Res<B>, ...)>::get, as we have done in bevy_render's RenderCommands. This is a bit involved but it delivers on the use case of wanting to fetch multiple resources (or queries) from the World., and it comes built in with conflict tracking already.

The primary use case that I can see where this specific PR could be useful is needing multiple mutable resources, without the use of resource_scope, or where SystemState is infeasible to use. As for whether it's worth the extra complexity and compile time costs (adding two all_tuples pyramids), I'm not 100% sold on just yet.

[Adamkob12]

I genuinely think this is worthwhile.
But I also agree it's important to be mindful that we don't add bloat to hurt compilation time.

I also think that using SystemState like that is off putting. It's fine internally but I wouldn't want this to be the go-to for users.

Also, since we'll end up deprecating methods as well hopefully it shouldn't hurt compilation time as much in the long run.

Lastly, are "all_tuples pyramids" shown to be a bottleneck? I honestly don't have the slightest experience in that area.

Lastly (for real this time), we can also lower this pyramid to 6-8, that's likely more than enough. If that's not enough I'll merge the traits so we don't need two separate pyramids.

[Adamkob12]

Thoughts?

[alice-i-cecile]

Still a bit nervous about compile times. If we had variadics I'd be fully in favor. Actual data would help make this decision.

[alice-i-cecile]

I don't currently feel that this is worth the added API surface or internal complexity.

Closing this out as a result. If and when we get variadics, I would be thrilled to reconsider.