bevy icon indicating copy to clipboard operation
bevy copied to clipboard

bevy_ecs: add untyped methods for inserting components and bundles

Open Suficio opened this issue 2 years ago • 7 comments

This MR is a rebased and alternative proposal to https://github.com/bevyengine/bevy/pull/5602

Objective

  • https://github.com/bevyengine/bevy/pull/4447 implemented untyped (using component ids instead of generics and TypeId) APIs for inserting/accessing resources and accessing components, but left inserting components for another PR (this one)

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

Suficio avatar Jan 15 '23 05:01 Suficio

I like it! Needs more docs though.

alice-i-cecile avatar Jan 15 '23 05:01 alice-i-cecile

Controversial because we need to decide between this and https://github.com/bevyengine/bevy/pull/5602.

alice-i-cecile avatar Jan 15 '23 16:01 alice-i-cecile

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

Suficio avatar Jan 17 '23 08:01 Suficio

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

alice-i-cecile avatar Jan 18 '23 02:01 alice-i-cecile

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 avatar Jan 18 '23 15:01 Suficio

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.

Suficio avatar Jan 18 '23 20:01 Suficio

Rebased PR

Renamed insert_bundle_by_id to insert_by_ids

Suficio avatar Mar 10 '23 02:03 Suficio