bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Unified API for adding things to `App`

Open benfrankel opened this issue 1 year ago • 6 comments

What problem does this solve or what need does it fill?

There is a mild proliferation of "add XYZ to the App" methods.

  • By type:
    • init_state
    • init_resource
    • add_event
  • By value:
    • insert_state
    • insert_resource
    • add_plugins
    • add_systems
    • configure_sets (for "adding" system sets)

When you include 3rd-party crates, this list gets much longer. Furthermore, some of these methods are attached to App itself, making the 3rd-party extension traits second-class.

What solution would you like?

Proposed API:

app.add::<(MyResource, MyEvent, MyState, MySystemSet)>();
app.insert((MyResource { ... }, MyEvent::Foo, MyState::Bar, MySystemSet::Quux));

Hand-wavy implementation:

// Impl this for all tuples.
trait AddToApp {
    fn add_to_app(app: &mut App);
}

// Replaces `trait Plugin` as well.
// Impl this for all tuples.
trait InsertToApp {
    fn insert_to_app(self, app: &mut App);
}

impl App {
    fn add<T: AddToApp>(&mut self) -> &mut Self {
        T::add_to_app(self);
        self
    }

    fn insert<T: InsertToApp>(&mut self, val: T) -> &mut Self {
        val.insert_to_app(self);
        self
    }
}

Example usage:

fn main() {
    App::new().insert(DefaultPlugins).add::<MySystemSet>().run();
}

#[derive(SystemSet)]
enum MySystemSet { A, B, C }

impl AddToApp for MySystemSet {
    fn add_to_app(app: &mut App) {
        app.configure_sets(Update, (Self::A, Self::B, Self::C).chain());
    }
}

What alternative(s) have you considered?

The status quo, which definitely isn't bad, but maybe it could be improved.

benfrankel avatar Jul 10 '24 01:07 benfrankel

To clarify, the proposed API is in addition to the current methods, correct? Otherwise it would be impossible to add anything to App (since their AddToApp implementation needs to call something).

MrGVSV avatar Jul 10 '24 01:07 MrGVSV

The AddToApp implementation can use app.world_mut() (especially the ones generated by derive macros).

Systems / system sets may be a unique case (they need to be added to a particular schedule), but I threw them into the PR description anyways because there may be a way to design around it. So for .add_systems and .configure_sets, as of now, this proposal would leave them be, and .add / .insert would be in addition and require a manual impl.

benfrankel avatar Jul 10 '24 01:07 benfrankel

Today if you really mess up, you get error messages specialized to the type you're trying to add/insert. For example, the following code. How would the new super-generic API affect error messages like this? (it seems like you'd get the same error message about AddToApp for everything)

use bevy::prelude::*;

fn main() {
    App::new()
        .init_state::<SomeEvent>()
        .add_plugins(DefaultPlugins)
        .run();
}

struct SomeEvent;
error[E0277]: the trait bound `SomeEvent: FreelyMutableState` is not satisfied
  --> src/main.rs:5:23
   |
5  | ...  .init_state::<SomeEvent...
   |       ----------   ^^^^^^^^^ the trait `FreelyMutableState` is not implemented for `SomeEvent`
   |       |
   |       required by a bound introduced by this call
   |
   = note: consider annotating `SomeEvent` with `#[derive(States)]`

you can also derive multiple insertable and addable types for a given type (a silly example, you could derive Resource, States, and Event, and implement Plugin all for the same type. Its kind of silly to do this all at once, but I have definitely seen people promoting deriving combinations like Resource and Component at the same time. How would the proposed API know which type of value to insert as?

#[derive(
    Default,
    Resource,
    States,
    Debug,
    Hash,
    Eq,
    PartialEq,
    Clone,
    Event,
)]
enum SomeEvent {
    #[default]
    StateA,
}

impl Plugin for SomeEvent {
    fn build(&self, app: &mut App) {
        app;
    }
}

ChristopherBiscardi avatar Jul 10 '24 01:07 ChristopherBiscardi

you can also derive multiple insertable and addable types for a given type

That's a good point. This proposal is not compatible with the same type being used to represent different global "things" in the ECS world.

Everything should be compatible with Component at least, since there's no such thing as "adding" or "inserting" a component onto an App, unless you qualify that the component is specifically a Resource or an Event or a State.

Deriving two "global" traits like Resource and Event for the same type seems like an anti-pattern to me. Disallowing it would be a new restriction, but that might be reasonable.

benfrankel avatar Jul 10 '24 01:07 benfrankel

How would the new super-generic API affect error messages like this?

Yeah the error message would become less useful. It could ask you if you meant to derive e.g. "Resource, Event, State, or something else that can be added to the app", but that would definitely be less helpful.

benfrankel avatar Jul 10 '24 01:07 benfrankel

This may be better off reduced to an "in addition" API proposal with no auto-derived impls for AddToApp or InsertToApp. Effectively adding two new Plugin-like traits with different receivers (no receiver, by reference, or by move).

benfrankel avatar Jul 10 '24 01:07 benfrankel

Closing in favor of a smaller proposal: https://github.com/bevyengine/bevy/issues/14261.

benfrankel avatar Jul 10 '24 08:07 benfrankel