NiklasEi / bevy_asset_loader

Bevy plugin helping with asset loading and organization
Apache License 2.0
505 stars 56 forks source link

Consider renaming `init_resource` to `then_init_resource` #244

Open mgi388 opened 3 days ago

mgi388 commented 3 days ago

Before:

app.add_loading_state(
    LoadingState::new(MyState::Preloading)
        .continue_to_state(MyState::Preloaded)
        .load_collection::<MyAssetCollection>()
        .load_collection::<MyAssetCollection2>()
        .init_resource::<MyResource>(),
);

After:

app.add_loading_state(
    LoadingState::new(MyState::Preloading)
        .continue_to_state(MyState::Preloaded)
        .load_collection::<MyAssetCollection>()
        .load_collection::<MyAssetCollection2>()
        .then_init_resource::<MyResource>(),
);

To me this reads clearer in that the resource is going to be initialised once all asset collections are loaded.

It could be even better if it was a bug to use a then_init_resource in the wrong position of the builder. For example, this wouldn't compile:

app.add_loading_state(
    LoadingState::new(MyState::Preloading)
        .continue_to_state(MyState::Preloaded)
        .then_init_resource::<MyResource>() // ERROR
        .load_collection::<MyAssetCollection>()
        .load_collection::<MyAssetCollection2>(),
);

Not sure what builder term you'd need to throw in there, maybe something like collect (shrug, don't love the term):

app.add_loading_state(
    LoadingState::new(MyState::Preloading)
        .continue_to_state(MyState::Preloaded)
        .load_collection::<MyAssetCollection>()
        .load_collection::<MyAssetCollection2>()
        .collect()
        .then_init_resource::<MyResource>(),
);

You might have some better ideas for a more fluent API here. E.g. maybe the argument to add_loading_state could remain as it is, but you can call app.add_loading_state().when_ready_init_resource::<MyResource>() instead.

P.S. Not sure if then_init_resource or and_then_init_resource would be better / more idiomatic.

If you prefer not to have a change like, feel free to close the issue, it's just some feedback from using the API here and it feeling a bit "messy", if I can say that.

NiklasEi commented 13 hours ago

I would like to find a name that does not depend on the call position. You can configure different parts of a loading state in multiple places, so the methods should read well everywhere. But I agree that it would be good to rename the method to distinguish it from Bevy's init_resource.

What do you think about finally_init_resource?

mgi388 commented 11 hours ago

Yeah I like that better than what we have today because it makes it really clear the "init_resource" is something that happens afterwards, not at the time you call the function in the builder.

app.add_loading_state(
    LoadingState::new(MyState::Preloading)
        .continue_to_state(MyState::Preloaded)
        .load_collection::<MyAssetCollection>()
        .load_collection::<MyAssetCollection2>()
        .finally_init_resource::<MyResource>(),
);

I'd still say to keep an eye out for any design that emerges wrt some finally_init_resource / then_init_resource being something that has to be called last. It doesn't make sense to me that you could tell the loading state what resource to init before you've told it about all of the dependent asset collections it needs to actually be able to init that resource, so it feels like there's possibly some design in here that enforces that, but still gives the flexibility for users to construct their loading state in pieces. Don't let any improvement block on this though, but if you do see some design emerge that could work, keep it in mind! For now, finally_init_resource feels better.