reflect: decouple Array from List
#7121
Labels
A-Reflection
Runtime information about types
C-Usability
A targeted quality-of-life change that makes Bevy easier to use
Comments
soqb
added
C-Feature
|
Could you link to, reference, or copy/paste some of the additional details I mentioned in this comment? I think it includes some important reasoning for making this kind of change. Edit: I suppose the existence of this comment works as well haha |
MrGVSV
added
C-Usability
myreprise1
pushed a commit
to myreprise1/bevy
that referenced
this issue
Feb 14, 2023
Resolves bevyengine#7121 Decouples `List` and `Array` by removing `Array` as a supertrait of `List`. Additionally, similar methods from `Array` have been added to `List` so that their usages can remain largely unchanged. My guess for why we originally made `List` a subtrait of `Array` is that they share a lot of common operations. We could potentially move these overlapping methods to a `Sequence` (name taken from bevyengine#7059) trait and make that a supertrait of both. This would allow functions to contain logic that simply operates on a sequence rather than "list vs array". However, this means that we'd need to add methods for converting to a `dyn Sequence`. It also might be confusing since we wouldn't add a `ReflectRef::Sequence` or anything like that. Is such a trait worth adding (either in this PR or a followup one)? --- - Removed `Array` as supertrait of `List` - Added methods to `List` that were previously provided by `Array` The `List` trait is no longer dependent on `Array`. Implementors of `List` can remove the `Array` impl and move its methods into the `List` impl (with only a couple tweaks). ```rust // BEFORE impl Array for Foo { fn get(&self, index: usize) -> Option<&dyn Reflect> {/* ... */} fn get_mut(&mut self, index: usize) -> Option<&mut dyn Reflect> {/* ... */} fn len(&self) -> usize {/* ... */} fn is_empty(&self) -> bool {/* ... */} fn iter(&self) -> ArrayIter {/* ... */} fn drain(self: Box<Self>) -> Vec<Box<dyn Reflect>> {/* ... */} fn clone_dynamic(&self) -> DynamicArray {/* ... */} } impl List for Foo { fn insert(&mut self, index: usize, element: Box<dyn Reflect>) {/* ... */} fn remove(&mut self, index: usize) -> Box<dyn Reflect> {/* ... */} fn push(&mut self, value: Box<dyn Reflect>) {/* ... */} fn pop(&mut self) -> Option<Box<dyn Reflect>> {/* ... */} fn clone_dynamic(&self) -> DynamicList {/* ... */} } // AFTER impl List for Foo { fn get(&self, index: usize) -> Option<&dyn Reflect> {/* ... */} fn get_mut(&mut self, index: usize) -> Option<&mut dyn Reflect> {/* ... */} fn insert(&mut self, index: usize, element: Box<dyn Reflect>) {/* ... */} fn remove(&mut self, index: usize) -> Box<dyn Reflect> {/* ... */} fn push(&mut self, value: Box<dyn Reflect>) {/* ... */} fn pop(&mut self) -> Option<Box<dyn Reflect>> {/* ... */} fn len(&self) -> usize {/* ... */} fn is_empty(&self) -> bool {/* ... */} fn iter(&self) -> ListIter {/* ... */} fn drain(self: Box<Self>) -> Vec<Box<dyn Reflect>> {/* ... */} fn clone_dynamic(&self) -> DynamicList {/* ... */} } ``` Some other small tweaks that will need to be made include: - Use `ListIter` for `List::iter` instead of `ArrayIter` (the return type from `Array::iter`) - Replace `array_hash` with `list_hash` in `Reflect::reflect_hash` for implementors of `List`
myreprise1
pushed a commit
to myreprise1/bevy
that referenced
this issue
Feb 15, 2023
Resolves bevyengine#7121 Decouples `List` and `Array` by removing `Array` as a supertrait of `List`. Additionally, similar methods from `Array` have been added to `List` so that their usages can remain largely unchanged. My guess for why we originally made `List` a subtrait of `Array` is that they share a lot of common operations. We could potentially move these overlapping methods to a `Sequence` (name taken from bevyengine#7059) trait and make that a supertrait of both. This would allow functions to contain logic that simply operates on a sequence rather than "list vs array". However, this means that we'd need to add methods for converting to a `dyn Sequence`. It also might be confusing since we wouldn't add a `ReflectRef::Sequence` or anything like that. Is such a trait worth adding (either in this PR or a followup one)? --- - Removed `Array` as supertrait of `List` - Added methods to `List` that were previously provided by `Array` The `List` trait is no longer dependent on `Array`. Implementors of `List` can remove the `Array` impl and move its methods into the `List` impl (with only a couple tweaks). ```rust // BEFORE impl Array for Foo { fn get(&self, index: usize) -> Option<&dyn Reflect> {/* ... */} fn get_mut(&mut self, index: usize) -> Option<&mut dyn Reflect> {/* ... */} fn len(&self) -> usize {/* ... */} fn is_empty(&self) -> bool {/* ... */} fn iter(&self) -> ArrayIter {/* ... */} fn drain(self: Box<Self>) -> Vec<Box<dyn Reflect>> {/* ... */} fn clone_dynamic(&self) -> DynamicArray {/* ... */} } impl List for Foo { fn insert(&mut self, index: usize, element: Box<dyn Reflect>) {/* ... */} fn remove(&mut self, index: usize) -> Box<dyn Reflect> {/* ... */} fn push(&mut self, value: Box<dyn Reflect>) {/* ... */} fn pop(&mut self) -> Option<Box<dyn Reflect>> {/* ... */} fn clone_dynamic(&self) -> DynamicList {/* ... */} } // AFTER impl List for Foo { fn get(&self, index: usize) -> Option<&dyn Reflect> {/* ... */} fn get_mut(&mut self, index: usize) -> Option<&mut dyn Reflect> {/* ... */} fn insert(&mut self, index: usize, element: Box<dyn Reflect>) {/* ... */} fn remove(&mut self, index: usize) -> Box<dyn Reflect> {/* ... */} fn push(&mut self, value: Box<dyn Reflect>) {/* ... */} fn pop(&mut self) -> Option<Box<dyn Reflect>> {/* ... */} fn len(&self) -> usize {/* ... */} fn is_empty(&self) -> bool {/* ... */} fn iter(&self) -> ListIter {/* ... */} fn drain(self: Box<Self>) -> Vec<Box<dyn Reflect>> {/* ... */} fn clone_dynamic(&self) -> DynamicList {/* ... */} } ``` Some other small tweaks that will need to be made include: - Use `ListIter` for `List::iter` instead of `ArrayIter` (the return type from `Array::iter`) - Replace `array_hash` with `list_hash` in `Reflect::reflect_hash` for implementors of `List`
myreprise1
pushed a commit
to myreprise1/bevy
that referenced
this issue
Feb 15, 2023
Resolves bevyengine#7121 Decouples `List` and `Array` by removing `Array` as a supertrait of `List`. Additionally, similar methods from `Array` have been added to `List` so that their usages can remain largely unchanged. My guess for why we originally made `List` a subtrait of `Array` is that they share a lot of common operations. We could potentially move these overlapping methods to a `Sequence` (name taken from bevyengine#7059) trait and make that a supertrait of both. This would allow functions to contain logic that simply operates on a sequence rather than "list vs array". However, this means that we'd need to add methods for converting to a `dyn Sequence`. It also might be confusing since we wouldn't add a `ReflectRef::Sequence` or anything like that. Is such a trait worth adding (either in this PR or a followup one)? --- - Removed `Array` as supertrait of `List` - Added methods to `List` that were previously provided by `Array` The `List` trait is no longer dependent on `Array`. Implementors of `List` can remove the `Array` impl and move its methods into the `List` impl (with only a couple tweaks). ```rust // BEFORE impl Array for Foo { fn get(&self, index: usize) -> Option<&dyn Reflect> {/* ... */} fn get_mut(&mut self, index: usize) -> Option<&mut dyn Reflect> {/* ... */} fn len(&self) -> usize {/* ... */} fn is_empty(&self) -> bool {/* ... */} fn iter(&self) -> ArrayIter {/* ... */} fn drain(self: Box<Self>) -> Vec<Box<dyn Reflect>> {/* ... */} fn clone_dynamic(&self) -> DynamicArray {/* ... */} } impl List for Foo { fn insert(&mut self, index: usize, element: Box<dyn Reflect>) {/* ... */} fn remove(&mut self, index: usize) -> Box<dyn Reflect> {/* ... */} fn push(&mut self, value: Box<dyn Reflect>) {/* ... */} fn pop(&mut self) -> Option<Box<dyn Reflect>> {/* ... */} fn clone_dynamic(&self) -> DynamicList {/* ... */} } // AFTER impl List for Foo { fn get(&self, index: usize) -> Option<&dyn Reflect> {/* ... */} fn get_mut(&mut self, index: usize) -> Option<&mut dyn Reflect> {/* ... */} fn insert(&mut self, index: usize, element: Box<dyn Reflect>) {/* ... */} fn remove(&mut self, index: usize) -> Box<dyn Reflect> {/* ... */} fn push(&mut self, value: Box<dyn Reflect>) {/* ... */} fn pop(&mut self) -> Option<Box<dyn Reflect>> {/* ... */} fn len(&self) -> usize {/* ... */} fn is_empty(&self) -> bool {/* ... */} fn iter(&self) -> ListIter {/* ... */} fn drain(self: Box<Self>) -> Vec<Box<dyn Reflect>> {/* ... */} fn clone_dynamic(&self) -> DynamicList {/* ... */} } ``` Some other small tweaks that will need to be made include: - Use `ListIter` for `List::iter` instead of `ArrayIter` (the return type from `Array::iter`) - Replace `array_hash` with `list_hash` in `Reflect::reflect_hash` for implementors of `List`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What problem does this solve or what need does it fill?
Successor to #7059.
TLDR; lists should not be arrays, the nomenclature is confusing.
Additionally, the supertrait relationship is awkward and the only example within the current reflection typing. Separating them keeps our reflection trait tree flatter and therefore simpler to understand. The
clone_dynamiccollision also adds to the confusion.What solution would you like?
Remove the
Arraysupertrait onList. Redeclare the methods onArray(get,iter,lenetc.) ontoList.What alternative(s) have you considered?
Rename the
Arraytrait as proposed in #7059.Additional context
Idea proposed by @MrGVSV
Rename advised against in #7110.
The text was updated successfully, but these errors were encountered: