Provide getters for fields of ReflectFromPtr #9748
Conversation
alice-i-cecile
added
C-Usability
There was a problem hiding this comment.
I think this reasoning makes sense. get_to_reflect is pretty awful as a name, but I don't have any really strong alternatives. get_reflect_conversion and get_reflect_mut_conversion maybe?
I'd really like to either see those safety comments duplicated on the fields themselves, or #[inline] annotations + using these methods internally.
|
I agree the name is pretty poor. I think the field should be renamed to |
|
I think I'm happy enough with that rename :) @MrGVSV may have other opinions, but we can paint that bike shed when we get to it. |
The reasoning is similar to bevyengine#8687. I'm building a dynamic query. Currently, I store the ReflectFromPtr in my dynamic `Fetch` type. However, `ReflectFromPtr` is: - 16 bytes for TypeId - 8 bytes for the non-mutable function pointer - 8 bytes for the mutable function pointer It's a lot, it adds 32 bytes to my base `Fetch` which is only `ComponendId` (8 bytes) for a total of 40 bytes. I only need one function per fetch, reducing the total dynamic fetch size to 16 bytes. Since I'm querying the components by the ComponendId associated with the function pointer I'm using, I don't need the TypeId, it's a redundant check. In fact, I've difficulties coming up with situations where checking the TypeId beforehand is relevant. So to me, if ReflectFromPtr makes sense as a public API, exposing the function pointers also makes sense.
nicopap
force-pushed
the
public_to_reflect
branch
from
81a94a8
to
6e78ad9
Compare
nicopap
added
the
C-Breaking-Change
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Could you add some context on what users should update when this change get released in a new version of Bevy? |
alice-i-cecile
added
the
S-Ready-For-Final-Review
Head branch was pushed to by a user without write access
|
@alice-i-cecile I forgot to update some doc examples and CI didn't pass. It should be fixed now. |
| unsafe { ptr.deref::<T>() as &dyn Reflect } | ||
| }, | ||
| from_ptr_mut: |ptr| { | ||
| // SAFE: only called from `as_reflect_mut`, where the `ptr` is guaranteed to be of type `T`, | ||
| // and `as_reflect_ptr_mut`, where the caller promises to call it with type `T` | ||
| // SAFETY: same as above, but foor `as_reflect_mut`, `from_ptr_mut` and `deref_mut`. |
There was a problem hiding this comment.
Oh noes foor, that's what I get for hurried changes.
# Objective The reasoning is similar to bevyengine#8687. I'm building a dynamic query. Currently, I store the ReflectFromPtr in my dynamic `Fetch` type. [See relevant code](https://github.com/nicopap/bevy_mod_dynamic_query/blob/97ba68ae1e13f15cabfac1dcd58c2483396dfd3f/src/fetches.rs#L14-L17) However, `ReflectFromPtr` is: - 16 bytes for TypeId - 8 bytes for the non-mutable function pointer - 8 bytes for the mutable function pointer It's a lot, it adds 32 bytes to my base `Fetch` which is only `ComponendId` (8 bytes) for a total of 40 bytes. I only need one function per fetch, reducing the total dynamic fetch size to 16 bytes. Since I'm querying the components by the ComponendId associated with the function pointer I'm using, I don't need the TypeId, it's a redundant check. In fact, I've difficulties coming up with situations where checking the TypeId beforehand is relevant. So to me, if ReflectFromPtr makes sense as a public API, exposing the function pointers also makes sense. ## Solution - Make the fields public through methods. --- ## Changelog - Add `from_ptr` and `from_ptr_mut` methods to `ReflectFromPtr` to access the underlying function pointers - `ReflectFromPtr::as_reflect_ptr` is now `ReflectFromPtr::as_reflect` - `ReflectFromPtr::as_reflect_ptr_mut` is now `ReflectFromPtr::as_reflect_mut` ## Migration guide - `ReflectFromPtr::as_reflect_ptr` is now `ReflectFromPtr::as_reflect` - `ReflectFromPtr::as_reflect_ptr_mut` is now `ReflectFromPtr::as_reflect_mut`
Objective
The reasoning is similar to #8687.
I'm building a dynamic query. Currently, I store the ReflectFromPtr in my dynamic
Fetchtype.See relevant code
However,
ReflectFromPtris:It's a lot, it adds 32 bytes to my base
Fetchwhich is onlyComponendId(8 bytes) for a total of 40 bytes.I only need one function per fetch, reducing the total dynamic fetch size to 16 bytes.
Since I'm querying the components by the ComponendId associated with the function pointer I'm using, I don't need the TypeId, it's a redundant check.
In fact, I've difficulties coming up with situations where checking the TypeId beforehand is relevant. So to me, if ReflectFromPtr makes sense as a public API, exposing the function pointers also makes sense.
Solution
Changelog
from_ptrandfrom_ptr_mutmethods toReflectFromPtrto access the underlying function pointersReflectFromPtr::as_reflect_ptris nowReflectFromPtr::as_reflectReflectFromPtr::as_reflect_ptr_mutis nowReflectFromPtr::as_reflect_mutMigration guide
ReflectFromPtr::as_reflect_ptris nowReflectFromPtr::as_reflectReflectFromPtr::as_reflect_ptr_mutis nowReflectFromPtr::as_reflect_mut