New Resources API

[JonahPlusPlus]

Objective

With 0.10, I really enjoyed merging add_system calls into add_systems. So I wanted to see if that can be done for calls like insert_resource and init_resource.

Solution

This PR deprecates <App/Commands/World>::insert_resource and <App/Commands/World>::init_resource in favor of <App/Commands/World>::insert_resources and <App/Commands/World>::init_resources.

It introduces new traits: InitResources and InsertResources (separate because of different constraints). InitResources has an associated type for whether ComponentId or [ComponentId; _] should be returned from World::init_resources.

Examples

// Before
App::new()
  .insert_resource(A::new())
  .insert_resource(B::new())
  .init_resource::<C>()
  .init_resource::<D>()
  .init_resource::<E>()
  .run();

// After
App::new()
  .insert_resources((A::new(), B::new()))
  .init_resources::<(C, D, E)>()
  .run();

---

Changelog

Added

• Added <App/Commands/World>::init_resources
• Added InitResources
• Added InitResourceCommand
• Added <App/Commands/World>::insert_resources
• Added InsertResources
• Added InsertResourcesCommand

Changed

• Changed Command implementers to have -Command suffix
• Changed invocations of <App/Commands/World>::init_resource to <App/Commands/World>::init_resources
• Changed invocations of <App/Commands/World>::insert_resource to <App/Commands/World>::insert_resources

Removed

• Removed <App/Commands/World>::init_resource
• Removed <App/Commands/World>::insert_resources

Migration Guide

• Calls to <App/Commands/World>::init_resource should be changed to <App/Commands/World>::init_resources.
• Calls to <App/Commands/World>::insert_resource should be changed to <App/Commands/World>::insert_resource

[JonahPlusPlus]

