rename bevy_reflect's Array
#7059
Labels
A-Reflection
Runtime information about types
C-Usability
A targeted quality-of-life change that makes Bevy easier to use
S-Ready-For-Implementation
This issue is ready for an implementation PR. Go for it!
Comments
soqb
added
C-Feature
alice-i-cecile
added
C-Usability
|
I'm on board with this rename. |
MrGVSV
added
the
D-Trivial
|
perhaps also |
|
IMO sequence > indexable >> elements. |
|
Just wanna make sure I'm on the right track before I submit anything since I'm new around here (Hi! 😃). The main affected files are If this is correct I would like to tackle this change as my first submit. If not, could you please point me in the right direction? |
|
@stefanmitic this seems correct: feel free to make the PR and we'll verify there! |
|
Thinking on this some more, I wanted to point out another option could be to separate I think the only reason they're connected is because they share similar methods and logic. However, because of this we have a few issues:
If we separated them then most of these issues go away. Consumers of Additionally, it might make future work simpler since we no longer need to worry about possible complexities around subtraits of subtraits (if any). The only downside (at least the only one I can think of right now) is that we can no longer pass data around as both And if we really want to, we could add a So separating them could be a potential solution here. Any thoughts on this? |
|
This is a compelling argument: by simplifying the trait hierarchy we can clearly seperate the two different classes of data structure. I think this is more intuitive and gets rid of complexity that doesn't seem to be pulling its weight. |
|
i like this course of action. i'll leave this issue open for now until one suggesting the split opens up (if not i'll make one myself). |
alice-i-cecile
added
S-Ready-For-Implementation
|
closed in favour of #7121. |
bors bot
pushed a commit
that referenced
this issue
Feb 13, 2023
# Objective Resolves #7121 ## Solution 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. #### Possible Alternatives ##### `Sequence` 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 #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)? --- ## Changelog - Removed `Array` as supertrait of `List` - Added methods to `List` that were previously provided by `Array` ## Migration Guide 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 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?
The term "array" in Rust is used most commonly to describe a linear array defined by Rust's array syntax (e.g.
[u8; 4]) which has a strict, stable in-memory layout. Less frequently and more conceptually, the term may also describe a span of homogeneously-sized elements (typically bytes) in memory ({ start: *const T, end: *const T }is occasionally thought of as an array).We have a data type in
bevy_reflect(i.e. a variant ofReflectRefwith a matching trait providing core functionality) calledArray. Importantly, theArraytrait is a supertrait ofList.Although the trait provides the same core functionality as a Rust array (it has a fixed-length and access-by-index), the name "array" is confusing:
Arrayare not necessarily fixed-length as arrays are. In fact, all types implementingListhave variable length.Arrayare not necessarily backed by an array (by either definition) either and this could lead to dissonance trying to understand the relationship betweenbevy_reflectand the type.Vecis backed by the second definition of array described above but this is an implementation detail subject (although unlikely) to change.LinkedLists are spatially disjoint, heap-allocated nodes and definitely not arrays except in the most general possible case.VecDequeis perhaps the worst offender, implemented IIRC similarly toVecbut where the first element of the underlying array is not necessarily the first element in the deque.What solution would you like?
Rename the
Arraytrait,DynamicArray, and theArrayvariant ofReflectRef,ReflectMutandReflectOwnedas well as any conceptual docs using the term "array" in this context.A name I proposed was
Sequenceinspired by serde's primitive data type of the same name, although the word is rarely used in my experience and serde's sequence does not have to provide a length during serialisation (similar to theIteratortrait).What alternative(s) have you considered?
Leave name as is. Potential confusion may be mitigated by clear docs.
Additional context
Originally briefly discussed with @MrGVSV on discord.
The text was updated successfully, but these errors were encountered: