-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Provide getters for fields of ReflectFromPtr #9748
Conversation
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 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.
81a94a8
to
6e78ad9
Compare
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? |
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 agree from_ptr[_mut]
isn't the best but I think it's pretty good. Especially for an API that's more catered to advanced usages anyways.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Fetch
type.See relevant code
However,
ReflectFromPtr
is:It's a lot, it adds 32 bytes to my base
Fetch
which 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_ptr
andfrom_ptr_mut
methods toReflectFromPtr
to access the underlying function pointersReflectFromPtr::as_reflect_ptr
is nowReflectFromPtr::as_reflect
ReflectFromPtr::as_reflect_ptr_mut
is nowReflectFromPtr::as_reflect_mut
Migration guide
ReflectFromPtr::as_reflect_ptr
is nowReflectFromPtr::as_reflect
ReflectFromPtr::as_reflect_ptr_mut
is nowReflectFromPtr::as_reflect_mut