Skip to content

Implement ToGodot/FromGodot for Vec<T> #263

Closed
@astrale-sharp

Description

@astrale-sharp

For option Maybe returning T::to_variant() if option is some else Variant::nil()
for Result dict!("Ok" : T::to_variant() )
for Vec, returning a variant array

Activity

Mercerenies

Mercerenies commented on May 8, 2023

@Mercerenies
Contributor

Option is being worked on (at least for Option<Gd<T>>) at #240, by the way. I'm not aware of any work being done for vectors or results.

astrale-sharp

astrale-sharp commented on May 8, 2023

@astrale-sharp
ContributorAuthor

I'd be interested to contribute if no one is on the case yet !

lilizoey

lilizoey commented on May 8, 2023

@lilizoey
Member

Vec<T> definitely has a pretty obvious implementation, alongside other obvious collections. In fact we already have a From impl to convert Array<T> into Vec<T>, though not the reverse which is weird, but i guess you can just do .iter().collect().

Result i'm a bit more unsure about. i think we should base Result on #246 when that's merged, rather than make a separate implementation.

Option will be handled by #247 as mentioned, and it makes more sense to give it a separate implementation since it's used to represent nullable values.

added
quality-of-lifeNo new functionality, but improves ergonomics/internals
c: registerRegister classes, functions and other symbols to GDScript
on May 8, 2023
added
featureAdds functionality to the library
and removed
quality-of-lifeNo new functionality, but improves ergonomics/internals
on May 8, 2023
Bromeon

Bromeon commented on May 8, 2023

@Bromeon
Member

What would the semantics for Result<T, E> be?
Would any Err(...) enumerator always map to Variant::nil()?

Also, what's the motivation behind it? Might be more explicit to use Result::ok() if that's desired 🤔

Bromeon

Bromeon commented on Jun 13, 2023

@Bromeon
Member

@astrale-sharp any comments? 🙂

astrale-sharp

astrale-sharp commented on Oct 26, 2023

@astrale-sharp
ContributorAuthor

Would any Err(...) enumerator always map to Variant::nil()?

I don't understand what you mean there.

If V and E in Result<V,E> are both ToGodot I don't see what's lost in making the Result ToGodot as well, it can make handling Errors on the gdscript side a little more convenient.

Actually I don't really care about Result but Option and Vec are definitly something I came across making my game

changed the title [-]Feature : implement ToVariant for Vec<T>, Result<T>, Option<T> where T : ToVariant[/-] [+]Feature : implement ToGodot for Vec<T>, Result<T>, Option<T> where T : ToGodot[/+] on Oct 27, 2023
lilizoey

lilizoey commented on Oct 27, 2023

@lilizoey
Member

Note: ToVariant has now been renamed/re-purposed into ToGodot

lilizoey

lilizoey commented on Oct 27, 2023

@lilizoey
Member

Suggestion:
Assume here that T: GodotConvert/ToGodot/FromGodot as appropriate.

GodotConvert:

  • Option<T>, with Via = Variant (unless other more specific implementation exists, such as for Option<Gd<T>>)
  • Vec<T>, with Via = Array<T::Via>
  • Result<T, ConvertError>, with Via = T::Via (see Make FromGodot conversions fallible #467)

ToGodot:

  • Option<T> maps None => Variant::nil(), Some(x) => x.to_variant()
  • Vec<T> does effectively vec.iter().collect()
  • Result<T,E> is either not implemented, or we just panic for Err. In the future we may support using Result as a return type, where Err prints an error to Godot or something like that.

FromGodot:

  • Option<T> map null => None and value => T::from_godot(value)
  • Vec<T> does effectively arr.iter_shared().collect()
  • Result<T, ConvertError> does Ok(T::try_from_godot(value)) (see Make FromGodot conversions fallible #467)

We then have:

  • Option always maps to a nullable variant, except when the type itself is nullable.
  • Vec is Array as most people would expect
  • Result can be used to detect errors when a function is called

Additionally we should implement Property and maybe Export for at least Option<T>, unless this causes godot to behave weirdly (such as the editor displaying weird values if you attempt to set it to the wrong value). We may consider only implementing it for Option<Variant> if that's the case.

Bromeon

Bromeon commented on Oct 28, 2023

@Bromeon
Member

Sounds good, thanks for listing it up.

I'm not sure about Option<Variant> though -- you would have two possible null states: None and Some(Variant::nil()).
It might be a necessity if we have an Option<T> blanket impl though...

lilizoey

lilizoey commented on Oct 28, 2023

@lilizoey
Member

Sounds good, thanks for listing it up.

I'm not sure about Option<Variant> though -- you would have two possible null states: None and Some(Variant::nil()). It might be a necessity if we have an Option<T> blanket impl though...

i think we'd probably implement it for each different option of Via. Since there's only a limited number of possible Via types. so we can choose to leave out Variant if we want. (kinda necessary if we want Option<Gd<T>> to be different)

9 remaining items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    c: registerRegister classes, functions and other symbols to GDScriptfeatureAdds functionality to the library

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Implement `ToGodot`/`FromGodot` for `Vec<T>` · Issue #263 · godot-rust/gdext