Skip to content

[reflection] impl array reflection#2383

Closed
NathanSWard wants to merge 9 commits intobevyengine:mainfrom
NathanSWard:nward/array-reflection
Closed

[reflection] impl array reflection#2383
NathanSWard wants to merge 9 commits intobevyengine:mainfrom
NathanSWard:nward/array-reflection

Conversation

@NathanSWard
Copy link
Contributor

@NathanSWard NathanSWard commented Jun 23, 2021 •
edited
Loading

Objective

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.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jun 23, 2021
@NathanSWard NathanSWard added C-Feature A new feature, making something new possible A-Reflection Runtime information about types and removed S-Needs-Triage This issue needs to be labelled labels Jun 23, 2021
@bjorn3
Copy link
Contributor

bjorn3 commented Jun 23, 2021

Can Array be made a super trait of List? List is just a growable Array, right?

@NathanSWard
Copy link
Contributor Author

NathanSWard commented Jun 23, 2021 •
edited
Loading

Can Array be made a super trait of List? List is just a growable Array, right?

Ideally, this was my first thought, however, I thought both traits need to have a unique clone_dynamic function.
However, I guess I could just change these to be clone_dynamic_array and clone_dynamic_list perhaps?

@NathanSWard NathanSWard requested review from Davier and bjorn3 June 23, 2021 21:49
@NathanSWard NathanSWard force-pushed the nward/array-reflection branch from 4e21346 to 076f54e Compare June 25, 2021 06:31
@NathanSWard
Copy link
Contributor Author

NathanSWard commented Jun 25, 2021

Can Array be made a super trait of List? List is just a growable Array, right?

Yep :)
Just did some refactoring to make List: Array

bjorn3
bjorn3 reviewed Jun 25, 2021
View reviewed changes
bjorn3
bjorn3 reviewed Jun 25, 2021
View reviewed changes
@NathanSWard NathanSWard requested a review from bjorn3 June 25, 2021 17:00
Davier
Davier reviewed Jun 25, 2021
View reviewed changes
@NathanSWard NathanSWard requested a review from Davier June 25, 2021 20:50
@NathanSWard NathanSWard added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 25, 2021
@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Apr 25, 2022

@NathanSWard are you able to rebase this? I'm tackling reflection and would like to merge this in.

@alice-i-cecile alice-i-cecile added this to the Bevy 0.8 milestone Apr 25, 2022
@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label May 2, 2022
@alice-i-cecile
Copy link
Member

alice-i-cecile commented May 2, 2022

I just tried rebasing this and it's real gnarly. I think we should probably copy-paste and remake this PR fresh, crediting Nathan appropriately.

@MrGVSV MrGVSV mentioned this pull request May 9, 2022
Closed
@MrGVSV
Copy link
Member

MrGVSV commented May 9, 2022

I just tried rebasing this and it's real gnarly. I think we should probably copy-paste and remake this PR fresh, crediting Nathan appropriately.

@alice-i-cecile I rebased this PR onto main as #4701. If everything there looks okay (and no objections from @NathanSWard), I think we can go ahead and close this one.

bors bot pushed a commit that referenced this pull request May 13, 2022
@MrGVSV @NathanSWard
bevy_reflect: Reflect arrays (#4701)
acbee77 
# Objective

> ℹ️ **Note**: This is a rebased version of #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 #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>
robtfm pushed a commit to robtfm/bevy that referenced this pull request 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 pull request 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>
@MrGVSV MrGVSV mentioned this pull request Jul 28, 2022
Closed
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@Davier Davier Davier approved these changes

@bjorn3 bjorn3 bjorn3 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Reflection Runtime information about types C-Feature A new feature, making something new possible S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Milestone

Bevy 0.8

Development

Successfully merging this pull request may close these issues.

Implement Reflect for arrays

7 participants

@NathanSWard @bjorn3 @alice-i-cecile @MrGVSV @Davier @cart @mockersf