Skip to content

Implement Spawn trait#14247

Closed
MiniaczQ wants to merge 2 commits intobevyengine:mainfrom
MiniaczQ:spawn-trait
Closed

Implement Spawn trait#14247
MiniaczQ wants to merge 2 commits intobevyengine:mainfrom
MiniaczQ:spawn-trait

Conversation

@MiniaczQ
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

benfrankel commented Jul 9, 2024

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


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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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