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.

Sign up for GitHub

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

Jump to bottom

Implement Reflect for arrays #1213

Closed
TehPers opened this issue Jan 6, 2021 · 2 comments
Closed

Implement Reflect for arrays #1213

TehPers opened this issue Jan 6, 2021 · 2 comments
Labels
A-Reflection Runtime information about types C-Feature A new feature, making something new possible

Comments

@TehPers
Copy link
Contributor

TehPers commented Jan 6, 2021 •
edited
Loading

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

Currently, only Vec can be reflected on. Sometimes it's more convenient to store data in an array, and currently it isn't possible to implement Reflect for types that have fields that are arrays.

What solution would you like?

Implement Reflect and List for arrays. It should be possible either via a macro which implements it for common array sizes (1-32, and maybe some powers of 2) or by taking advantage of const generics (currently nightly only).

What alternative(s) have you considered?

For my particular use case, a tuple would have worked as well, but tuples currently don't implement Reflect either. Another option is to just use a Vec, but it doesn't enforce a specific number of elements the way that arrays do.

Edit: Removed the bit about slices. Reflect can't be implemented for slices because of the lifetime.
@TehPers TehPers changed the title Implement Reflect for arrays and slices Implement Reflect for arrays Jan 6, 2021
@Moxinilian Moxinilian added core C-Feature A new feature, making something new possible labels Jan 15, 2021
@TehPers
Copy link
Contributor Author

TehPers commented Apr 9, 2021

Now that const generics are stable, it might be better to just use them rather than only supporting common array sizes.

@alice-i-cecile alice-i-cecile added A-Reflection Runtime information about types and removed core labels Apr 14, 2021
@NathanSWard
Copy link
Contributor

NathanSWard commented Jun 23, 2021 •
edited
Loading

Do we want to implement List for arrays? Especially since push doesn't make sense for these.
IMO having a SizedList trait would probably be the best route.
(Edit: or just even have an Array trait)

