bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Implement `Spawn` trait

Open MiniaczQ opened this issue 1 year ago • 6 comments

Objective

  • Implement part of #14231

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 avatar Jul 09 '24 16:07 MiniaczQ

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 avatar Jul 09 '24 16:07 MiniaczQ

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

MiniaczQ avatar Jul 09 '24 16:07 MiniaczQ

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.

benfrankel avatar Jul 09 '24 19:07 benfrankel

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

MiniaczQ avatar Jul 09 '24 19:07 MiniaczQ

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

benfrankel avatar Jul 09 '24 19:07 benfrankel

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).

benfrankel avatar Jul 09 '24 22:07 benfrankel

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

MiniaczQ avatar Jul 14 '24 21:07 MiniaczQ