-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Merged by Bors] - Make Commands and World apis consistent #1703
Conversation
This is a pretty big change to user facing apis, so please weigh in if you have alternative ideas. I'd like to avoid touching these any more than we need to. |
4da8266
to
7b64840
Compare
User-facing changes to
|
Now, thoughts. I love these changes. The API is much clearer, more consistent and more explicit, without making anything significantly harder to do. The previous API often worked in... difficult to grasp and challenging to intuit ways as soon as you stepped beyond copying directly from the examples. The new This API also seems like an excellent step towards batching commands due to the increased explicitness and clarity. IMO these are the right changes to make, and I'd much rather break things all at once and earlier than do it later. The consistency with |
examples/2d/contributors.rs
Outdated
}) | ||
.with_bundle(SpriteBundle { | ||
let e = commands | ||
.spawn_bundle(( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much nicer to read if you use .spawn().insert_bundle().insert_bundle()
.
Is it worth solving #792 quickly before 0.5 to make this API really ergonomic? It fits in well with these changes and would be useful to folks trying to refactor their related code. |
the new api looks nice 👍 |
i like the new API! thanks a lot for that. however:
i do agree with the confusion. i feel that they should contain a verb (eg: |
I'd prefer not to. Adding more features than we already are feels like to much risk at this point. Bundle merging also feels like a pretty superficial change from a user perspective (and non breaking). Folks are free to explore this and we can make a call when the code prior to release if the in front of us, but I think it would be a stretch.
I do think the naming of We could consider alternative names like |
I'm alright with this; we can explore it in another PR, probably after 0.5's release.
Your explanation was helpful, and I think that brevity is important to consider here. I think that good docstrings and examples should alleviate the confusion well enough here to stick with the shorter names. |
Wow what a dumpster fire of a sentence. I meant to say: "Folks are free to explore this and we can make a call prior to release if the code is in front of us".
Cool cool. I'll add the docs. |
c036000
to
44cd3fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've read over the new docs and I think this is ready to merge. I'd prefer to do it sooner than later to reduce merge conflict headaches and give plugin authors a headstart on updating.
commands.entity(child).despawn();
commands.despawn_recursive(entity); It seems like this inconsistency may have been overlooked, or maybe the hierarchy/state/alien cake examples didn't get updated. |
Yup it was overlooked. Fixed! |
bors r+ |
Resolves #1253 #1562 This makes the Commands apis consistent with World apis. This moves to a "type state" pattern (like World) where the "current entity" is stored in an `EntityCommands` builder. In general this tends to cuts down on indentation and line count. It comes at the cost of needing to type `commands` more and adding more semicolons to terminate expressions. I also added `spawn_bundle` to Commands because this is a common enough operation that I think its worth providing a shorthand.
Pull request successfully merged into main. Build succeeded: |
This reverts commit 81b53d1.
Little late to the party here, but I think |
)"" This reverts commit 8860534.
The only API to add a parent/child relationship between existing entities is through commands, there is no easy way to do it from `World`. Manually inserting the components is not completely possible since `PreviousParent` has no public constructor. This PR adds two methods to set entities as children of an `EntityMut`: `insert_children` and `push_children`. ~~The API is similar to the one on `Commands`, except that the parent is the `EntityMut`.~~ The API is the same as in #1703. However, the `Parent` and `Children` components are defined in `bevy_transform` which depends on `bevy_ecs`, while `EntityMut` is defined in `bevy_ecs`, so the methods are added to the `BuildWorldChildren` trait instead. If #1545 is merged this should be fixed too. I'm aware cart was experimenting with entity hierarchies, but unless it's a coming soon this PR would be useful to have meanwhile. Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Resolves #1253 #1562
This makes the Commands apis consistent with World apis. This moves to a "type state" pattern (like World) where the "current entity" is stored in an
EntityCommands
builder.In general this tends to cuts down on indentation and line count. It comes at the cost of needing to type
commands
more and adding more semicolons to terminate expressions.I also added
spawn_bundle
to Commands because this is a common enough operation that I think its worth providing a shorthand.