Properly export arrays, nodes, and resources by default #241

lilizoey · 2023-04-22T19:40:05Z

Adds in support for each type to define what hint and hint_string is used for that type when exported as a property. This is used here to make arrays get properly exported, as well as nodes and resources have their type restricted to the actual type they can hold. (see #230)

So now if you export a Gd<Mesh> or Option<Gd<Mesh>>, you wont get a list of every single resource in existence, but rather only resources that inherit from Mesh. And if you export a specific node-type, like Node3D you can only assign nodes that inherit from Node3D to it and not other nodes.

Also i couldn't find an easy way to conditionally implement some of this stuff for Gd based on whether they inherited from Resource, Node, or not. Orphan rules prevents me from doing so based on Inherits<T>. So i added an inherits method to check at runtime. It shouldn't have a big impact on performance since afaik each property will only call this once to register the property.

I'm not sure if this is the best solution, see my comment below for a major drawback. But overall it works fairly well.

closes #230

lilizoey · 2023-04-22T22:39:38Z

so a major issue with this approach in particular is that you can't export arbitrary types without specifying a hint and hint_string (unless we use specialization... which is a whole other can of worms, though it should be possible to make sound in this case (im not suggesting we use it)). This can be helped by us implementing ExportDefaultInfo for various commonly used types.

One thing this method enables is that we could represent some export variants using types, we could for instance do:

#[export]
multiline_string: MultilineString

Where MultilineString is just a newtype around GodotString (with deref maybe?) that has different defaults making it show up as a multiline string in the editor.

Bromeon

Thanks a lot!

I didn't have time to review everything yet, but here's already some feedback 🙂

Bromeon · 2023-04-25T19:06:47Z

godot-core/src/export.rs

+// Implement ExportDefaultInfo for some various common types so people can use them without specifying a
+// hint_type and hint_string.
+impl ExportDefaultInfo for () {}
+impl ExportDefaultInfo for String {}
+impl ExportDefaultInfo for i128 {}
+impl ExportDefaultInfo for isize {}
+impl ExportDefaultInfo for u64 {}
+impl ExportDefaultInfo for u128 {}
+impl ExportDefaultInfo for usize {}


All these types should not be exportable in my opinion.

i128, u128, u64 cannot be represented in Godot, so it's just unnecessary potential for errors.

isize and usize have a platform-dependent size, so it makes shipping robust applications harder. Godot also has no equivalent for them.

() can only ever have one value (aka zero information), so what's the idea here?

String is too expensive because it needs copying + reallocation on every access (see #gdnative/1020).

this just means that you can do

#[export(get = get_prop, set = set_prop)] prop: String

you still can't export them without specifying a setter/getter. since they dont implement Export. you just dont need to provide a hint/hint_string

i considered not doing () but it can be useful if you want a property exposed but dont actually need to store anything/you're storing something elsewhere than in the class.

Bromeon · 2023-04-25T19:09:17Z

godot-core/src/export.rs

+    impl_export_by_clone!(u8 => Int);
+
+    // Callables can be exported, however you can't do anything with them in the editor.
+    // But we do need to be able to export them since we can't make something a property without exporting.


since we can't make something a property without exporting.

Is that still true, after we discovered we can hide them from the editor via flag?

it isnt strictly true no, but until we change this it's true. since we currently do need anything that's exposed as a property to implemented Export.

Could you add a todo then, otherwise it is forgotten for sure.

(Not that we actively track TODOs, but we track comments even less 😉)

This one would still be nice 🙂

Bromeon · 2023-04-25T19:11:01Z

godot-core/src/obj/gd.rs

+    /// Returns whether `T` inherits from `U`.
+    ///
+    /// This is reflexive, i.e `T` inherits from itself.
+    ///
+    /// See also [`Inherits`] for a trait bound.
+    pub fn inherits<U: GodotClass>() -> bool {
+        if T::CLASS_NAME == U::CLASS_NAME {
+            true
+        } else if T::Base::CLASS_NAME == <()>::CLASS_NAME {
+            false
+        } else {
+            Gd::<T::Base>::inherits::<U>()
+        }
+    }


Quite a clever recursive implementation! 💡

If we want to provide this, it should probably being inherent on T and not Gd<T>. The usage below would also support this. There are 3 options:

global function inherits::<T, Base>()

GodotClass default impl T::inherits::<Base>()

extension trait T::inherits::<Base>() -- although I don't like defining a new trait just for this

makes sense, I'd probably go for the second option. it makes sense to me that a trait for classes would have such a function.

Bromeon · 2023-04-25T19:33:12Z

godot-core/src/export.rs

+ * file, You can obtain one at https://mozilla.org/MPL/2.0/.
+ */
+
+use crate::{builtin::GodotString, engine::global::PropertyHint};


Please separate use statements 🙂

Bromeon · 2023-05-08T19:06:02Z

godot-core/src/export.rs

+    /// Create a new `ExportInfo` with a property hint of
+    /// [`PROPERTY_HINT_NONE`](PropertyHint::PROPERTY_HINT_NONE).
+    pub fn new_none(variant_type: VariantType) -> Self {
+        Self {
+            variant_type,
+            hint: PropertyHint::PROPERTY_HINT_NONE,
+            hint_string: GodotString::new(),
+        }
+    }


Maybe name it with_hint_none() or so?
Otherwise it's not really clear that "none" refers to the property hint.

Bromeon · 2023-05-08T19:29:24Z

godot-core/src/export.rs

+
+impl<T: Export> Export for Option<T> {
+    fn export(&self) -> Self {
+        self.as_ref().map(|val| val.export())


Would this work?

Suggested change

self.as_ref().map(|val| val.export())

self.as_ref().map(Export::export)

Bromeon · 2023-05-08T19:32:43Z

godot-core/src/export.rs

+/// Trait for types that can be represented as a type string for use with
+/// [`PropertyHint::PROPERTY_HINT_TYPE_STRING`].
+pub trait TypeString {
+    /// Returns the representation of this type as a type string.
+    ///
+    /// See [`PropertyHint.PROPERTY_HINT_TYPE_STRING`](
+    ///     https://docs.godotengine.org/en/stable/classes/class_%40globalscope.html#enum-globalscope-propertyhint
+    /// ).
+    fn type_string() -> String;
+}


The type sounds like it's some sort of string; maybe name it TypeStringHint?
(*Hint could apply to other similar traits/types)

Bromeon · 2023-05-08T19:37:49Z

godot-core/src/export.rs

+    impl_export_by_clone!(f64 => Float);
+    impl_export_by_clone!(f32 => Float);
+    impl_export_by_clone!(i64 => Int);
+    impl_export_by_clone!(i32 => Int);
+    impl_export_by_clone!(i16 => Int);
+    impl_export_by_clone!(i8 => Int);
+    impl_export_by_clone!(u32 => Int);
+    impl_export_by_clone!(u16 => Int);
+    impl_export_by_clone!(u8 => Int);


I know they were already there, but how do these basic types (besides f64 and i64) behave in terms of exporting?

What happens if GDScript assigns a value that doesn't fit into the target type (especially for the integers)?

f32 seems to mostly be fine, generally just rounding/becoming inf

the integers however panic in rust. this causes godot to display the weird error of

though you also get the panic from rust which does state what the issue is:

Ok, that's at least reasonable behavior. Could you maybe add a short comment above the impl_export_by_clone! statements explaining this?

Bromeon · 2023-05-08T19:38:56Z

godot-core/src/export.rs

+    impl_export_by_clone!(u8 => Int);
+
+    // Callables can be exported, however you can't do anything with them in the editor.
+    // But we do need to be able to export them since we can't make something a property without exporting.


Could you add a todo then, otherwise it is forgotten for sure.

(Not that we actively track TODOs, but we track comments even less 😉)

GodotRust · 2023-05-10T00:56:51Z

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-241

Bromeon · 2023-05-11T21:50:39Z

godot-core/src/export.rs

+    impl_export_by_clone!(f64 => Float);
+    impl_export_by_clone!(f32 => Float);
+    impl_export_by_clone!(i64 => Int);
+    impl_export_by_clone!(i32 => Int);
+    impl_export_by_clone!(i16 => Int);
+    impl_export_by_clone!(i8 => Int);
+    impl_export_by_clone!(u32 => Int);
+    impl_export_by_clone!(u16 => Int);
+    impl_export_by_clone!(u8 => Int);


Ok, that's at least reasonable behavior. Could you maybe add a short comment above the impl_export_by_clone! statements explaining this?

Bromeon · 2023-05-11T21:51:04Z

godot-core/src/export.rs

+    impl_export_by_clone!(u8 => Int);
+
+    // Callables can be exported, however you can't do anything with them in the editor.
+    // But we do need to be able to export them since we can't make something a property without exporting.


This one would still be nice 🙂

Bromeon · 2023-05-11T21:56:29Z

godot-core/src/export.rs

@@ -0,0 +1,163 @@
+/*


This would probably fit better into the existing godot::bind module, rather than its own top-level one.

(I know the name "bind" is not great; it's coming from binding. "export" is however specifically the variable-export, and not functions/classes/etc. We can discuss this later maybe 🙂)

lilizoey · 2023-05-11T22:01:33Z

godot-core/src/export.rs

+    // But we do need to be able to export them since we can't make something a property without exporting.
+    // And it should be possible to access Callables by property from for instance GDScript.
+    // TODO:
+    // Remove export impl when we can create properties without exporting them.


@Bromeon i added a TODO

Bromeon · 2023-05-13T07:20:14Z

There are now some merge conflicts; other than that it looks good 👍
You can merge after resolving them.

Add TypeStringHint trait Add support for exporting nodes, resources, and arrays with type info

lilizoey · 2023-05-14T13:37:14Z

bors r+

bors · 2023-05-14T13:46:13Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

full-ci

282: godot-core: delegate TypeStringHint on Option<T> to T r=Bromeon a=gg-yb So when trying to export an array of resources property from my Rust struct I encountered the problem of crashes when the user adds null to the array (i.e. adding an element, not initializing it, then reopening the scene). I will open a separate issue for this. However, in investigating this, I saw that TypeStringHint is not implemented for Option<Gd<T>>, only for Gd<T>. I feel that it should be implemented for that type as well, but I'm not sure delegating to Gd<T> is the right call here, that's why this is a draft. Currently I just want to get this topic on the radar and spark discussion. However, should it turn out that delegating to Gd<T> here is the right thing to do, I believe the change can be applied as-is. From what I've seen from #241 there are no tests for TypeStringHint yet, so I haven't added any here, either. Co-authored-by: gg-yb <[email protected]>

lilizoey added bug c: register Register classes, functions and other symbols to GDScript labels Apr 23, 2023

lilizoey force-pushed the feature/default-export-hint branch 3 times, most recently from 28faf54 to b8995d0 Compare April 25, 2023 17:12

lilizoey marked this pull request as ready for review April 25, 2023 17:51

lilizoey requested a review from Bromeon April 25, 2023 17:52

Bromeon reviewed Apr 25, 2023

View reviewed changes

lilizoey force-pushed the feature/default-export-hint branch 2 times, most recently from 3c13cff to a36a002 Compare April 30, 2023 01:48

Bromeon reviewed May 8, 2023

View reviewed changes

lilizoey force-pushed the feature/default-export-hint branch from 06d0da0 to 1a1b5a5 Compare May 10, 2023 00:51

Bromeon reviewed May 11, 2023

View reviewed changes

lilizoey commented May 11, 2023

View reviewed changes

lilizoey force-pushed the feature/default-export-hint branch from 1a1b5a5 to 09f2e03 Compare May 12, 2023 17:18

Add ExportInfo struct

34faefa

Add TypeStringHint trait Add support for exporting nodes, resources, and arrays with type info

lilizoey force-pushed the feature/default-export-hint branch from 09f2e03 to 34faefa Compare May 14, 2023 13:32

bors bot merged commit 8990464 into godot-rust:master May 14, 2023

gg-yb mentioned this pull request May 23, 2023

godot-core: delegate TypeStringHint on Option<T> to T #282

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly export arrays, nodes, and resources by default #241

Properly export arrays, nodes, and resources by default #241

lilizoey commented Apr 22, 2023 •

edited

Loading

lilizoey commented Apr 22, 2023 •

edited

Loading

Bromeon left a comment

Bromeon Apr 25, 2023

lilizoey Apr 25, 2023 •

edited

Loading

Bromeon Apr 25, 2023

lilizoey Apr 25, 2023

Bromeon May 8, 2023

Bromeon May 11, 2023

Bromeon Apr 25, 2023

lilizoey Apr 25, 2023

Bromeon Apr 25, 2023

Bromeon May 8, 2023

Bromeon May 8, 2023

Bromeon May 8, 2023 •

edited

Loading

Bromeon May 8, 2023

lilizoey May 10, 2023

Bromeon May 11, 2023

Bromeon May 8, 2023

GodotRust commented May 10, 2023

Bromeon May 11, 2023

Bromeon May 11, 2023

Bromeon May 11, 2023

lilizoey May 11, 2023

Bromeon commented May 13, 2023

lilizoey commented May 14, 2023

bors bot commented May 14, 2023

	self.as_ref().map(\|val\| val.export())
	self.as_ref().map(Export::export)

Properly export arrays, nodes, and resources by default #241

Properly export arrays, nodes, and resources by default #241

Conversation

lilizoey commented Apr 22, 2023 • edited Loading

lilizoey commented Apr 22, 2023 • edited Loading

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilizoey Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon May 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GodotRust commented May 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented May 13, 2023

lilizoey commented May 14, 2023

bors bot commented May 14, 2023

lilizoey commented Apr 22, 2023 •

edited

Loading

lilizoey commented Apr 22, 2023 •

edited

Loading

lilizoey Apr 25, 2023 •

edited

Loading

Bromeon May 8, 2023 •

edited

Loading