-
-
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
bevy_ecs: add untyped methods for inserting components and bundles #5602
bevy_ecs: add untyped methods for inserting components and bundles #5602
Conversation
Pining here to see if there is any plans to pursue this PR? |
ping @jakobhellermann |
Sorry for not responding earlier. I'm a bit busy with Uni right now but feel free to adopt this PR. |
2e1fe5a
to
199c646
Compare
I rebased the PR and adressed the review comments. It's not quite ready for review yet because I made the |
199c646
to
34c9332
Compare
@@ -665,6 +672,7 @@ impl<'a, 'b> BundleSpawner<'a, 'b> { | |||
pub struct Bundles { | |||
bundle_infos: Vec<BundleInfo>, | |||
bundle_ids: HashMap<TypeId, BundleId>, | |||
bundle_ids_dynamic: HashMap<Vec<ComponentId>, BundleId>, |
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.
The Vec<ComponentId>
could be reduced to a Box<[ComponentId]>
since it's never appending elements, therefore the entire Vec
is not needed.
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.
Creating boxed slices will induce a lot of unnecessary reallocations which are far more expensive than the 8 bytes you save of not having a Vec
.
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.
ahh yes true, forgot the Vec would need to drop any excess storage and possibly reallocate. Not just a simple pointer transfer.
34c9332
to
25115e8
Compare
rebased the PR |
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 taken a look, and this seems solid from an API side. @DJMcNab, you made a very similar PR for me at one point, can I get your review?
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'm not quite sure on the API design here; allocating vectors every time you need this is unfortunate.
I also think that the ordering things require more careful consideration.
DynamicBundle also needs to be unsafe I believe. Hmm, technically if all the APIs which use it require it to be implemented correctly as part of their contracts, maybe not?
OTOH, I don't mind making this slow that much; people should just write their game code in Rust.
mut component_ids: Vec<ComponentId>, | ||
components: I, | ||
) -> &mut Self { | ||
component_ids.sort(); |
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 not obvious correct.
Do you not also need to simultaneously sort the components
?
This PR looks amazing and is super helpful! I was looking into implementing something similar and would like to throw a suggestion up for discussion, Would it be possible to move the The resulting This would have the added benefit of being able to remove Could rename EDIT: Dumped the changes in jakobhellermann#69 |
Controversial because we need to decide between this and #7204 :) |
…7204) This MR is a rebased and alternative proposal to #5602 # Objective - #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`
Objective
TypeId
) APIs for inserting/accessing resources and accessing components, but left inserting components for another PR (this one)Solution
EntityMut::{insert_by_id, insert_bundle_by_id}
Bundle
intoDynamicBundle
withget_components
andBundle: DynamicBundle
. This allows theBundleInserter
machinery to be reused for bundles that can only be written, not read, and have no statically availableComponentId
sBundles::bundle_ids_dynamic: HashMap<Vec<ComponentId>, BundleId>
next to theHashMap<TypeId, BundleId>
Changelog
world.entity_mut(entity).insert_bundle_by_id/insert_by_id
Unresolved questions
can we get rid of some allocations?
init_info_dynamic
could remove a.clone()
of aVec<ComponentId>
whenHashMap::raw_entry
stabilizesinsert_by_id
could not reuseinsert_bundle_by_id
to avoid theVec
overhead, or alternativelyinsert_bundle_by_id
could maybe be changed to not require theVec
for theOwningPtr
scurrently
insert_bundle_by_ids
requires takes aVec<ComponentId>
that must be sorted and aVec<OwningPtr<'_>
that must match the component ids. This is because internally we also need theVec<ComponentId>
so if the caller already has it we can just use that.&[(ComponentId, OwningPtr<'_>)]
and internally collect that into a sorted vec of component ids?ComponentId
vec in the function so that we don't have such a subtle safety requirement? sorting an already sorted vec is the best case scenario for insertion sort (which is used forstd
sort for <=20 elements) so it's just a smallO(n)
cost