Feat/5764 map paths in bevy reflect #5812
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These methods use the same code (duplicated) and we should make sure they behave the same (modulo _mut). This also found a minor "bug" where an error was reported different between the implementations. We may want to derive the two tests with some kind of macro to make sure they stay the same?
This adds some parser code to be able to parse access to maps via the reflection get_path and get_path_mut API. The current implementation only supports maps with String keys, dues to lack of type casting from the read ident to the map key. Needs some more logic there.
Weibye
added
C-Usability
NathanSWard
suggested changes
Aug 27, 2022
crates/bevy_reflect/src/path.rs
Outdated
Comment on lines
155
to
161
| if let Some(Token::SingleQuote) = next_token(path, &mut index) { | ||
| } else { | ||
| return Err(ReflectPathError::ExpectedToken { | ||
| index: current_index, | ||
| token: "'", | ||
| }); | ||
| } |
There was a problem hiding this comment.
an if/else branch with one of the branches being empty is a little odd. This could be condensed to
if !matches!(Some(Token::SingleQuote), next_token(path, &mut index)) {
return Err(..);
}There was a problem hiding this comment.
You are correct, that read better indeed. I just used the empty if-branch because it was already used further down in the function to check for the CloseBracked token. I changed all cases to if !matched! now to keep them consistent
crates/bevy_reflect/src/path.rs
Outdated
Comment on lines
263
to
269
| if let Some(Token::SingleQuote) = next_token(path, &mut index) { | ||
| } else { | ||
| return Err(ReflectPathError::ExpectedToken { | ||
| index: current_index, | ||
| token: "'", | ||
| }); | ||
| } |
There was a problem hiding this comment.
ditto here with the if !matches!(..) syntax.
| @@ -136,12 +141,48 @@ impl GetPath for dyn Reflect { | |||
| index: current_index, | |||
| }) | |||
| } | |||
| }, | |||
| Some(Token::SingleQuote) => { | |||
There was a problem hiding this comment.
Also notably, the two match branches in path() and path_mut() are exactly the same (minus the get/get_mut call on the map. It's probably worth DRY'ing this out into a similar function to reduce code duplication.
There was a problem hiding this comment.
I moved the code to find the complete quoted identifier to its own function, which only leaves the match reflect_ref vs match reflect_mut part in those branches. Not sure if it possible/feasible to further reduce duplication here without creating some kind ob abstraction over mutability?
Some parts could easily be factored our to reduce code duplication. Also improved readability by cleaning up empty if-branches, as suggested by QA.
We want to report the correct error locations so users understand these errors correctly.
Thanks clippy!
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.
|
Closing this as I cannot finish it right now. |
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
I aimed to solve #5764 here, implementing a way to access map entries using bevy_reflects
get_pathandget_path_mut.Solution
The implementation is incomplete, only allowing to access entries in maps with
Strings as key. I wasnt able to come up with a proper solution yet on how to get from having a&str identto aK map_keyfor aMap<K, V>.The current implementation uses the
m['key']syntax for specifiing the map key, but it should be easy to adapt it to a different syntax, if required.I added a new
SingleQuoteTokenvariant, which is returned bynext_tokenwhen it encounters a'.Then extended the
get_pathandget_path_mutmethods to support aSingleQuotetoken after aOpenBrackettoken instead of anIdent. If thats encountered, we expect the next token to be anIdent(the key) and then a closingSingleQuote, and we expect thecurrentreflect node to be aMap.Resolving the
map_keyfrom the readidentseems to only challenging part here and thus its the only part I was not able to solve (or I might have missed other stuff?). I opted for going for "String keys only for the first draft", but of course I do not expect this to be merged like this.Everything after that is basically a mirror of the behaviour for lists.
Additionally I split/copied the test for get_path into a second one for get_path_mut which basically tests the exact same stuff but using the
_mutvariant to make sure both behave the same, since their code is roughly identical and seems prone to minor differences which may cause subtle bugs/inconsistencies.Changelog
TODO
I plan on doing some experiments on how the
&str ident => K map_keyresolution could work, but if anyone got a good idea on how this can be done, I'd very happy to read that!