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

Use traits to make ECS APIs for working with entities more consistent #14231

Open
MiniaczQ opened this issue Jul 9, 2024 · 10 comments
Open

Use traits to make ECS APIs for working with entities more consistent #14231

MiniaczQ opened this issue Jul 9, 2024 · 10 comments
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Contentious There are nontrivial implications that should be thought through

Comments

@MiniaczQ
Copy link
Contributor

MiniaczQ commented Jul 9, 2024

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

Parts of ECS API are missing methods, which have no reason to not exist.

Types that this is most visible on are:

  1. Commands
  2. ChildBuilder
  3. EntityCommands
  4. World
  5. WorldChildBuilder
  6. EntityWorldMut

For example, you cannot use trigger from ChildBuilder, but if you spawn an entity and get EntityCommands you can call EntityCommands::commands() and then Commands::trigger_targets().

fn spawn_trigger<E: Event>(&mut self, event: E) -> EntityCommands {
    // Type juggling to get around lifetime downcasting.
    // We cannot recover `entity_commands` if we ever drop it.
    let mut entity_commands = self.spawn_empty();
    let entity = entity_commands.id();
    let mut commands = entity_commands.commands();
    commands.trigger_targets(event, entity);
    entity_commands
}

Proposed solution

This issue proposed the following traits:
a) Spawn with spawn_empty() and spawn() for [1, 2, 4, 5],
b) GetCommands with commands() for [1, 2, 3],
c) GetEntity with entity() for [1, 2, 4, 5],
d) Trigger trait trigger() and trigger_targeted() for [1, 2, 4, 5],
e) TriggerEntity with trigger() for [3, 6],
f) Observe with observe() for [1, 3, 4, 6],
g) ModifyEntity with insert() and remove() for [3, 6],
h) WithChildren with with_children() for [3, 6].

In general pairs of [1, 4], [2, 5], [3, 6] should share traits.

Now, this is just a proposal.
I want this issue to help decide the actual scope.
Ideally, key traits would make it to 0.14 without introducing breaking changes.

Example implementation for Spawn

What alternative(s) have you considered?

The traits can theoretically be a 3rd part crate, but missing methods need to be added in bevy_ecs anyways.

Additional context

This is mostly frustrations collected during development of Bevy Jam Template
Relevant discord discussion can be found here

@MiniaczQ MiniaczQ added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jul 9, 2024
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Contentious There are nontrivial implications that should be thought through and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jul 9, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jul 9, 2024

This takes the bevy_color approach to API design. This duplication also extends to EntityRef, EntityMut, and DeferredWorld. We've struggled to keep these in sync before. The downside of course is that core functionality gets moved off of the struct itself, making it a bit less visible in the docs.

@MiniaczQ
Copy link
Contributor Author

MiniaczQ commented Jul 9, 2024

We should consider allowing methods, e.g. spawn to exist both as a struct impl and as a trait impl.
The traits will still ensure that all methods are covered and multiple types share functionality,
while if the type is known the struct impl will be selected and fully documented for that scenario.

@alice-i-cecile alice-i-cecile changed the title Fill holes in ECS APIs Use traits to make ECS APIs for working with entities more consistent Jul 9, 2024
@alice-i-cecile
Copy link
Member

I'm not worried about migration, so I'd prefer to avoid duplicating the APIs.

@benfrankel
Copy link
Contributor

benfrankel commented Jul 9, 2024

As part of this, I would also propose considering Spawn::spawn_with as a generalization of Spawn::spawn:

  • spawn(bundle) = spawn_empty + add(insert(bundle))
  • spawn_with(command) = spawn_empty + add(command)

@ItsDoot
Copy link
Contributor

ItsDoot commented Jul 27, 2024

Old related issue: #5647 (for .spawn() APIs only)

@benfrankel
Copy link
Contributor

benfrankel commented Sep 3, 2024

I think this could use a working group once there's enough bandwidth, since it'll require design work and has cross-cutting impact.

@alice-i-cecile
Copy link
Member

Agreed with that. Lots of changes that need to be made in concert, and high levels of controversy.

@MiniaczQ
Copy link
Contributor Author

Another family of traits is GetCommands, GetWorld, etc.
Commands and World are usually the core of implementing additional functionality, if access to them is unified across word, subapp, app, etc. it's going to reduce extension trait boilerplate and remove bevy_app requirements

@MiniaczQ
Copy link
Contributor Author

@alice-i-cecile I think now might be a good time to do this since a lot of work is focused outside of bevy repo, on the editor.
If so, what do you think the next steps should be and who should we bother?
I think we need to decide which traits and what methods ahead of implementation, since the merges and postponing it will be painful.

@alice-i-cecile
Copy link
Member

I'd really like to land the BSN work before tackling this, because of the large impact it will have on how users initialize entities.

Fully agree on the design-first approach here; that'll need sign-off from SME-ECS and Cart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

No branches or pull requests

4 participants