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

bevy_ecs: add untyped methods for inserting components and bundles #7204

Merged
merged 1 commit into from
Mar 21, 2023

Conversation

Suficio
Copy link
Contributor

@Suficio Suficio commented Jan 15, 2023

This MR is a rebased and alternative proposal to #5602

Objective

Solution

  • add EntityMut::insert_by_id

  • split Bundle into DynamicBundle with get_components and Bundle: DynamicBundle. This allows the BundleInserter machinery to be reused for bundles that can only be written, not read, and have no statically available ComponentIds

  • Compared to the original MR this approach exposes unsafe endpoints and requires the user to manage instantiated BundleIds. This is quite easy for the end user to do and does not incur the performance penalty of checking whether component input is correctly provided for the BundleId.

  • This MR does ensure that constructing BundleId itself is safe


Changelog

  • add methods for inserting bundles and components to: world.entity_mut(entity).insert_by_id

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use D-Complex Quite challenging from either a design or technical perspective. Ask for help! labels Jan 15, 2023
@alice-i-cecile
Copy link
Member

I like it! Needs more docs though.

@james7132 james7132 self-requested a review January 15, 2023 05:53
Copy link
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.

Very nice, I'm happy with this now. Please feel free to resolve my previous comments to make reviewing easier.

Copy link
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.

Very nice, I'm happy with this now. Please feel free to resolve my previous comments to make reviewing easier.

@alice-i-cecile alice-i-cecile added A-Reflection Runtime information about types X-Controversial There is active debate or serious implications around merging this PR labels Jan 15, 2023
@alice-i-cecile
Copy link
Member

Controversial because we need to decide between this and #5602.

Copy link
Contributor

@jakobhellermann jakobhellermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments.

What I would really like to see is a method for inserting a single component that doesn't require manual caching and unsafety (other than OwningPtr).

crates/bevy_ecs/src/bundle.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
@Suficio
Copy link
Contributor Author

Suficio commented Jan 17, 2023

Thanks for the review @jakobhellermann @james7132!

I made the following changes to address your review comments:

  • init_dynamic_info now caches dynamic bundles and only verifies component existence when creating new bundles.
  • init_dynamic_info caches into two hash maps, one for dynamic length bundles and the single component bundle case (additionally caching the storage type).
  • Added insert_by_id which only takes ComponentId, turned out very nicely
  • insert_bundle_by_id now doesn't require the storage type
  • Added insert_bundle_by_id_unchecked which is used by insert_by_id and insert_bundle_by_id but is also available to be used publicly (as it skips an allocation).

@alice-i-cecile
Copy link
Member

I like the new changes a lot; this is a much friendlier API now.

@Suficio
Copy link
Contributor Author

Suficio commented Jan 18, 2023

Now that I think about it init_dynamic_info is quite cheap to call so I could get away with creating API that doesn't expose BundleId.

@Suficio Suficio requested review from jakobhellermann and removed request for james7132 and MrGVSV January 18, 2023 20:05
@Suficio
Copy link
Contributor Author

Suficio commented Jan 18, 2023

Worked like a charm, only insert_by_id and insert_bundle_by_id are retained as public APIs.

The only drawback is I had to cache the storage types of bundle components, this seems relatively innocuous but can be relatively easily removed.

crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/bundle.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/bundle.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
@Suficio Suficio force-pushed the insert-components branch 2 times, most recently from 0306cf5 to 504aec9 Compare January 29, 2023 01:22
@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 3, 2023
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be significantly slower than the typed API, but I don't think there's a solid way around that.

IMO we should clean up the HashMaps used in Bundles, but that's strictly non-blocking and can be tackled in a follow-up PR.

bundle_ids: HashMap<TypeId, BundleId>,
/// Cache dynamic [`BundleId`] with multiple components
dynamic_bundle_ids: HashMap<Vec<ComponentId>, (BundleId, Vec<StorageType>)>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: I feel like this could be better represented by putting StorageType back into BundleInfo and replacing the Vec<ComponentId> with Vec<BundleComponent> which contains both the ID and the storage type. The same for the below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can definitely do this in a follow-up for removing dynamic components!

crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
@Suficio
Copy link
Contributor Author

Suficio commented Mar 10, 2023

Rebased PR

Renamed insert_bundle_by_id to insert_by_ids

@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Mar 10, 2023
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I think this is how we should do it. Minimally invasive, which I like.

@cart cart added this pull request to the merge queue Mar 21, 2023
@cart cart merged commit caa6622 into bevyengine:main Mar 21, 2023
@Suficio Suficio deleted the insert-components branch June 12, 2023 21:30
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 A-Reflection Runtime information about types C-Usability A simple quality-of-life change that makes Bevy easier to use D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

6 participants