[Merged by Bors] - untyped APIs for components and resources #4447

jakobhellermann · 2022-04-09T18:11:26Z

Objective

Even if bevy itself does not provide any builtin scripting or modding APIs, it should have the foundations for building them yourself.
For that it should be enough to have APIs that are not tied to the actual rust types with generics, but rather accept ComponentIds and bevy_ptr ptrs.

Solution

Add the following APIs to bevy

fn EntityRef::get_by_id(ComponentId) -> Option<Ptr<'w>>;
fn EntityMut::get_by_id(ComponentId) -> Option<Ptr<'_>>;
fn EntityMut::get_mut_by_id(ComponentId) -> Option<MutUntyped<'_>>;

fn World::get_resource_by_id(ComponentId) -> Option<Ptr<'_>>;
fn World::get_resource_mut_by_id(ComponentId) -> Option<MutUntyped<'_>>;

// Safety: `value` must point to a valid value of the component
unsafe fn World::insert_resource_by_id(ComponentId, value: OwningPtr);

fn ComponentDescriptor::new_with_layout(..) -> Self;
fn World::init_component_with_descriptor(ComponentDescriptor) -> ComponentId;

This PR would definitely benefit from #3001 (lifetime'd pointers) to make sure that the lifetimes of the pointers are valid and the my-move pointer in insert_resource_by_id could be an OwningPtr, but that can be adapter later if/when #3001 is merged.

Not in this PR

inserting components on entities (this is very tied to types with bundles and the BundleInserter)
an untyped version of a query (needs good API design, has a large implementation complexity, can be done in a third-party crate)

MinerSebas · 2022-04-09T18:30:53Z

There is a typo in the PR Description fn EntityMut::get_by_id(ComponentId) -> Option<*mut ()>;, the return type is actualy Option<*const ()>

alice-i-cecile · 2022-04-10T02:24:31Z

This could be very useful for #1527 down the line.

This is an essential first step towards #142.

Related to #623, which was abandoned but is worth poking at.

crates/bevy_ecs/src/change_detection.rs

crates/bevy_ecs/src/component.rs

crates/bevy_ecs/src/world/entity_ref.rs

crates/bevy_ecs/src/world/mod.rs

alice-i-cecile

Very cool to see how easy this is. It looks like it should be quite reasonable to work with and build upon.

Obviously it needs more docs, tests and functionality. For this PR, I think we should toss in basic docs and tests, and build out functionality in later work.

Other small nit: there are a lot of tiny methods that would likely benefit from #[inline]. A lot of these, like get_by_id are likely to be run inside of hot loops.

…_layout

mockersf · 2022-04-11T22:21:07Z

I don't like that this makes the *by_id methods very visible. To hide them a little, we could:

put them behind a feature
put them on a trait not in the prelude

I think I would prefer the second one, a feature would be not enough visible, and a trait would make grouping them in doc easier. What do you think?

alice-i-cecile · 2022-04-11T22:23:05Z

My preference is for the latter: I had the same thought for how to handle integration testing methods.

jakobhellermann · 2022-04-12T08:57:40Z

Hm, I'm not a huge fan of introducing the complexity of a feature or a trait just to discourage the use of those APIs.
What do you think of putting the functions in a separate impl block to force them to the bottom of the rustdoc docs and emphasizing the warning more?

Prioritization in rust-analyzer seems good to me too:

I don't think people would accidentally use them if they are never mentioned prominently in the examples or other resources, most people will never even have seen a ComponentId in the code. They'd need to do world.components().get_id(std::any::TypeId::of::<Component>()).unwrap(), then cast the pointer to their component type and unsafely dereference it, which is enough of a barrier that people will look at the docs and make sure they are using the intended API.

That said, I would prefer a non-prelude trait over a cargo feature.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

james7132

Generally looks good, super useful stuff too. Probably could reduce some of the complexity of some of the built in Reflect functionality with this too.

bjorn3

Custom component insertion is missing, but that can be left for another PR if you prefer.

bjorn3 · 2022-05-24T16:33:18Z

crates/bevy_ecs/src/change_detection.rs

+    /// Returns the pointer to the value, without marking it as changed.
+    ///
+    /// In order to mark the value as changed, you need to call [`set_changed`](DetectChanges::set_changed) manually.
+    pub fn into_inner(self) -> PtrMut<'a> {


You can't call this method before set_changed due to it taking self by value.

Hm, yeah.

A fn into_inner(&mut self) -> &mut PtrMut<'a> wouldn't be very useful since methods like PtrMut::deref_mut take self by value.
fn into_inner(&mut self) -> PtrMut<'a> would be bad since you could create aliasing PtrMuts with it.
fn into_inner<'s>(&'s mut self) -> PtrMut<'s>) should be fine I think.

Alternatively we could let into_inner return (PtrMut<'a>, Ticks<'a>) but we'd have to make Ticks public.

You can call set_changed first at least, so I think I'm okay with this design.

jakobhellermann · 2022-05-24T17:54:32Z

Custom component insertion is missing, but that can be left for another PR if you prefer.

I'd prefer to do that in a separate PR because I'm not very familiar with that code and it looks very tied to rust types and Bundles.

alice-i-cecile · 2022-05-30T15:23:34Z

bors r+

alice-i-cecile

Well-engineered, well-motivated, solid docs. I'm very happy with this work.

# Objective Even if bevy itself does not provide any builtin scripting or modding APIs, it should have the foundations for building them yourself. For that it should be enough to have APIs that are not tied to the actual rust types with generics, but rather accept `ComponentId`s and `bevy_ptr` ptrs. ## Solution Add the following APIs to bevy ```rust fn EntityRef::get_by_id(ComponentId) -> Option<Ptr<'w>>; fn EntityMut::get_by_id(ComponentId) -> Option<Ptr<'_>>; fn EntityMut::get_mut_by_id(ComponentId) -> Option<MutUntyped<'_>>; fn World::get_resource_by_id(ComponentId) -> Option<Ptr<'_>>; fn World::get_resource_mut_by_id(ComponentId) -> Option<MutUntyped<'_>>; // Safety: `value` must point to a valid value of the component unsafe fn World::insert_resource_by_id(ComponentId, value: OwningPtr); fn ComponentDescriptor::new_with_layout(..) -> Self; fn World::init_component_with_descriptor(ComponentDescriptor) -> ComponentId; ``` ~~This PR would definitely benefit from #3001 (lifetime'd pointers) to make sure that the lifetimes of the pointers are valid and the my-move pointer in `insert_resource_by_id` could be an `OwningPtr`, but that can be adapter later if/when #3001 is merged.~~ ### Not in this PR - inserting components on entities (this is very tied to types with bundles and the `BundleInserter`) - an untyped version of a query (needs good API design, has a large implementation complexity, can be done in a third-party crate) Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>

bors · 2022-05-30T15:57:18Z

Pull request successfully merged into main.

Build succeeded:

# Objective Even if bevy itself does not provide any builtin scripting or modding APIs, it should have the foundations for building them yourself. For that it should be enough to have APIs that are not tied to the actual rust types with generics, but rather accept `ComponentId`s and `bevy_ptr` ptrs. ## Solution Add the following APIs to bevy ```rust fn EntityRef::get_by_id(ComponentId) -> Option<Ptr<'w>>; fn EntityMut::get_by_id(ComponentId) -> Option<Ptr<'_>>; fn EntityMut::get_mut_by_id(ComponentId) -> Option<MutUntyped<'_>>; fn World::get_resource_by_id(ComponentId) -> Option<Ptr<'_>>; fn World::get_resource_mut_by_id(ComponentId) -> Option<MutUntyped<'_>>; // Safety: `value` must point to a valid value of the component unsafe fn World::insert_resource_by_id(ComponentId, value: OwningPtr); fn ComponentDescriptor::new_with_layout(..) -> Self; fn World::init_component_with_descriptor(ComponentDescriptor) -> ComponentId; ``` ~~This PR would definitely benefit from bevyengine#3001 (lifetime'd pointers) to make sure that the lifetimes of the pointers are valid and the my-move pointer in `insert_resource_by_id` could be an `OwningPtr`, but that can be adapter later if/when bevyengine#3001 is merged.~~ ### Not in this PR - inserting components on entities (this is very tied to types with bundles and the `BundleInserter`) - an untyped version of a query (needs good API design, has a large implementation complexity, can be done in a third-party crate) Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>

…t ()` (#4475) # Objective #4447 adds functions that can fetch resources/components as `*const ()` ptr by providing the `ComponentId`. This alone is not enough for them to be usable safely with reflection, because there is no general way to go from the raw pointer to a `&dyn Reflect` which is the pointer + a pointer to the VTable of the `Reflect` impl. By adding a `ReflectFromPtr` type that is included in the type type registration when deriving `Reflect`, safe functions can be implemented in scripting languages that don't assume a type layout and can access the component data via reflection: ```rust #[derive(Reflect)] struct StringResource { value: String } ``` ```lua local res_id = world:resource_id_by_name("example::StringResource") local res = world:resource(res_id) print(res.value) ``` ## Solution 1. add a `ReflectFromPtr` type with a `FromType<T: Reflect>` implementation and the following methods: - ` pub unsafe fn as_reflect_ptr<'a>(&self, val: Ptr<'a>) -> &'a dyn Reflect` - ` pub unsafe fn as_reflect_ptr_mut<'a>(&self, val: PtrMut<'a>) -> &'a mud dyn Reflect` Safety requirements of the methods are that you need to check that the `ReflectFromPtr` was constructed for the correct type. 2. add that type to the `TypeRegistration` in the `GetTypeRegistration` impl generated by `#[derive(Reflect)]`. This is different to other reflected traits because it doesn't need `#[reflect(ReflectReflectFromPtr)]` which IMO should be there by default. Co-authored-by: Jakob Hellermann <hellermann@sipgate.de> Co-authored-by: Carter Anderson <mcanders1@gmail.com>

…t ()` (bevyengine#4475) # Objective bevyengine#4447 adds functions that can fetch resources/components as `*const ()` ptr by providing the `ComponentId`. This alone is not enough for them to be usable safely with reflection, because there is no general way to go from the raw pointer to a `&dyn Reflect` which is the pointer + a pointer to the VTable of the `Reflect` impl. By adding a `ReflectFromPtr` type that is included in the type type registration when deriving `Reflect`, safe functions can be implemented in scripting languages that don't assume a type layout and can access the component data via reflection: ```rust #[derive(Reflect)] struct StringResource { value: String } ``` ```lua local res_id = world:resource_id_by_name("example::StringResource") local res = world:resource(res_id) print(res.value) ``` ## Solution 1. add a `ReflectFromPtr` type with a `FromType<T: Reflect>` implementation and the following methods: - ` pub unsafe fn as_reflect_ptr<'a>(&self, val: Ptr<'a>) -> &'a dyn Reflect` - ` pub unsafe fn as_reflect_ptr_mut<'a>(&self, val: PtrMut<'a>) -> &'a mud dyn Reflect` Safety requirements of the methods are that you need to check that the `ReflectFromPtr` was constructed for the correct type. 2. add that type to the `TypeRegistration` in the `GetTypeRegistration` impl generated by `#[derive(Reflect)]`. This is different to other reflected traits because it doesn't need `#[reflect(ReflectReflectFromPtr)]` which IMO should be there by default. Co-authored-by: Jakob Hellermann <hellermann@sipgate.de> Co-authored-by: Carter Anderson <mcanders1@gmail.com>

# Objective remove `insert_resource_with_id` because `insert_resource_by_id` exists and does almost exactly the same thing blocked on #5587 because otherwise we will leak a resource when it's inserted ## Solution remove the function and also add a safety invariant of to `insert_resource_by_id` that the id be valid for the world. I didn't see any discussion in #4447 about this safety invariant being left off in favor of a panic so I'm curious if there was one or if it just seemed nicer to have less safety invariants for callers to uphold 😅 --- ## Changelog - safety invariant added to `insert_resource_by_id` requiring the id to be valid for world ## Migration Guide - audit any calls to `insert_resource_by_id` making sure that the id is valid for the world Co-authored-by: Carter Anderson <mcanders1@gmail.com>

…t ()` (bevyengine#4475) # Objective bevyengine#4447 adds functions that can fetch resources/components as `*const ()` ptr by providing the `ComponentId`. This alone is not enough for them to be usable safely with reflection, because there is no general way to go from the raw pointer to a `&dyn Reflect` which is the pointer + a pointer to the VTable of the `Reflect` impl. By adding a `ReflectFromPtr` type that is included in the type type registration when deriving `Reflect`, safe functions can be implemented in scripting languages that don't assume a type layout and can access the component data via reflection: ```rust #[derive(Reflect)] struct StringResource { value: String } ``` ```lua local res_id = world:resource_id_by_name("example::StringResource") local res = world:resource(res_id) print(res.value) ``` ## Solution 1. add a `ReflectFromPtr` type with a `FromType<T: Reflect>` implementation and the following methods: - ` pub unsafe fn as_reflect_ptr<'a>(&self, val: Ptr<'a>) -> &'a dyn Reflect` - ` pub unsafe fn as_reflect_ptr_mut<'a>(&self, val: PtrMut<'a>) -> &'a mud dyn Reflect` Safety requirements of the methods are that you need to check that the `ReflectFromPtr` was constructed for the correct type. 2. add that type to the `TypeRegistration` in the `GetTypeRegistration` impl generated by `#[derive(Reflect)]`. This is different to other reflected traits because it doesn't need `#[reflect(ReflectReflectFromPtr)]` which IMO should be there by default. Co-authored-by: Jakob Hellermann <hellermann@sipgate.de> Co-authored-by: Carter Anderson <mcanders1@gmail.com>

# Objective remove `insert_resource_with_id` because `insert_resource_by_id` exists and does almost exactly the same thing blocked on bevyengine#5587 because otherwise we will leak a resource when it's inserted ## Solution remove the function and also add a safety invariant of to `insert_resource_by_id` that the id be valid for the world. I didn't see any discussion in bevyengine#4447 about this safety invariant being left off in favor of a panic so I'm curious if there was one or if it just seemed nicer to have less safety invariants for callers to uphold 😅 --- ## Changelog - safety invariant added to `insert_resource_by_id` requiring the id to be valid for world ## Migration Guide - audit any calls to `insert_resource_by_id` making sure that the id is valid for the world Co-authored-by: Carter Anderson <mcanders1@gmail.com>

…t ()` (bevyengine#4475) # Objective bevyengine#4447 adds functions that can fetch resources/components as `*const ()` ptr by providing the `ComponentId`. This alone is not enough for them to be usable safely with reflection, because there is no general way to go from the raw pointer to a `&dyn Reflect` which is the pointer + a pointer to the VTable of the `Reflect` impl. By adding a `ReflectFromPtr` type that is included in the type type registration when deriving `Reflect`, safe functions can be implemented in scripting languages that don't assume a type layout and can access the component data via reflection: ```rust #[derive(Reflect)] struct StringResource { value: String } ``` ```lua local res_id = world:resource_id_by_name("example::StringResource") local res = world:resource(res_id) print(res.value) ``` ## Solution 1. add a `ReflectFromPtr` type with a `FromType<T: Reflect>` implementation and the following methods: - ` pub unsafe fn as_reflect_ptr<'a>(&self, val: Ptr<'a>) -> &'a dyn Reflect` - ` pub unsafe fn as_reflect_ptr_mut<'a>(&self, val: PtrMut<'a>) -> &'a mud dyn Reflect` Safety requirements of the methods are that you need to check that the `ReflectFromPtr` was constructed for the correct type. 2. add that type to the `TypeRegistration` in the `GetTypeRegistration` impl generated by `#[derive(Reflect)]`. This is different to other reflected traits because it doesn't need `#[reflect(ReflectReflectFromPtr)]` which IMO should be there by default. Co-authored-by: Jakob Hellermann <hellermann@sipgate.de> Co-authored-by: Carter Anderson <mcanders1@gmail.com>

# Objective remove `insert_resource_with_id` because `insert_resource_by_id` exists and does almost exactly the same thing blocked on bevyengine#5587 because otherwise we will leak a resource when it's inserted ## Solution remove the function and also add a safety invariant of to `insert_resource_by_id` that the id be valid for the world. I didn't see any discussion in bevyengine#4447 about this safety invariant being left off in favor of a panic so I'm curious if there was one or if it just seemed nicer to have less safety invariants for callers to uphold 😅 --- ## Changelog - safety invariant added to `insert_resource_by_id` requiring the id to be valid for world ## Migration Guide - audit any calls to `insert_resource_by_id` making sure that the id is valid for the world Co-authored-by: Carter Anderson <mcanders1@gmail.com>

# Objective Even if bevy itself does not provide any builtin scripting or modding APIs, it should have the foundations for building them yourself. For that it should be enough to have APIs that are not tied to the actual rust types with generics, but rather accept `ComponentId`s and `bevy_ptr` ptrs. ## Solution Add the following APIs to bevy ```rust fn EntityRef::get_by_id(ComponentId) -> Option<Ptr<'w>>; fn EntityMut::get_by_id(ComponentId) -> Option<Ptr<'_>>; fn EntityMut::get_mut_by_id(ComponentId) -> Option<MutUntyped<'_>>; fn World::get_resource_by_id(ComponentId) -> Option<Ptr<'_>>; fn World::get_resource_mut_by_id(ComponentId) -> Option<MutUntyped<'_>>; // Safety: `value` must point to a valid value of the component unsafe fn World::insert_resource_by_id(ComponentId, value: OwningPtr); fn ComponentDescriptor::new_with_layout(..) -> Self; fn World::init_component_with_descriptor(ComponentDescriptor) -> ComponentId; ``` ~~This PR would definitely benefit from bevyengine#3001 (lifetime'd pointers) to make sure that the lifetimes of the pointers are valid and the my-move pointer in `insert_resource_by_id` could be an `OwningPtr`, but that can be adapter later if/when bevyengine#3001 is merged.~~ ### Not in this PR - inserting components on entities (this is very tied to types with bundles and the `BundleInserter`) - an untyped version of a query (needs good API design, has a large implementation complexity, can be done in a third-party crate) Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>

…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`

jakobhellermann added 6 commits April 9, 2022 20:01

bevy_ecs: untyped API to access components

d0d8d37

bevy_ecs: untyped API to access resources

ae98499

validate nonsend correctness for dynamic resource APIs

d13bbe5

bevy_ecs: add insert_resource_by_id

497685c

bevy_ecs: untyped API to init components

9bcbcae

bevy_ecs: add Components::iter

612fd92

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 9, 2022

TheRawMeatball self-requested a review April 9, 2022 18:49

TheRawMeatball added C-Enhancement A new feature A-ECS Entities, components, systems, and events A-Modding Supporting infrastructure for player-controlled modifications to games and removed S-Needs-Triage This issue needs to be labelled labels Apr 9, 2022