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

bevy_reflect: Support Cow<'static, [T]> #7429

Closed
ItsDoot opened this issue Jan 30, 2023 · 1 comment · Fixed by #7454
Closed

bevy_reflect: Support Cow<'static, [T]> #7429

ItsDoot opened this issue Jan 30, 2023 · 1 comment · Fixed by #7454
Labels
A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@ItsDoot
Copy link
Contributor

ItsDoot commented Jan 30, 2023

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

At present, only Cow<'static, str> implements Reflect. However, Cow has another use: Cow<'static, [T]> for owned or borrowed lists.

What solution would you like?

Implement Reflect and all other associated/required traits for Cow<'static, [T]>.

What alternative(s) have you considered?

Just use a Vec<T>. However, Vecs don't work for const lists like arrays, and potentially use up more memory in the same way a String and Cow<'static, str> are compared.

@ItsDoot ItsDoot added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jan 30, 2023
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types D-Trivial Nice and easy! A great choice to get started with Bevy and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jan 31, 2023
@james7132
Copy link
Member

Wonder if we should generically support Cow<'static, T> for the relevant traits (with the appropriate bounds).

alice-i-cecile pushed a commit that referenced this issue May 8, 2023
# Objective

- Add Reflect and FromReflect for AssetPath
- Fixes #8458

## Solution

- Straightforward derive of `Reflect` and `FromReflect` for `AssetPath`
- Implement `Reflect` and `FromReflect` for `Cow<'static, Path>` as to
satisfy the 'static lifetime requierments of bevy_reflect.
Implementation is a direct copy of that for `Cow<'static, str>` so maybe
it begs the question that was already asked in #7429 - maybe it would be
benefitial to write a general implementation for `Reflect` for
`Cow<'static, T>`.
james7132 pushed a commit to james7132/bevy that referenced this issue Jun 19, 2023
# Objective

- Implementing reflection for Cow<'static, [T]>
- Hopefully fixes bevyengine#7429

## Solution

- Implementing Reflect, Typed, GetTypeRegistration, and FromReflect for
Cow<'static, [T]>

---

## Notes

I have not used bevy_reflection much yet, so I may not fully understand
all the use cases. This is also my first attempt at contributing, so I
would appreciate any feedback or recommendations for changes. I tried to
add cases for using Cow<'static, str> and Cow<'static, [u8]> to some of
the bevy_reflect tests, but I can't guarantee those tests are
comprehensive enough.

---------

Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
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-Trivial Nice and easy! A great choice to get started with Bevy
Projects
Status: Done
3 participants