-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 Bundle for every component and simplify API #2974
Comments
I don't think we should move forward on this until we resolve the "how do Bundles relate to scenes and editor experiences" conversation (ex: #3833 (comment) and the various discussions we've had on discord). Bundles might be promoted to "metadata that exists on entities" and might expand to include "prefab-like" contexts. I'm also personally against this because it further blurs the lines between components and bundles, which our users constantly mess up in their mental models and in their code. Pulling in @bevyengine/ecs-team as this is pretty fundamental stuff. |
I agree with this: but I actually think that this change will help. Fundamentally, this user confusion is driven by the fact that bundles are treated as a distinct concept by our APIs: particularly spawn_bundle and insert_bundle. I think we should:
The type signature of fn insert(&mut EntityCommands, components: impl ComponentBundle) -> &mut EntityCommands This removes the concept of "bundles" from our API calls, and reinforces that
IMO if this exists, it should not use |
#3877 is a solid vision of what this would look like in practice. The needs appear to be quite divergent, and I suspect they will only diverge further as we explore more. |
My thinking around this is that behaviour only makes sense in relation to a combinations of components on the entity. With this view, unifying the apis which deal with |
All agreeing with the above from @alice-i-cecile and @DJMcNab, but I thought this feature was further along / already implemented, so let me present another use case that is blocked until this is implemented: fluent-style component construction. In
For Bevy, this could be implemented with a trait: pub trait TransformAnimation {
fn move_to(self, dst: Vec3) -> (Transform, Animator);
}
impl TransformAnimation for Transform {
pub fn move_to(self, dst: Vec3) -> (Transform, Animator) {
// create the Animator
// [...]
(self, animator)
}
}
commands.spawn(Transform::default().move_to(Vec3::new(1., 2., 3.)); However this is blocked by the fact the API for adding a single component is different from the one for adding 2+ components, which means users would have to change all call sites where they want to animate a component on insertion. |
This comment was marked as resolved.
This comment was marked as resolved.
Which impl do you think will cause |
This comment was marked as outdated.
This comment was marked as outdated.
Oh then I misunderstood this proposal! Thanks for making this clear to me. |
bevyengine#2975) @BoxyUwU this is your fault. Also cart didn't arrive in time to tell us not to do this. # Objective - Fix bevyengine#2974 ## Solution - The first commit just does the actual change - Follow up commits do steps to prove that this method works to unify as required, but this does not remove `insert_bundle`. ## Changelog ### Changed Nested bundles now collapse automatically, and every `Component` now implements `Bundle`. This means that you can combine bundles and components arbitrarily, for example: ```rust // before: .insert(A).insert_bundle(MyBBundle{..}) // after: .insert_bundle((A, MyBBundle {..})) ``` Note that there will be a follow up PR that removes the current `insert` impl and renames `insert_bundle` to `insert`. ### Removed The `bundle` attribute in `derive(Bundle)`. ## Migration guide In `derive(Bundle)`, the `bundle` attribute has been removed. Nested bundles are not collapsed automatically. You should remove `#[bundle]` attributes. Co-authored-by: Carter Anderson <mcanders1@gmail.com>
bevyengine#2975) @BoxyUwU this is your fault. Also cart didn't arrive in time to tell us not to do this. # Objective - Fix bevyengine#2974 ## Solution - The first commit just does the actual change - Follow up commits do steps to prove that this method works to unify as required, but this does not remove `insert_bundle`. ## Changelog ### Changed Nested bundles now collapse automatically, and every `Component` now implements `Bundle`. This means that you can combine bundles and components arbitrarily, for example: ```rust // before: .insert(A).insert_bundle(MyBBundle{..}) // after: .insert_bundle((A, MyBBundle {..})) ``` Note that there will be a follow up PR that removes the current `insert` impl and renames `insert_bundle` to `insert`. ### Removed The `bundle` attribute in `derive(Bundle)`. ## Migration guide In `derive(Bundle)`, the `bundle` attribute has been removed. Nested bundles are not collapsed automatically. You should remove `#[bundle]` attributes. Co-authored-by: Carter Anderson <mcanders1@gmail.com>
bevyengine#2975) @BoxyUwU this is your fault. Also cart didn't arrive in time to tell us not to do this. # Objective - Fix bevyengine#2974 ## Solution - The first commit just does the actual change - Follow up commits do steps to prove that this method works to unify as required, but this does not remove `insert_bundle`. ## Changelog ### Changed Nested bundles now collapse automatically, and every `Component` now implements `Bundle`. This means that you can combine bundles and components arbitrarily, for example: ```rust // before: .insert(A).insert_bundle(MyBBundle{..}) // after: .insert_bundle((A, MyBBundle {..})) ``` Note that there will be a follow up PR that removes the current `insert` impl and renames `insert_bundle` to `insert`. ### Removed The `bundle` attribute in `derive(Bundle)`. ## Migration guide In `derive(Bundle)`, the `bundle` attribute has been removed. Nested bundles are not collapsed automatically. You should remove `#[bundle]` attributes. Co-authored-by: Carter Anderson <mcanders1@gmail.com>
What problem does this solve or what need does it fill?
The distinction between components and bundles is often lost on new users, and we end up with a lot of redundant, nearly-duplicated APIs.
What solution would you like?
Implement Bundle for every Component, representing the one element bundle containing only that component.
Remove all duplicated API surfaces, keeping the
Component
names but accepting bundles:insert
andinsert_bundle
->insert
remove
andremove_bundle
->remove
spawn_bundle
; there's now less boilerplate and the design of this routinely confuses the mental model for new usersThere are some additional benefits:
What alternative(s) have you considered?
Just leave it as is.
Additional context
Design credit to @BoxyUwU, @TheRawMeatball and @Frizi. Originally described here: https://hackmd.io/EvBrYLQeQbCjE2eycBlo3w
Made possible by #2254!
The text was updated successfully, but these errors were encountered: