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

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

Open
benfrankel opened this issue Jun 17, 2024 · 7 comments · May be fixed by #233
Open

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

benfrankel opened this issue Jun 17, 2024 · 7 comments · May be fixed by #233

Comments

@benfrankel
Copy link

benfrankel commented Jun 17, 2024

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:

  • use bevy::prelude::*; will pull in bevy_state::prelude::*.
  • app.add_plugins(DefaultPlugins) will set up bevy_state::StatesPlugin.

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 benfrankel changed the title Depend on bevy_asset and bevy_state crates directly instead of via bevy Depend on bevy's subcrates directly instead of as bevy features Jun 17, 2024
@benfrankel
Copy link
Author

benfrankel commented Jun 17, 2024

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

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

benfrankel commented Jun 18, 2024

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

NiklasEi commented Jul 2, 2024

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

benfrankel commented Jul 2, 2024

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

mgi388 commented Jul 11, 2024

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 benfrankel linked a pull request Jul 13, 2024 that will close this issue
@benfrankel
Copy link
Author

benfrankel commented Jul 13, 2024

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.

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

Successfully merging a pull request may close this issue.

3 participants