Skip to content

Converting TypedArray to Vec fails unexpectedly #806

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bluenote10 opened this issue Oct 30, 2021 · 5 comments · Fixed by #843
Closed

Converting TypedArray to Vec fails unexpectedly #806

bluenote10 opened this issue Oct 30, 2021 · 5 comments · Fixed by #843
Labels
c: core Component: core (mod core_types, object, log, init, ...) quality-of-life No new functionality, but improves ergonomics/internals
Milestone

Comments

@bluenote10
Copy link
Contributor

There is a high likelihood that I'm simply doing something wrong (perhaps related to #774), but it still feels like a potential bug.

Basically my goal is to convert a TypedArray<GodotString> to Vec<String>. After studying potentially relevant trait implementations, it occurred to me that I'll have to take the detour through Variant, because the ToVariant is implemented for TypedArray and FromVariant is implemented for Vec:

fn test_conversion(typed_array: TypedArray<GodotString>) {
    let _vec = Vec::<GodotString>::from_variant(&typed_array.to_variant()).unwrap();
}

However this panics with:

thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: InvalidVariantType { variant_type: StringArray, expected: VariantArray }', src/native_plot.rs:66:76

Shouldn't a StringArray be convertible as well, in particular because homogeneity is guaranteed?


Side note: For now I'm using this work-around

fn test_conversion(typed_array: TypedArray<GodotString>) {
    let tmp = typed_array.read();
    let _vec: Vec<_> = tmp.iter().collect();
}

which feels a bit clumsy, because the read() has an unusual name and the mandatory temporary avoids doing it in one expression I guess.

Since the conversion from Vec to TypedArray is well documented here, it would be great to give a short hint about the reverse direction as well. Or alternatively somewhere here in the book.

@Bromeon
Copy link
Member

Bromeon commented Oct 30, 2021

In GDScript, there are two ways of representing arrays of strings:

  • PoolStringArray -- this is TypedArray<GodotString> in Rust
  • Array -- this is VariantArray in Rust, with dynamically typed elements

The two are not interchangeable, which is why you get the error when you try to extract a VariantArray from a Variant, which actually holds a PoolStringArray.

So this is not a bug, but I agree with your observation that conversion between the two should be easier. There is currently quite a bit of confusion about different types and their convertibility in the godot-rust API. Part of it has to do with documentation, but an important part is also naming, which we're trying to address in #773.

Regarding this issue, since we already have TypedArray::from_vec(), it might be a natural extension to provide a to_vec() or into_vec() method (we need to consider CoW semantics here). I'm also open to other suggestions.

@Bromeon Bromeon added c: core Component: core (mod core_types, object, log, init, ...) feature Adds functionality to the library labels Oct 30, 2021
@bluenote10
Copy link
Contributor Author

The idea with to_vec() and into_vec() sounds great!

which is why you get the error when you try to extract a VariantArray from a Variant, which actually holds a PoolStringArray

But am I trying to extract a VariantArray? I guess what confuses me is that the conversion is using

  • Vec::<GodotString>::from_variant(&some_variant) and not
  • Vec::<Variant>::from_variant(&some_variant).

In the latter case it is logical that the conversion tries to extract a dynamically-typed Array. However, in the former case, the underlying type is GodotString, so it is surprising that it still tries to extract a dynamically-typed Array and not the properly typed PoolStringArray, right?

@Bromeon Bromeon added quality-of-life No new functionality, but improves ergonomics/internals and removed feature Adds functionality to the library labels Oct 30, 2021
@Bromeon Bromeon modified the milestones: v0.11, v0.10.1 Nov 1, 2021
@chitoyuu
Copy link
Contributor

chitoyuu commented Nov 23, 2021

Having a separate code path in Vec<T> as FromVariant for TypedArray element types would require (full) specialization, since the implementation for Vec<T> is generic.

To convert a TypedArray<GodotString> to a Vec<String> you might have to go through an iterator and collect for now.

@bluenote10
Copy link
Contributor Author

Having a separate code path in Vec as FromVariant for TypedArray element types would require (full) specialization, since the implementation for Vec is generic.

To make sure I understand correctly: By full specializations you mean the 7 specializations for PoolColorArray, PoolVector2Array, PoolVector3Array, PoolStringArray, PoolRealArray, PoolIntArray, PoolByteArray, or are there more specializations needed? If it is limited to these seven by the nature of GDScript it doesn't sound too bad, but I may be underestimating the complexity.

To convert a TypedArray to a Vec you might have to go through an iterator and collect for now.

As long as it is documented somewhere (and perhaps we can simplify it so that the temporary isn't needed) that would be fine as well. From a user perspective it is just slightly weird that it is harder to convert a TypedArray to a Vec than an untyped one, considering that "typed" makes it homogeneous, i.e., it's conceptionally already closer to a Vec<T>.

@chitoyuu
Copy link
Contributor

chitoyuu commented Nov 23, 2021

By "full specialization" I mean #[feature(specialization)], which is an incomplete feature with major known soundness issues. In other words, the code path is impossible to implement on stable, and will probably stay that way for a long time, since not even min_specialization is stabilized yet.

As long as it is documented somewhere (and perhaps we can simplify it so that the temporary isn't needed) that would be fine as well.

I agree that wrapping that in a to_vec can be a good idea. I don't think the into version makes much sense, since we can't move anything out of the CoW arrays, and have to copy/clone anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Component: core (mod core_types, object, log, init, ...) quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants