[Merged by Bors] - Add reflection support for VecDeque

[james7132]

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.

[MrGVSV]

LGTM!

One thing to note, though, is that the List::pop doc says this:

bevy/crates/bevy_reflect/src/list.rs

Lines 18 to 19 in 6ada356

	/// Removes the last element from the list (highest index in the array) and returns it, or [`None`] if it is empty.
	fn pop(&mut self) -> Option<Box<dyn Reflect>>;

This was never something we necessarily could guarantee, but maybe we should update this as part of this PR since we're adding built-in support for VecDeque (where push/pop is FIFO). Perhaps something that better indicates that the direction (or lack thereof) from which an item is pushed/popped is determined by the implementor rather than by bevy_reflect.

I don't think we need to block on this if you'd rather that be a separate thing we just make an issue for, though. Just wanted to address it now.

[cart]

This was never something we necessarily could guarantee, but maybe we should update this as part of this PR since we're adding built-in support for VecDeque (where push/pop is FIFO). Perhaps something that better indicates that the direction (or lack thereof) from which an item is pushed/popped is determined by the implementor rather than by bevy_reflect.

For these "abstract collection types" I think we need to provide a consistent mental model of the type (ex: index 0 always points to the "first" item, pushing always add to the "back", popping always removes from the "back"). Otherwise the methods are useless, as you can't safely implement algorithms on top of them. If we can't provide consistent behaviors for a given type, we either need to consider not implementing List for that type, or removing the inconsistent behavior to properly generalize the List abstraction.

In this case, I think VecDeque should push to the back and pop from the back, which means we should change the impl in this PR.

I don't think we need to block on this if you'd rather that be a separate thing we just make an issue for, though. Just wanted to address it now.

I think we should block on this, as merging as-is violates the currently defined contract (and fundamentally changes what I think should be a core nature of these abstractions).

[MrGVSV]

In this case, I think VecDeque should push to the back and pop from the back, which means we should change the impl in this PR.

If this is the case, then we should probably also update the docs to better explain this. I'll make a separate issue for that, though.

I think I generally agree with this direction. Ideally, it'd be nice to support it but yeah that does make it confusing for places where the order of insertion/removal is important. And it's probably more likely that these are reflected for serialization anyways as opposed to using them as VecDeques.

And like mentioned in another PR, this functionality can probably be encapsulated in registered type data.

[james7132]

@cart @MrGVSV should be addressed now. #6872 addresses the docs disparity.

[alice-i-cecile]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-and-install-on-iOS
• build-android
• build (macos-latest)
• build (ubuntu-latest)
• build-wasm
• build (windows-latest)
• build-without-default-features (bevy)
• build-without-default-features (bevy_ecs)
• build-without-default-features (bevy_reflect)
• check-compiles
• check-doc
• check-missing-examples-in-docs
• ci
• markdownlint
• run-examples
• run-examples-on-wasm
• run-examples-on-windows-dx12