Packed Arrays could use some more trait impls

[lilizoey]

For instance, you cannot currently do

let v = vec![Vector3::new(1.0, 2.0, 3.0)];
let packed: PackedVector3Array = v.into();

This could be fixed with a From<Vec<Vector3>> impl for PackedVector3Array.

In general since packed arrays are very similar to Vec we could also take inspiration from what Vec has implemented.

See discussion below for more details.

Traits to implement:

• From<Vec<T>>
• From<[T; N]>
• From<Array<T>>
• From<PackedTArray> for Array<T>
• Index and IndexMut, i think we can use I: SliceIndex like Vec does.
• IntoIterator
• Write, for PackedByteArray only but preferably have a use-case first

Other trait impls we can consider:

• Deref and DerefMut to [T]. may conflict with Godot's api for packed arrays
• From<Box<[T]>>, From<VecDeque<T>>. We can already do this with collect()
• AsRef<[T]>, AsMut<[T]>, Borrow<[T]>, BorrowMut<[T]>. Should have a use-case first

Additionally we apparently have From<&VariantArray> for PackedVector3Array etc, which is wrong. (looking at the implementation this might actually be UB).

[Bromeon]

Definitely agree it makes sense to provide easier conversions between Packed*Array and Vec or slices. Thanks for the list!

As mentioned in #297, we should make sure these satisfy common use cases. Otherwise, with this being marked good first issue, the risk is that newcomers just start working off the list of traits.

Some things to keep in mind:

1. Deref/DerefMut is mostly convenience for as_slice()/as_slice_mut(). However, since packed arrays have their own API inherited from Godot, it may actually be worth to keep this explicit. There are some overlapping methods like binary_search or contains which are using different implementations.
2. From<Vec> may be more explicit as from_vec constructor
3. From<Array> could also be a method Array::to_packed() or Packed*Array::from_array()
4. From<Box<[T]>> and From<VecDeque<T>> are extremely niche, possibly better via collect()
5. std::io::Write would also need a concrete use case. Since they operate on [u8], it may only really be useful for PackedByteArray
6. Index/IndexMut definitely make sense
7. AsRef/Borrow need use cases

---

Additionally we apparently have From<&VariantArray> for PackedVector3Array etc, which is wrong. (looking at the implementation this might actually be UB).

Interesting, we should remove that. Even without looking at the implementation, From should be infallible, which this can't be.

[lilizoey]

I'll update the issue to reflect some of these comments.

1. Deref/DerefMut, that makes sense.

I think From<Vec> would be good to have even with a from_vec constructor because you can then do vec![a,b,c].into(). In general i think From<X> methods are nice to have as long as it's appropriate and there isn't a good reason not to have it, such as it being surprisingly expensive or something one should be careful about using for some reason. But i dont think there are any particular reasons to not have From<Vec> or From<Array> here.

Having from_vec, to_packed and from_array additionally could make sense, but if i had to choose between those and From impls i'd prefer the From impls.

4. That also makes sense to me, Box<[T]> and VecDeque<T> both have FromIterator<Item = T> impls.
5. If someone wants to implement Write for PackedByteArray that'd probably be fine imo. I can imagine it being useful. But i do agree it'd be better for there to be an actual use-case first.

[andreymal]

I want AsRef<[u8]> for PackedByteArray to store it as a reader in some Rust code, something like:

pub struct SomeSmartParser {
    input: std::io::Cursor<PackedByteArray>,
}

In real code, this would probably be a generic (or maybe Box<dyn ReadSeek> depending on use case):

pub struct SomeSmartParser<R: std::io::Read + std::io::Seek> {
    input: R,
}
let mut s = SomeSmartParser { input: std::io::Cursor::new(packed_byte_array) };

It's already possible to use packed_byte_array.as_slice(), but this raises the question of where to store packed_byte_array itself _{(self-referential structs boo👻)}

[Bromeon]

From for Rust arrays and Vec was implemented in #827.

I added Index/IndexMut in #725, currently for usize. If someone feels like extending it for SliceIndex, please go ahead.

---

@andreymal if we added AsRef, this would enable Read + Seek traits on Cursor<Packed*Array>, right?

What about Write? The docs only mention impl Write for Cursor<&mut [u8]>, and not for Cursor<T: AsMut<[u8]>>, so can we even enable this trait? The initial docs say it's possible, but how?

Cursors are used with in-memory buffers, anything implementing AsRef<[u8]>, to allow them to implement Read and/or Write, allowing these buffers to be used anywhere you might use a reader or writer that does actual I/O.

If not, that's a bit asymmetric -- should we then consider implementing Read + Seek + Write on the container directly?

[andreymal]

should we then consider implementing Read + Seek ... on the container directly?

You can't because you need to store the cursor position somewhere

(but the Write implementation can be stolen from Vec<u8> which simply calls extend_from_slice so it doesn't need a cursor)

[andreymal]

Regarding AsMut, rust-lang/rust#92663 (comment) mentions that this implies a fixed-size buffer, which is not good

An alternative is implementing our own pub struct PackedByteArrayCursor { inner: PackedByteArray, pos: u64, } but not sure if this is a good idea

[Bromeon]

(but the Write implementation can be stolen from Vec<u8> which simply calls extend_from_slice so it doesn't need a cursor)

But that implies writing can only be done by having an initially empty buffer and extending it? This will be inefficient due to reallocations, especially since packed arrays have no reserve() function...

It also limits writing to the end of the array.

---

An alternative is implementing our own pub struct PackedByteArrayCursor { inner: PackedByteArray, pos: u64, } but not sure if this is a good idea

Hm yeah, maybe that's necessary. There's really no way to use standard cursors for writing custom data structures? What does this doc mean then?

Cursors are used with in-memory buffers, anything implementing AsRef<[u8]>, to allow them to implement Read and/or Write, allowing these buffers to be used anywhere you might use a reader or writer that does actual I/O.

[andreymal]

What does this doc mean then?

impl Write for Cursor<...> is implemented for &mut [u8], Vec<u8> and Box<[u8]>, all these types implement AsRef<[u8]>, maybe this is what this doc mean (or maybe this is just a mistake made in rust-lang/rust#52548, the old doc didn't mention AsRef<[u8]>)

(Anyway, I personally don't care about Write, I just want a reader)

[Bromeon]

OK, so I suggest we can provide AsRef<[u8]>, with docs to indicate std::io::Cursor use case.
However, AsMut<[u8]> doesn't seem to be needed, as we also have as_mut_slice() for regular slice writing...

[Bromeon]

@andreymal what do you think? Would you be interested in a PR? 🙂

[andreymal]

Not yet, I'm too busy currently

[Bromeon]

Many of the traits above have been implemented, and there don't seem to be demand for more at the moment.