Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider renaming init_resource to then_init_resource #244

Open
mgi388 opened this issue Nov 24, 2024 · 2 comments
Open

Consider renaming init_resource to then_init_resource #244

mgi388 opened this issue Nov 24, 2024 · 2 comments

Comments

@mgi388
Copy link
Contributor

mgi388 commented Nov 24, 2024

  • I am often confused when scanning my own code when I see init_resource because it's the same name as Bevy's app.init_resource, but it's slightly different.
  • Ultimately it does end up initializing a resource, but I don't like that it's mixed in with the other function calls of my loading state when it's actually the last thing that is constructed after my loading state is ready.
  • I note the docs for bevy_asset_loader's says:

The resource will be initialized at the end of the loading state using its FromWorld implementation

  • Therefore I can see the then_ prefix flowing a bit better when reading a loading state.

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
Copy link
Owner

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
Copy link
Contributor Author

mgi388 commented Nov 27, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants