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

Implement Spawn trait #14247

Closed
wants to merge 2 commits into from
Closed

Implement Spawn trait #14247

wants to merge 2 commits into from

Conversation

MiniaczQ
Copy link
Contributor

@MiniaczQ MiniaczQ commented Jul 9, 2024

Objective

Solution

  • Implement Spawn trait that provides spawn_empty and spawn API
  • It's implemented for World, Commands, WorldChildBuilder and ChildBuilder

Migration Guide

  • Spawn was added to bevy::prelude, but will have to be included manually if prelude is not used.

@MiniaczQ
Copy link
Contributor Author

MiniaczQ commented Jul 9, 2024

After going through all the code for this, I think Spawn may be too specific,
we could include entity(Entity) -> Self::SpawnOuyput as well, since it shares the return type,
but we'd need a better name than Spawn

@MiniaczQ
Copy link
Contributor Author

MiniaczQ commented Jul 9, 2024

I'll fix docs after we agree on the code, it's a pain to maintain at all time

@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 X-Contentious There are nontrivial implications that should be thought through S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 9, 2024
@benfrankel
Copy link
Contributor

benfrankel commented Jul 9, 2024

Not a full review yet but one note: Since Spawn::SpawnOutput is not given any bounds, generic contexts won't be able to do anything besides return it:

fn foo<C: Spawn>(commands: C) -> C::SpawnOutput {
    commands.spawn_empty()
        .insert(...) // Error
        .with_children(...) // Error
}

This would require another trait, so if the plan is to do that in a separate PR that makes sense.

@MiniaczQ
Copy link
Contributor Author

MiniaczQ commented Jul 9, 2024

I'm getting the easy traits out first, hopefully we can get a few in 0.14.1

The trait for EntityCommands and WorldMutEntity is pretty controversial imo, since the mutation would be either deferred or immediate and that's something to consider

@benfrankel
Copy link
Contributor

benfrankel commented Jul 9, 2024

Deferred vs immediate is already the case for Commands as Spawn vs World as Spawn, no?

type SpawnOutput<'a> = EntityWorldMut<'a> where Self: 'a;

fn spawn(&mut self, bundle: impl Bundle) -> EntityWorldMut {
let entity = self.world.spawn((bundle, Parent(self.parent))).id();
/// Spawns an entity with the given bundle and inserts it into the parent entity's [`Children`].
Copy link
Contributor

@benfrankel benfrankel Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs got swapped between spawn_empty and spawn.

@benfrankel
Copy link
Contributor

benfrankel commented Jul 9, 2024

Naming proposition:

  • Spawn -> EntityContext
  • Spawn::SpawnOutput -> EntityContext::EntityBuilder (and later this will be bound by a trait named EntityBuilder)

Or something along these lines. Issue is, EntityContext can be interpreted as either a context where you can interact with entities, OR the context of a particular entity (which would actually be EntityBuilder).

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to see a more cohesive overview of which traits would be used, and what they would be implemented for. I don't think this is wise to merge piecemeal, although I am in favor of the broad idea.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 14, 2024
@MiniaczQ
Copy link
Contributor Author

I want to see a more cohesive overview of which traits would be used, and what they would be implemented for. I don't think this is wise to merge piecemeal, although I am in favor of the broad idea.

If we can't do it piecewise then this will turn into a large scale PR that will need to be for 0.15, in that case I'm closing the issue in favor of future discussion in #14231 with this as an example

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 S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants