[Merged by Bors] - Refactor ECS to reduce the dependency on a 1-to-1 mapping between components and real rust types#2490
[Merged by Bors] - Refactor ECS to reduce the dependency on a 1-to-1 mapping between components and real rust types#2490bjorn3 wants to merge 8 commits intobevyengine:mainfrom
Conversation
bjorn3
added
A-ECS
|
cc @BoxyUwU re your relationships PR. |
| type_id: Some(TypeId::of::<T>()), | ||
| layout: Layout::new::<T>(), | ||
| drop: Self::drop_ptr::<T>, | ||
| } |
There was a problem hiding this comment.
Making this public may help with #2486, but there are probably other places that need to be changed too for it to work.
| /// The [`ComponentDescriptor`] must match the [`TypeId`] | ||
| #[inline] | ||
| pub(crate) fn get_or_insert_with( | ||
| pub(crate) unsafe fn get_or_insert_with( |
There was a problem hiding this comment.
This method was already de-facto unsafe. Nobody checked that the TypeId in the TypeInfo matched the type_id argument.
| &mut self, | ||
| type_id: TypeId, | ||
| func: impl FnOnce() -> TypeInfo, | ||
| func: impl FnOnce() -> ComponentDescriptor, |
There was a problem hiding this comment.
do we know if perf is actually worse without this func param? can we just take a ComponentDescriptor always and drop the type_id arg?
There was a problem hiding this comment.
ComponentDescriptor allocates a String for the type name. That is not something you want in the hot path.
There was a problem hiding this comment.
type_name returns a &'static str though, seems like we could atleast use Cow<&'static str> here. Wouldn't be the best for custom names though :frown:
There was a problem hiding this comment.
Cow<'static, str> would work. The name is not frequently used, so the extra branch shouldn't hurt. I will leave that for another PR though I think.
There was a problem hiding this comment.
I think personally I'd rather drop the unsafe, use a Cow and then always take a ComponentDescriptor
There was a problem hiding this comment.
ComponentDescriptor is 72 bytes with String and 80 bytes with Cow, which is relatively large. https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=93f494d4a982196b786f0f7d7f69c895
There was a problem hiding this comment.
I dont know if that actually tells us anything about the performance here though? Can we run benches or whatever and see
cart
added
the
S-Pre-Relicense
mockersf
removed
the
S-Pre-Relicense
cart
left a comment
cart
left a comment
There was a problem hiding this comment.
Looks good to me. Just some nits
This allows it to work with dynamic or untyped components if we get any. It would also be useful for the relationships PR.
This allow TypeInfo to exist for non-rust types. It is not yet possible to construct such a TypeInfo.
|
bors r+ |
…ponents and real rust types (#2490) # Objective There is currently a 1-to-1 mapping between components and real rust types. This means that it is impossible for multiple components to be represented by the same rust type or for a component to not have a rust type at all. This means that component types can't be defined in languages other than rust like necessary for scripting or sandboxed (wasm?) plugins. ## Solution Refactor `ComponentDescriptor` and `Bundle` to remove `TypeInfo`. `Bundle` now uses `ComponentId` instead. `ComponentDescriptor` is now always created from a rust type instead of through the `TypeInfo` indirection. A future PR may make it possible to construct a `ComponentDescriptor` from it's fields without a rust type being involved.
|
Timed out. |
|
bors retry |
…ponents and real rust types (#2490) # Objective There is currently a 1-to-1 mapping between components and real rust types. This means that it is impossible for multiple components to be represented by the same rust type or for a component to not have a rust type at all. This means that component types can't be defined in languages other than rust like necessary for scripting or sandboxed (wasm?) plugins. ## Solution Refactor `ComponentDescriptor` and `Bundle` to remove `TypeInfo`. `Bundle` now uses `ComponentId` instead. `ComponentDescriptor` is now always created from a rust type instead of through the `TypeInfo` indirection. A future PR may make it possible to construct a `ComponentDescriptor` from it's fields without a rust type being involved.
|
nice that worked! |
bors
bot
changed the title
5 participants
Objective
There is currently a 1-to-1 mapping between components and real rust types. This means that it is impossible for multiple components to be represented by the same rust type or for a component to not have a rust type at all. This means that component types can't be defined in languages other than rust like necessary for scripting or sandboxed (wasm?) plugins.
Solution
Refactor
ComponentDescriptorandBundleto removeTypeInfo.Bundlenow usesComponentIdinstead.ComponentDescriptoris now always created from a rust type instead of through theTypeInfoindirection. A future PR may make it possible to construct aComponentDescriptorfrom it's fields without a rust type being involved.