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

OwningPtr should impl From<Box<T>> #13287

Open
dmyyy opened this issue May 9, 2024 · 2 comments
Open

OwningPtr should impl From<Box<T>> #13287

dmyyy opened this issue May 9, 2024 · 2 comments
Labels
A-Pointers Relating to Bevy pointer abstractions A-Reflection Runtime information about types C-Enhancement A new feature D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes X-Uncontroversial This work is generally agreed upon

Comments

@dmyyy
Copy link
Contributor

dmyyy commented May 9, 2024

What problem does this solve or what need does it fill?

I am trying to copy the value for a type erased entity component of type &dyn Reflect.

insert_by_id requires an OwningPtr to be passed in. I want to copy the component &dyn Reflect points to and insert the same component for an entity. I can copy the value of &dyn Reflect via clone_value() giving me Box<dyn Reflect>.

There isn't an easy way to get an OwningPtr from a Box. A Box is a unique pointer to some value on the heap - I don't see any reason why it can't be an OwningPtr.

What solution would you like?

implement From<Box<T>> for OwningPtr.

What alternative(s) have you considered?

This can be done with raw pointers.

Additional context

@dmyyy dmyyy added C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels May 9, 2024
@MrGVSV
Copy link
Member

MrGVSV commented May 9, 2024

Note that for dyn Reflect we probably want to only do this via the ReflectFromPtr type data.

The reason for this is because a Box<dyn Reflect> might not actually be a Box<T>.

Specifically, when cloning via Reflect::clone_value, you almost always get back a dynamic type (e.g. Box<DynamicStruct>). It would be unsafe to treat the OwningPtr as one that points to T when it really points to DynamicStruct.

So ideally we expose this API through the ReflectFromPtr type data to help ensure these kinds of operations are done safely.

@MrGVSV MrGVSV added A-Reflection Runtime information about types A-Pointers Relating to Bevy pointer abstractions D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes X-Uncontroversial This work is generally agreed upon and removed S-Needs-Triage This issue needs to be labelled labels May 9, 2024
@SkiFire13
Copy link
Contributor

Another reason why this should be avoided is that OwningPtr only owns the data, not the allocation, so the proposed conversion would have to leak the allocation.

I would instead suggest using ReflectComponent to insert a reflected value as component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Pointers Relating to Bevy pointer abstractions A-Reflection Runtime information about types C-Enhancement A new feature D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

No branches or pull requests

3 participants