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

[Merged by Bors] - Make arrays behave like lists in reflection #5987

Closed

Conversation

themasch
Copy link
Contributor

Objective

Currently, arrays cannot indexed using the reflection path API.
This change makes them behave like lists so x.get_path("list[0]") will behave the same way, whether x.list is a "List" (e.g. a Vec) or an array.

Solution

When syntax is encounterd [ <idx> ] we check if the referenced type is either a ReflectRef::List or ReflectRef::Array (or ReflectMut for the mutable case). Since both provide the identical API for accessing entries, we do the same for both, although it requires code duplication as far as I can tell.

This was born from working on #5764, but since this seems to be an easier fix (and I am not sure if I can actually solve #5812) I figured it might be worth to split this out.

Comment on lines 135 to 142
let list_index = value.parse::<usize>()?;
let list_item = reflect_arr.get(list_index).ok_or(
ReflectPathError::InvalidListIndex {
index: current_index,
list_index,
},
)?;
current = list_item;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realistically, since List is a subtrait of Array, you should be able to extract this logic out and use it both lists and arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, I missed that. Will rework.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not able to combin the match branches themself, but move the inner logic in a function that accepts a &T where T: Array, which seems to work fine.

Is there a smarter way to do this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope that's how I'd do it!

Although, I’d actually use &(mut) dyn Array rather than T since we don't really have a true concrete type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that, but as far as I understand, we'd need feature(trait_upcasting) for that, right?
At least when I try it with this very simple example it fails in the same way: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9a17c93739790d86fd0f57a1452140c3

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right haha then ignore what I suggested!

@alice-i-cecile alice-i-cecile added A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Sep 14, 2022
@themasch themasch force-pushed the feat/reflection-arrays-and-lists branch from f0ccb66 to 7cfa74e Compare September 14, 2022 18:35
@rparrett rparrett 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 Sep 23, 2022
@cart
Copy link
Member

cart commented Sep 27, 2022

bors r+

bors bot pushed a commit that referenced this pull request Sep 27, 2022
# Objective

Currently, arrays cannot indexed using the reflection path API. 
This change makes them behave like lists so `x.get_path("list[0]")` will behave the same way, whether x.list is a "List" (e.g. a Vec) or an array.

## Solution

When syntax is encounterd `[ <idx> ]` we check if the referenced type is either a `ReflectRef::List` or `ReflectRef::Array`   (or `ReflectMut` for the mutable case). Since both provide the identical API for accessing entries, we do the same for both, although it requires code duplication as far as I can tell. 


This was born from working on #5764, but since this seems to be an easier fix (and I am not sure if I can actually solve #5812) I figured it might be worth to split this out.
@bors
Copy link
Contributor

bors bot commented Sep 27, 2022

@bors bors bot changed the title Make arrays behave like lists in reflection [Merged by Bors] - Make arrays behave like lists in reflection Sep 27, 2022
@bors bors bot closed this Sep 27, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 19, 2022
# Objective

Currently, arrays cannot indexed using the reflection path API. 
This change makes them behave like lists so `x.get_path("list[0]")` will behave the same way, whether x.list is a "List" (e.g. a Vec) or an array.

## Solution

When syntax is encounterd `[ <idx> ]` we check if the referenced type is either a `ReflectRef::List` or `ReflectRef::Array`   (or `ReflectMut` for the mutable case). Since both provide the identical API for accessing entries, we do the same for both, although it requires code duplication as far as I can tell. 


This was born from working on bevyengine#5764, but since this seems to be an easier fix (and I am not sure if I can actually solve bevyengine#5812) I figured it might be worth to split this out.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

Currently, arrays cannot indexed using the reflection path API. 
This change makes them behave like lists so `x.get_path("list[0]")` will behave the same way, whether x.list is a "List" (e.g. a Vec) or an array.

## Solution

When syntax is encounterd `[ <idx> ]` we check if the referenced type is either a `ReflectRef::List` or `ReflectRef::Array`   (or `ReflectMut` for the mutable case). Since both provide the identical API for accessing entries, we do the same for both, although it requires code duplication as far as I can tell. 


This was born from working on bevyengine#5764, but since this seems to be an easier fix (and I am not sure if I can actually solve bevyengine#5812) I figured it might be worth to split this out.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Currently, arrays cannot indexed using the reflection path API. 
This change makes them behave like lists so `x.get_path("list[0]")` will behave the same way, whether x.list is a "List" (e.g. a Vec) or an array.

## Solution

When syntax is encounterd `[ <idx> ]` we check if the referenced type is either a `ReflectRef::List` or `ReflectRef::Array`   (or `ReflectMut` for the mutable case). Since both provide the identical API for accessing entries, we do the same for both, although it requires code duplication as far as I can tell. 


This was born from working on bevyengine#5764, but since this seems to be an easier fix (and I am not sure if I can actually solve bevyengine#5812) I figured it might be worth to split this out.
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 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
Development

Successfully merging this pull request may close these issues.

6 participants