Add reflection support for VecDeque #5792
Conversation
| @@ -217,6 +218,133 @@ impl<T: FromReflect> FromReflect for Vec<T> { | |||
| } | |||
| } | |||
|
|
|||
| impl<T: FromReflect> Array for VecDeque<T> { | |||
There was a problem hiding this comment.
It seems like this implementation is also exactly the same as the Vec<T> impl (minus the Vec/VecDeque type haha).
Since they're basically the exact same, imo, it's worth making a macro therefore we can DRY the code up a bit.
e.g.
macro_rules! vec_like_reflection { .. }
vec_like_reflection!(Vec);
vec_like_reflection!(VecDeque);
There was a problem hiding this comment.
You'd probably need to specify the push method to be used as well
|
Hm, this is an interesting one because it's a slightly different data structure. We sorta lose its usefulness (the queue properties) when reflecting, and we can only really get it back after casting back to its original type (or providing Is it okay that we dumb all these structures down to their base types ( |
| } | ||
|
|
||
| impl<T: FromReflect> List for VecDeque<T> { | ||
| fn push(&mut self, value: Box<dyn Reflect>) { |
There was a problem hiding this comment.
I wonder if List should define a pop method. Then we can use sorta use this structure as intended.
There was a problem hiding this comment.
Agreed, a pop() method would make sense.
Granted though, a normal pop() on a Vec and a VecDeque is basically the same performance wise.
The difference is lies within a pop_front or push_front implementation.
Granted, I don't know if that means we also want to add a Deque trait for example.
Since then the question is, when does adding more and more traits become to cumbersome?
There was a problem hiding this comment.
Yeah that's what I was getting at in my other comment. I don't think it's going to be beneficial to add a new trait for each data structure type. That can probably be served somehow via TypeData.
But a pop method at least let's us handle lists, queues, and stacks in a general way.
alice-i-cecile
added
the
C-Usability
Dumbing down is the "standard" way in my experience. The serde model also hints at that. |
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
bors r+ |
# Objective Fixes #5791 ## Solution Implemented all the required reflection traits for `VecDeque`, taking from `Vec`'s impls.
|
Build failed: |
|
bors retry |
# Objective Fixes #5791 ## Solution Implemented all the required reflection traits for `VecDeque`, taking from `Vec`'s impls.
|
Build failed (retrying...): |
# Objective Fixes #5791 ## Solution Implemented all the required reflection traits for `VecDeque`, taking from `Vec`'s impls.
|
Build failed (retrying...): |
# Objective Fixes #5791 ## Solution Implemented all the required reflection traits for `VecDeque`, taking from `Vec`'s impls.
|
Build failed: |
cart
removed
the
S-Ready-For-Final-Review
rparrett
added
the
S-Adopt-Me
|
This PR has been adopted as #6831. |
# Objective This is an adoption of #5792. Fixes #5791. ## Solution Implemented all the required reflection traits for `VecDeque`, taking from `Vec`'s impls. --- ## Changelog Added: `std::collections::VecDeque` now implements `Reflect` and all relevant traits. Co-authored-by: james7132 <contact@jamessliu.com>
# Objective This is an adoption of bevyengine#5792. Fixes bevyengine#5791. ## Solution Implemented all the required reflection traits for `VecDeque`, taking from `Vec`'s impls. --- ## Changelog Added: `std::collections::VecDeque` now implements `Reflect` and all relevant traits. Co-authored-by: james7132 <contact@jamessliu.com>
# Objective This is an adoption of bevyengine#5792. Fixes bevyengine#5791. ## Solution Implemented all the required reflection traits for `VecDeque`, taking from `Vec`'s impls. --- ## Changelog Added: `std::collections::VecDeque` now implements `Reflect` and all relevant traits. Co-authored-by: james7132 <contact@jamessliu.com>
Objective
Fixes #5791
Solution
Implemented all the required reflection traits for
VecDeque, taking fromVec's impls.