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

[reflection] impl array reflection #2383

Closed
wants to merge 9 commits into from
Closed

[reflection] impl array reflection #2383

wants to merge 9 commits into from

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-Enhancement A new feature 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
Copy link
Contributor Author

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

@NathanSWard NathanSWard requested a review from bjorn3 June 25, 2021 17:00
Davier
Davier reviewed Jun 25, 2021
View reviewed changes
crates/bevy_reflect/src/impls/std.rs Outdated Show resolved Hide resolved
examples/reflection/reflection_types.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/array.rs Outdated Show resolved Hide resolved
@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

@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

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

@Davier Davier Davier approved these changes

@bjorn3 bjorn3 bjorn3 approved these changes

Assignees
No one assigned
Labels
A-Reflection Runtime information about types C-Enhancement A new feature 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