@NathanSWard NathanSWard mentioned this issue Jun 23, 2021
Closed
@MrGVSV MrGVSV mentioned this issue May 9, 2022
Closed
@bors bors bot closed this as completed in acbee77 May 13, 2022
@jakobhellermann jakobhellermann mentioned this issue May 17, 2022
Closed
robtfm pushed a commit to robtfm/bevy that referenced this issue May 17, 2022
@MrGVSV @NathanSWard @robtfm
bevy_reflect: Reflect arrays (bevyengine#4701)
6e23270 
# Objective

> ℹ️ **Note**: This is a rebased version of bevyengine#2383. A large portion of it has not been touched (only a few minor changes) so that any additional discussion may happen here. All credit should go to @NathanSWard for their work on the original PR.

- Currently reflection is not supported for arrays.
- Fixes bevyengine#1213

## Solution

* Implement reflection for arrays via the `Array` trait.
* Note, `Array` is different from `List` in the way that you cannot push elements onto an array as they are statically sized.
* Now `List` is defined as a sub-trait of `Array`.

---

## Changelog

* Added the `Array` reflection trait
* Allows arrays up to length 32 to be reflected via the `Array` trait

## Migration Guide

* The `List` trait now has the `Array` supertrait. This means that `clone_dynamic` will need to specify which version to use:
  ```rust
  // Before
  let cloned = my_list.clone_dynamic();
  // After
  let cloned = List::clone_dynamic(&my_list);
  ```
* All implementers of `List` will now need to implement `Array` (this mostly involves moving the existing methods to the `Array` impl)

Co-authored-by: NathanW <nathansward@comcast.net>
Co-authored-by: MrGVSV <49806985+MrGVSV@users.noreply.github.com>
exjam pushed a commit to exjam/bevy that referenced this issue May 22, 2022
@MrGVSV @NathanSWard @exjam
bevy_reflect: Reflect arrays (bevyengine#4701)
56b2690 
# Objective

> ℹ️ **Note**: This is a rebased version of bevyengine#2383. A large portion of it has not been touched (only a few minor changes) so that any additional discussion may happen here. All credit should go to @NathanSWard for their work on the original PR.

- Currently reflection is not supported for arrays.
- Fixes bevyengine#1213

## Solution

* Implement reflection for arrays via the `Array` trait.
* Note, `Array` is different from `List` in the way that you cannot push elements onto an array as they are statically sized.
* Now `List` is defined as a sub-trait of `Array`.

---

## Changelog

* Added the `Array` reflection trait
* Allows arrays up to length 32 to be reflected via the `Array` trait

## Migration Guide

* The `List` trait now has the `Array` supertrait. This means that `clone_dynamic` will need to specify which version to use:
  ```rust
  // Before
  let cloned = my_list.clone_dynamic();
  // After
  let cloned = List::clone_dynamic(&my_list);
  ```
* All implementers of `List` will now need to implement `Array` (this mostly involves moving the existing methods to the `Array` impl)

Co-authored-by: NathanW <nathansward@comcast.net>
Co-authored-by: MrGVSV <49806985+MrGVSV@users.noreply.github.com>
bors bot pushed a commit that referenced this issue May 30, 2022
@jakobhellermann
remove Serialize impl for dyn Array and friends (#4780)
4b7f904 
# Objective

`bevy_reflect` as different kinds of reflected types (each with their own trait), `trait Struct: Reflect`, `trait List: Reflect`, `trait Map: Reflect`, ...
Types that don't fit either of those are called reflect value types, they are opaque and can't be deconstructed further.

`bevy_reflect` can serialize `dyn Reflect` values. Any container types (struct, list, map) get deconstructed and their elements serialized separately, which can all happen without serde being involved ever (happens [here](https://github.com/bevyengine/bevy/blob/main/crates/bevy_reflect/src/serde/ser.rs#L50-L85=)).
 The only point at which we require types to be serde-serializable is for *value types* (happens [here](https://github.com/bevyengine/bevy/blob/main/crates/bevy_reflect/src/serde/ser.rs#L104=)).

So reflect array serializing is solved, since arrays are container types which don't require serde.

#1213 also introduced added the `serialize` method and `Serialize` impls for `dyn Array` and `DynamicArray` which use their element's `Reflect::serializable` function. This is 1. unnecessary, because it is not used for array serialization, and 2. annoying for removing the `Serialize` bound on container types, because these impls don't have access to the `TypeRegistry`, so we can't move the serialization code there.

# Solution

Remove these impls and `fn serialize`. It's not used and annoying for other changes.
james7132 pushed a commit to james7132/bevy that referenced this issue Jun 7, 2022
@jakobhellermann @james7132
remove Serialize impl for dyn Array and friends (bevyengine#4780)
74deb05 
# Objective

`bevy_reflect` as different kinds of reflected types (each with their own trait), `trait Struct: Reflect`, `trait List: Reflect`, `trait Map: Reflect`, ...
Types that don't fit either of those are called reflect value types, they are opaque and can't be deconstructed further.

`bevy_reflect` can serialize `dyn Reflect` values. Any container types (struct, list, map) get deconstructed and their elements serialized separately, which can all happen without serde being involved ever (happens [here](https://github.com/bevyengine/bevy/blob/main/crates/bevy_reflect/src/serde/ser.rs#L50-L85=)).
 The only point at which we require types to be serde-serializable is for *value types* (happens [here](https://github.com/bevyengine/bevy/blob/main/crates/bevy_reflect/src/serde/ser.rs#L104=)).

So reflect array serializing is solved, since arrays are container types which don't require serde.

bevyengine#1213 also introduced added the `serialize` method and `Serialize` impls for `dyn Array` and `DynamicArray` which use their element's `Reflect::serializable` function. This is 1. unnecessary, because it is not used for array serialization, and 2. annoying for removing the `Serialize` bound on container types, because these impls don't have access to the `TypeRegistry`, so we can't move the serialization code there.

# Solution

Remove these impls and `fn serialize`. It's not used and annoying for other changes.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
@MrGVSV @NathanSWard @ItsDoot
bevy_reflect: Reflect arrays (bevyengine#4701)
92d2d3e 
# Objective

> ℹ️ **Note**: This is a rebased version of bevyengine#2383. A large portion of it has not been touched (only a few minor changes) so that any additional discussion may happen here. All credit should go to @NathanSWard for their work on the original PR.

- Currently reflection is not supported for arrays.
- Fixes bevyengine#1213

## Solution

* Implement reflection for arrays via the `Array` trait.
* Note, `Array` is different from `List` in the way that you cannot push elements onto an array as they are statically sized.
* Now `List` is defined as a sub-trait of `Array`.

---

## Changelog

* Added the `Array` reflection trait
* Allows arrays up to length 32 to be reflected via the `Array` trait

## Migration Guide

* The `List` trait now has the `Array` supertrait. This means that `clone_dynamic` will need to specify which version to use:
  ```rust
  // Before
  let cloned = my_list.clone_dynamic();
  // After
  let cloned = List::clone_dynamic(&my_list);
  ```
* All implementers of `List` will now need to implement `Array` (this mostly involves moving the existing methods to the `Array` impl)

Co-authored-by: NathanW <nathansward@comcast.net>
Co-authored-by: MrGVSV <49806985+MrGVSV@users.noreply.github.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
@jakobhellermann @ItsDoot
remove Serialize impl for dyn Array and friends (bevyengine#4780)
6604558 
# Objective

`bevy_reflect` as different kinds of reflected types (each with their own trait), `trait Struct: Reflect`, `trait List: Reflect`, `trait Map: Reflect`, ...
Types that don't fit either of those are called reflect value types, they are opaque and can't be deconstructed further.

`bevy_reflect` can serialize `dyn Reflect` values. Any container types (struct, list, map) get deconstructed and their elements serialized separately, which can all happen without serde being involved ever (happens [here](https://github.com/bevyengine/bevy/blob/main/crates/bevy_reflect/src/serde/ser.rs#L50-L85=)).
 The only point at which we require types to be serde-serializable is for *value types* (happens [here](https://github.com/bevyengine/bevy/blob/main/crates/bevy_reflect/src/serde/ser.rs#L104=)).

So reflect array serializing is solved, since arrays are container types which don't require serde.

bevyengine#1213 also introduced added the `serialize` method and `Serialize` impls for `dyn Array` and `DynamicArray` which use their element's `Reflect::serializable` function. This is 1. unnecessary, because it is not used for array serialization, and 2. annoying for removing the `Serialize` bound on container types, because these impls don't have access to the `TypeRegistry`, so we can't move the serialization code there.

# Solution

Remove these impls and `fn serialize`. It's not used and annoying for other changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Reflection Runtime information about types C-Feature A new feature, making something new possible
Projects
None yet
Milestone
No milestone
4 participants
@TehPers @alice-i-cecile @Moxinilian @NathanSWard