Decided against adding init_non_send_resources (for now) since the trait bounds ('static + FromWorld) can't be properly constrained due to orphan rules (if Default was added upstream to a tuple like (A, B, C), it would conflict with A's implementation). If this is desired, in order to be consistent with the rest of the API, it would require creating a new trait.

[JonahPlusPlus]

Okay, I feel this is ready now.

I eliminated the use of Vec<ComponentId> by using an associated type (I really got to stop shooting myself in the foot by over constraining my traits). This allows for World::init_resource and World::insert_resource to be deprecated safely, as InitResources and InsertResources now carry all the functionality needed.

The traits InitResources and InsertResources needed corresponding commands, so I created InitResourcesCommand and InsertResourceCommand, then renamed all the other commands to have a -Command suffix for consistency. Maybe a bit overkill, but I figured it just makes the names more explicit.

[iiYese]

I think the issue people might have with this initially is that the semantics aren't completely parallel with bundles/systems. In those APIs the elements are related/coupled in some way. For resources it is less often the case (even on a plugin basis) that multiple resources are related to eachother.

However there's nothing stopping people from just using multiple calls for unrelated resources. I'm in favor of this anyway. I think it's fine to not be opinionated on this matter and let it come down to a choice of style for each author.

[james7132]

I'm not 100% sure what this wins us other than reducing the number of lines of code we need to write (a valid win onto itself).

My primary concern here is that this increases compile time (all_tuples generates a lot of code) and also multiple ways of initializing resources, and potentially introduces subtle order of operations issues. For example:

app.insert_resource((A::new(0), A::new(1), A::new(2)));

Unlike systems where each instance isn't unique, resources are. What value does the resource A take on in this case? We can document this, but more complex nested tuples make this extra confusing. I'm not confident that the usability win is worth the potential mental burden that this puts on users. IMO, the use of SystemConfigs::chain presents a concrete usability win that is works with the semantics of the nested tuples when adding systems that is just not present here.

[JonahPlusPlus]

@james7132 Yes, the main goal of this PR is usability when inserting or initializing many resources.

However, with World::init_resources, there is also the option of getting a [ComponentId; _], which might be useful in cases where you want to initialize many resources and then iterate on them with some common function or destructure it (let [a, b, c] = world.init_resources::<(A, B, C)>();).

As for order of operations, it works in the order of the tuple. So, for your example, the final value would be A::new(2). That one call would have looked like:

app.insert_resource(A::new(0));
app.insert_resource(A::new(1));
app.insert_resource(A::new(2));

If this needs to be documented, I will gladly do so.

As for any increase in mental burden, as @iiYese pointed out, nothing is stopping users from writing it out like before.
In fact, when migrating pieces of code to this new interface in Bevy, there were times where it was better to make separate calls, for the sake of organization.
On that note, this isn't forcing users to add resources all together, rather it gives them more options for organization.

On compile times: I don't think there is an issue. Sure, it isn't lowering times, but it isn't calling macros on every invocation and with incremental compilation the compile time cost will really only be seen on fresh builds.

[iiYese]

it works in the order of the tuple

Would be nice if we could just disallow this at a type level. If not I think adding a warning for tuples that contain more than one of the same resource should be considered.

[JonahPlusPlus]

@iiYese AFAIK there's no way to currently do that. It would require specialization or negative bounds/impls.

With negative bounds, you would create and implement a trait for checking equality and then use it as a negative bound. Ex:

trait EqTrait {}

impl<T> EqTrait for (T, T) {}

impl<A, B> MyTrait for (A, B) where (A, B): !EqTrait {}

This technique could also be used with specialization, but would only result in a panic, not a compile error.

However, one problem is expanding this to larger tuples, as checks between each type would grow exponentially (Ex: (A, B, C) => (A && B) || (A && C) || (B && C)).
It's not a big deal with compile time checks, but if I were to add runtime checks, they would get pretty expensive. They could be disabled in release builds though.

One solution was to move most of the work to compile time so that the runtime cost is constant, but that can only be done on nightly:

#![feature(const_trait_impl)]
#![feature(const_type_id)]
use std::any::TypeId;

trait MyTrait {
    const EQ: bool;

    fn foo(self);
}

impl<A: 'static, B: 'static> MyTrait for (A, B) {
    const EQ: bool = TypeId::of::<A>() == TypeId::of::<B>();

    fn foo(self) {
        if Self::EQ {
            println!("Error");
        } else {
            println!("Works");
        }
    }
}

fn main() {
    (2, 3).foo();
}

Though, I'm not sure how I feel about runtime warnings. If the call is from a system, it would be repeated ad nauseam.

[cart]

Yeah I'm torn on this one.

Pros:

• Reduces line noise + typing
• Consistent with add_systems api and potentially add_plugins as we've been discussing doing this for those too

Cons:

• People tend to think about resources as "separate things". This "groups" them.
• I think we'll see a lot of app.add_resources(Foo), which reads worse / is less clear than app.add_resource(Foo).
• Potentially increases compile times

Can someone take a few samples of clean compiles with / without these changes (make sure you use the same base commit)

[JonahPlusPlus]

@cart I went ahead and did some benchmarks with this PR branch and a branch based off the base commit (2010164). I ran cargo build inside of "bevy_ecs" (it's where the macros are, and by focusing on a smaller piece of code there is less variance) and ran cargo clean before each test.

Device specs: rustc 1.70.0-nightly (900c35403 2023-03-08), Windows 10, Ryzen 5 2600 CPU, 16GB DDR4 RAM, 1TB HDD

The first 5 trials were with other programs running (Firefox, Spotify, etc.). For the last 5, I closed those programs (keeping some up for noting the times). I switched branches between each test. Afterwards, I calculated the confidence interval using a t-score.

	Time (in sec)
Base # 01	30.80
PR # 01	31.02
Base # 02	29.50
PR # 02	29.26
Base # 03	29.67
PR # 03	29.15
Base # 04	28.78
PR # 04	29.18
Base # 05	30.13
PR # 05	30.75
Base # 06	29.82
PR # 06	29.62
Base # 07	29.58
PR # 07	29.47
Base # 08	30.66
PR # 08	31.13
Base # 09	28.69
PR # 09	30.27
Base # 10	29.43
PR # 10	30.81
Base Avg	29.706
PR Avg	30.0666
Base Std Dev	0.69324
PR Std Dev	0.811928
Base Std Err	0.219222
PR Std Err	0.256754
Base 95% Confidence Interval	29.706 +/- 0.49588
PR 95% Confidence Interval	30.0666 +/- 0.580778

From all this, we can say that in the worst reality, the difference between the true means (with 95% confidence) would be 1.44sec (In the best reality, it would be -0.72sec). But based on the sample mean, we should see a 0.36sec difference. That's a 1.21% increase from before. Frankly, that's pretty significant for a relatively small change, even if it's not very noticeable. Mind you, these are on clean builds, so most of the time, users won't deal with these costs. Edit: I should also note that this is only for bevy_ecs, the effect on total build time would only be a fraction of a percent.

Despite these costs, I still think it's worth it. We already have similar interfaces, so this one should be no big deal. I think consistency and usability are more important than 0.36sec on clean builds.

Edit 2: I thought I added this, but I guess not: The t-test was inconclusive that there is a difference between these samples. The p-value was something like 0.15, which means that at <85% confidence we can say there is a difference, but it isn't strong enough for 90-95%.

[cart]

I think the cost there is pretty reasonable / stomachable (if it even exists at all). Thanks for measuring! I think this should come down to "is this the API we want". I'm still largely on the fence, with a slight bias toward merging in the interest of API consistency (pros and cons feel reasonably matched to me).

I'd like to hear final thoughts from a few more ECS people before making a call.

[cart]

@bevyengine/ecs-team

[joseph-gio]

Nice writeup on the benchmarks, that does alleviate my concerns about codegen. My only objection at this point is that this will add slightly more friction when switching between insert_resource and init_resource -- not a very strong objection though.

Overall I'd say that I'm not really opposed to this PR, but I don't think it wins us too much either.

[hymm]

On the plus side things get a little more consistent with the add_systems api. On the minus side, internal code is a little harder to understand as things are put behind a macro. Overall I'm pretty neutral on the change.

Taking a bit from my jam game

        // before
        app.insert_resource(RapierConfiguration {
            timestep_mode: TimestepMode::Fixed {
                dt: 1. / 60.,
                substeps: 1,
            },
            ..Default::default()
        })
        .add_plugin(RapierPhysicsPlugin::<NoUserData>::pixels_per_meter(100.))
        .insert_resource(FixedTime::new_from_secs(1.0 / 60.0))
        .insert_resource(PhysicsSettings {
            initial_jump_speed: 400.0,
            gravity_pressed: 40.0,
            gravity_unpressed: 200.0,
            horizontal_speed: 200.0,
            max_speed: 700.0,
        });

        // after
        app.insert_resources(RapierConfiguration {
            timestep_mode: TimestepMode::Fixed {
                dt: 1. / 60.,
                substeps: 1,
            },
            ..Default::default()
        })
        .add_plugin(RapierPhysicsPlugin::<NoUserData>::pixels_per_meter(100.))
        .insert_resources((
            FixedTime::new_from_secs(1.0 / 60.0),
            PhysicsSettings {
                initial_jump_speed: 400.0,
                gravity_pressed: 40.0,
                gravity_unpressed: 200.0,
                horizontal_speed: 200.0,
                max_speed: 700.0,
            }
        ));

Looks slightly cleaner.

Note: There is also #8097 which does something similar for add_plugins

[cart]

Alrighty I think we should move forward with this. Marginal legibility/ergo increase in some cases and consistency with the direction other apis are going in. This would be a hard thing to justify in a "stable bevy" world, so its now or never.

[alice-i-cecile]

@JonahPlusPlus can you please rebase or otherwise fix the merge conflicts then?

[JonahPlusPlus]

@alice-i-cecile Sure, I'll get on it.

[JonahPlusPlus]

That should do it, but let me know if there are more merge conflicts. By the time I finished fixing these merge conflicts, there was already another one.

[alice-i-cecile]

I'm neutral on the syntax change but this appears correct now.

[alice-i-cecile]

@cart can you do a quick skim and merge this? I'd like to avoid leaving this longer because of the merge conflicts it generates.

[mockersf]

Not a fan of grouping resources together... in my opinion it's not the same as systems where you often want to be able to schedule several systems at the same time, resources each have their own meaning

[cart]

Sorry for the back and forth @JonahPlusPlus, but thats now two maintainers (@mockersf and @james7132 ... who is also notably an ECS SME) expressing concerns / no votes. Given the overall "neutral sentiment" and my only "slightly in favor" vote, that is enough for me to think we should hold off.

Closing for now, but feel free to keep the conversation going. If sentiment changes to something closer to "generally in favor" we can revisit.

[inodentry]

I'm not a maintainer, I'm not sure how much my opinion matters, but I'm in favor. I feel like API consistency is good to strive for.

Bevy has been steadily moving in the direction of "add multiple things with a single call, with tuple syntax" for a while now, and I love it personally.

First, we had the bundles/components unification, which let us combine many bundles and components into a single spawn or insert call.

Then, we moved to add_systems as the syntax for adding and configuring any number of systems at once.

Sounds like we are also going to have add_plugins.

We should have it for resources too. If we don't, it's going to feel like an outlier, given that all the rest of bevy follows this pattern. I like this pattern/syntax and I think we should be consistent everywhere, even if the pros/cons for resources are less clear than with systems or bundles/components.

@james7132 As for what happens if the user specifies the same resource type multiple times, we should look at how we handle bundles/components, where the same issue occurs. What happens when you put the same component/bundle type multiple times in a tuple in a single spawn call? If our insert_resources behavior is consistent with that, I don't see a problem.

[alice-i-cecile]

Looking at the compile time increases, the messiness of init_resources and the added complexity in our code, I've shifted from "neutral" to opposed.

[james7132]

@james7132 As for what happens if the user specifies the same resource type multiple times, we should look at how we handle bundles/components, where the same issue occurs. What happens when you put the same component/bundle type multiple times in a tuple in a single spawn call? If our insert_resources behavior is consistent with that, I don't see a problem.

This would indeed address the ambiguity issues with duplicated resources being inserted. For bundle insertion/removal, deduplication currently has the global allocator and panics included (increasing codegen even more), but is only done at bundle registration time. We could duplicate this infrastructure and track every possible InitResources/InsertResources implementation the same way we track Bundle implementors to avoid the deduplication cost, or we can eat the allocation/performance cost on every insert/initialization. Whether or not we should do that is dependent on whether we think the usability wins outweighs the associated costs here.

[B-Reif]

We don't currently add any behavior to, or implement any traits on, tuples-of-resources. For that reason, I don't think this syntax is appropriate for resources (or plugins for that matter). I'm in favor of leaving this API as it is.

Bevy has been steadily moving in the direction of "add multiple things with a single call, with tuple syntax" for a while now.

This syntax enables us to share some properties across the members of the given tuple, and to reason about the members within as a single thing. For systems, this enables useful patterns around scheduling and run conditions. Resources (and plugins) don't have these added 'decorators' and don't benefit from coupling the way that systems do.

Even after the schedule-first API, users won't configure the entire app with a single call. Systems will still be added with multiple calls on the app builder. The 'multiple things' in a single call are grouped together for specific, observable reasons.

I feel like API consistency is good to strive for.

The existing API is consistent: add behavior to the app by invoking methods of the app builder. These methods have different signatures because they each add different constructs.

In the case of plugins, grouping might even mislead users into believing that the member plugins are ordered or coupled somehow. The API would be less consistent because the plugin tuple wouldn't add or specify any behavior the way that system tuples do. In the case of resources, a new user might wonder what the tuple grouping does.

The only reason I would be in favor: this change could leave us the space to implement traits on resource tuples in the future. I don't know what the purpose of that would be. Even after a post-Bevy-1.0 world, though, it wouldn't be the end of the world to extend the API with pluralized methods.

[JonahPlusPlus]

How about I create a prototype crate that adds this functionality for now? That way users can opt into this right now and see whether or not it should be an official part of Bevy.

[JonahPlusPlus]

Here, published a crate. There are some limitations (can't implement the trait for the base case due to orphan rules), but it otherwise works. Give it a try and see how it works.