Skip to content

Conversation

@Escapingbug
Copy link

Objective

Solution

For most methods, we are able to use inner wrapped reflect value directly through self.as_ref() and rely on that when the semantic seems to be valid.
However there are several design choices that I made by my own and may need more discussion.
My preference is to minimize the changes so hopefully I don't introduce too many bugs in this PR.
So:

  1. I decided to make Box not a Value but a TupleStruct. Internally within Rust std, the Box is a tuple struct as well but over both the contained value and the allocator. I was saving the allocator part unless it is really needed in the future. In the previous PR (namely implement Reflect for Box<T> where T: Reflect #6098 ), it introduces a new kind called Wrapper, my guess is that, the reason it introduces this new kind is to allow access to the internal value contained in the Box but not be confused with the real other kind like TupleStruct. I'm solving this with the next choice, so I'm presuming it is OK to stay TupleStruct for Box.
  2. A new method fn as_inner(&self) -> Option<&dyn Reflect> is introduced within Reflect trait and returns None by default for all non-boxed type. This distinguishes Box type with all other normal TupleStruct types. By this way, we know if a type is a Box-ed or not. The drawback is that, the complexity of the Reflect is increased.

This should not be the only way of implementing Reflect for Box<T>.
We might need a bit discussion on this to make sure if these design choices are reasonable enough.
Also, if I'm wrong about the Wrapper type in #6098 , there's still things that I ignored, I'm happy to rethink my design.

(As a side note, I'm not sure if this is OK to re-implement a proposed PR. If I'm wrong about this, please tell me what is the better way of doing this, I'm OK to change.)


Changelog

  • Added Reflect trait implementation for Box<T> when T implements FromReflect and TypePath trait.

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@MrGVSV MrGVSV self-requested a review September 28, 2023 16:24
@MrGVSV MrGVSV added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Sep 28, 2023
@BenjaminBrienen BenjaminBrienen added D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants