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

expose current_entity() in with_children (aka ChildBuilder) #594

Closed
Gregoor opened this issue Sep 28, 2020 · 0 comments · Fixed by #595
Closed

expose current_entity() in with_children (aka ChildBuilder) #594

Gregoor opened this issue Sep 28, 2020 · 0 comments · Fixed by #595
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible

Comments

@Gregoor
Copy link
Contributor

Gregoor commented Sep 28, 2020

Big love for Bevy here (namely language choice, ECS, scope and API design ❤️ ). Thank you!

While playing around with Bevy in the context of Rapier, I struggled with getting newly created entities within the context of with_children, as the way to get entities that I knew (current_entity) is not exposed on the ChildBuilder struct. Rapier needs references to the entities when you build joints.
Thanks to the Discord I learned that it was possible to just not use with_children but rather collect what you need with current_entity and use push_children later.

Since with_children seems like a expressive way to declare entity hierarchy, I'd prefer to have access to an entity within it.

It also feels like there might be a larger point here wrt API simplification, things that are feeling a bit weird to me right now:

  • there's multiple ways to declare a hierarchy (with_children, insert_children, push_children), isn't one enough?
  • there's multiple ways to declare components for entities (spawn, with, with_bundle)
  • current_entity makes CommandsInternal feel like a stateful-iterator-esque thing and then there are methods which need to check whether the entity is even set. Might
    • somewhat related, I just noticed that there is also for_current_entity which seems to be another API to get an entity handle. I'm wondering whether these two APIs might be better dropped in favor of a variant of spawn which has this FnOnce(Entity) type as a second param
    • I'm also starting to appreciate the value of method chaining here and am wondering whether moving the with_*-fns to a new struct which is returned by spawn might be feasible, thereby alleviating those fns of the need to current_entity-check, aka making illegal state impossible

Okay sorry these spitballs turned into the lion share of the post, I'll make a PR for the less controversial first part now :)

@karroffel karroffel added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible labels Sep 28, 2020
@cart cart closed this as completed in #595 Oct 1, 2020
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-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants