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

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

Closed

Conversation

jakobhellermann
Copy link
Contributor

@jakobhellermann jakobhellermann commented Apr 9, 2022

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)

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

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

@TheRawMeatball TheRawMeatball self-requested a review April 9, 2022 18:49
@TheRawMeatball TheRawMeatball added C-Feature A new feature, making something new possible 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
@alice-i-cecile
Copy link
Member

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.

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

@mockersf
Copy link
Member

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

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

@jakobhellermann
Copy link
Contributor Author

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?

rustdoc output with a bold warning to prefer the typed APIs

Prioritization in rust-analyzer seems good to me too:
autocomplete suggestions in rust-analyzer, where the by_id methods are ranked below their typed counterparts

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.

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.

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.

Copy link
Contributor

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

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

/// 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> {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

@IceSentry IceSentry 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 May 24, 2022
@jakobhellermann
Copy link
Contributor Author

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

bors r+

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.

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

bors bot pushed a commit that referenced this pull request May 30, 2022
# 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
Copy link
Contributor

bors bot commented May 30, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title untyped APIs for components and resources [Merged by Bors] - untyped APIs for components and resources May 30, 2022
@bors bors bot closed this May 30, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 7, 2022
# 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>
bors bot pushed a commit that referenced this pull request Jul 19, 2022
…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>
bors bot pushed a commit that referenced this pull request Jul 19, 2022
…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>
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
…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>
bors bot pushed a commit that referenced this pull request Aug 30, 2022
# 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>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
…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>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# 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>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…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>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# 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>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# 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>
cart pushed a commit that referenced this pull request Mar 21, 2023
…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`
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-Modding Supporting infrastructure for player-controlled modifications to games C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants