NiklasEi / bevy_asset_loader

Bevy plugin helping with asset loading and organization
Apache License 2.0
481 stars 52 forks source link

Depend on bevy's subcrates directly instead of as `bevy` features #219

Open benfrankel opened 3 months ago

benfrankel commented 3 months ago

If I have the following in my game's Cargo.toml:

bevy = { version = "..", default-features = false, features = [
    # A few features enabled, but no `bevy_state`, for example
    # ..
] }
bevy_asset_loader = ".."

Then bevy_asset_loader will inject the bevy_state feature into my bevy dependency. As a result:

This is unexpected; it makes bevy_asset_loader incompatible with 3rd-party alternatives to bevy_state; and it increases the compile time of bevy_asset_loader on its own.

bevy_asset_loader only needs access to bevy_state, not the full bevy/bevy_state feature which has greater implications. Depending on subcrates directly would resolve these issues.

benfrankel commented 3 months ago

Well.. I still think this is undesirable behavior but that seems to be controversial on the bevy discord. I'm willing to write a PR for this if wanted, but otherwise feel free to close.

NiklasEi commented 3 months ago

This is unexpected and also undesirable if, for example, my game uses a 3rd-party alternative to bevy_state that conflicts with bevy_state::prelude::* and that already adds bevy_state::StatesPlugin itself for compatibility.

In this case my expectation would be that said 3rd-party states alternative only adds the states plugin if it isn't added yet. Bevy has an API for that (see e.g. how bevy_asset_loader adds an internal plugin).

The only scenario in which I would find it surprising that bevy_asset_loader adds the states plugin is if it is used without a loading state. It would be a separate issue though to support that in my opinion.

benfrankel commented 3 months ago

There's also the prelude issue. For example, the bevy_state prelude brings an extension trait into scope that adds app.init_state etc., so if the bevy_state alternative does the same for its own version of app.init_state, then a user has to either avoid using bevy::prelude::* (unreasonable) or write something awkward like this:

<App as OtherAppExt>::init_state::<S>(&mut app)

To prevent this, the 3rd-party states plugin would have to use a different name (like init_state_) any time there's a conflict, making the API worse and migration awkward.

So you have to make a choice between a worse API but compatible with crates that inject the bevy_state feature into bevy, or a reasonable API but incompatible with those crates. Imo the latter is the sane choice (e.g. bevy_kira_audio has chosen the latter as well), and the occasional ecosystem incompatibility is an unfortunate side-effect.

However, there would be no incompatibility if all 3rd-party crates depended on subcrates directly. The crux of the issue is that bevy_asset_loader doesn't need the bevy_state feature enabled on bevy in order to operate (which has greater implications than just making the bevy_state crate accessible), it only needs access to bevy_state.

NiklasEi commented 3 months ago

The preview issue is a good point. I chose the easy way out with bevy_kira_audio, because I didn't expect that people would want to use two different audio backends in one app. For state solutions, this might be the case more often.

The crux of the issue is that bevy_asset_loader doesn't need the bevy_state feature enabled on bevy in order to operate (which has greater implications than just making the bevy_state crate accessible), it only needs access to bevy_state.

I think I see your point. The default in Bevy is to add the DefaultPlugins and then expect external plugins to work. If the states feature is not enabled, bevy_asset_loader will not work as expected after adding the DefaultPlugins since no one is driving the loading state.

We could of course have bevy_asset_loader add the states plugin internally, but as far as I know this is not standard in the ecosystem.

Could you link to the discussion on Discord that you mentioned in your first comment?

benfrankel commented 3 months ago

We could of course have bevy_asset_loader add the states plugin internally, but as far as I know this is not standard in the ecosystem.

bevy_asset_loader could check if the states plugin is enabled, and warn / panic if it isn't, maybe.

Could you link to the discussion on Discord that you mentioned in your first comment?

Sure, conversation started here: https://discord.com/channels/691052431525675048/692572690833473578/1252386279996395581.

mgi388 commented 2 months ago

I added bevy_asset_loader to my non-game crate. My non-game crate depends on subcrates, e.g. like this:

bevy_app = "0.14"
bevy_asset = ...

But bevy_asset_loader wouldn't compile^ unless I also added bevy to my crate, like this:

bevy = "0.14"
bevy_app = "0.14"
bevy_asset = ...

Which isn't ideal nor what I was hoping, because I was hoping to have my non-game crate to just depend on the subcrates it needs, not bevy.

If I can make a minimal repro for this case, would that be a strong signal that bevy_asset_loader should instead depend on subcrates rather than bevy?

^ If I disable the bevy = "0.14" line from my non-game crate's Cargo.toml, the error is:

failed to resolve: could not find `bevy` in the list of imported crates
could not find `bevy` in the list of imported crates
image
benfrankel commented 2 months ago

I've fixed this in a fork so that I can use bevy_asset_loader + pyri_state in the upcoming game jam.

[patch.crates-io]
bevy_asset_loader = { git = "https://github.com/benfrankel/bevy_asset_loader.git", branch = "direct-depend" }

Created a PR because it takes no additional effort to do so.