Remove deprecated symbols from v0.9

[Bromeon]

Changes:

• PoolArray type aliases: document name of Godot equivalent + API link
• ClassBuilder
  • Remove add_method()
  • Remove add_method_advanced() -- now private
  • Remove add_method_with_rpc_mode()
  • Rename add_property() -> property()
  • Rename build_method() -> method()
  • Add signal()
• Add SignalBuilder + with_param*() methods
• Remove ScriptMethod, ScriptMethodFn, ScriptMethodAttributes
• Remove Color::rgb(), Color::rgba()
• Remove Reference::init_ref() (unsound to call directly)
• Remove FloatHint::Enum (has no effect)
• Rename Element -> PoolElement
• Rename SignalArgument -> SignalParam
• Rename String::forget() -> leak()
• Rename Variant::forget() -> leak()
• Fix bug with signal parameter types annotated in builder not propagated to Godot
• Annotate 3 builders with #[must_use] to avoid forgetting building
  • Not done for ClassBuilder; it's not a builder in that sense

[Bromeon]

Note: I did not remove the type aliases for PoolVector.

First, PoolArray<GodotString> is much more verbose than StringArray, even more so if qualified crate prefixes (like gd::) are used.

Second, it makes it harder to know which instantiations are valid for PoolArray. E.g. i32 works, but i64 doesn't. GodotString works, String doesn't. It needs an extra step of looking up the PoolElement documentation.

---

Now, I could rename them to the Godot equivalents, to make sure they are "pool" arrays. For example StringArray -> PoolStringArray.

However, I'm not sure if we would win that much. We have to consider that in GDScript, long type names are less of a problem due to dynamic typing. Plus, for PoolRealArray and PoolIntArray it's probably better if we keep the current names Float32Array and Int32Array to make clear which Rust type they can hold.

I added documentation for now, to make the relation to the Godot pool types clear. Let me know what you think -- it could also possibly be renamed later.

[chitoyuu]

Regarding build_method and build_property: do you think it might be a good idea to rename them to just method and property, now that it's no longer necessary to differentiate them from the older unsafe versions?

We might also want to builderize signal registration before the release, so we won't need to repeat this later.

[Bromeon]

Regarding build_method and build_property: do you think it might be a good idea to rename them to just method and property, now that it's no longer necessary to differentiate them from the older unsafe versions?

Since the only remaining methods are the builders for property, method and signals, definitely a good idea! Implemented.

---

We might also want to builderize signal registration before the release, so we won't need to repeat this later.

I agree. Pushed the changes -- I also added some convenience constructors for SignalArgument itself.
It looks like this now:

- builder.add_signal(Signal {
-    name: "tick_with_data",
-    args: &[SignalArgument {
-        name: "data",
-        default: Variant::new(100),
-        export_info: ExportInfo::new(VariantType::I64),
-        usage: PropertyUsage::DEFAULT,
-    }],
- });
+ builder
+    .signal("tick_with_data")
+    .arg(SignalArgument::with_default("data", Variant::new(100)))
+    .done();

or:

- builder.add_signal(Signal {
-     name: "completed",
-     args: &[SignalArgument {
-         name: "value",
-         default: Variant::nil(),
-         export_info: ExportInfo::new(VariantType::Nil),
-         usage: PropertyUsage::DEFAULT,
-     }],
- });
-
- builder.add_signal(Signal {
-     name: "resumable",
-     args: &[],
- });
+ builder
+     .signal("completed")
+     .arg(SignalArgument::untyped("value"))
+     .done();
+ builder.signal("resumable").done();

I might need a second pair of eyes on whether I understood the SignalArgument semantics correctly -- i.e. if new(), untyped() and with_default() constructors make sense. I haven't managed to find any documentation...

[Edit] Especially because signals are still untyped in GDScript, is the ExportInfo purely informational? Does the type appear in the editor or some other context?

[chitoyuu]

Perhaps we should just make name a type parameter then, if we just convert it to GodotString? Same with ClassBuilder::signal.

[Bromeon]

You probably mean name: impl Into<GodotString>? Yes, that's a good idea.

The new() method is not public, so conversion could also happen in ClassBuilder::signal(), and new() would directly take GodotString by value.

[chitoyuu]

i.e. if new(), untyped() and with_default() constructors make sense. I haven't managed to find any documentation...

[Edit] Especially because signals are still untyped in GDScript, is the ExportInfo purely informational? Does the type appear in the editor or some other context?

I think they make sense. IIRC the ExportInfo is only there for the editor (GDScript code generation, animations maybe?). I don't believe they are checked.

[chitoyuu]

I'm not sure if this works. I think you'd need PropertyUsage::NIL_IS_VARIANT (which is not exposed to GDNative) for this to work completely, but then again the argument types are not checked, so it might not matter anyway.

[Bromeon]

Ah ok, we had this code in impl NativeClass for FuncState, so I assumed there's a special meaning with nil and default.

https://github.com/godot-rust/godot-rust/blob/99a0d343d42767c26d2bd1ebb51a3c5c9ae61989/gdnative-async/src/rt/func_state.rs#L34-L43

Do you remember what the intention was there?

[chitoyuu]

There was no intention. It was just necessary to provide a value for all the fields. Arbitrary values are ok for the FuncState signals, since the latter would likely never show up in the editor. I was just unsure what such values could mean for signals that might.

[Bromeon]

I see. Then maybe we can go with untyped() which expresses the intention. If we find that another implementation fits better, we could change it transparently, and user code gets "auto-fixed".

[Waridley]

We might also want to builderize signal registration before the release, so we won't need to repeat this later.

I agree. Pushed the changes -- I also added some convenience constructors for SignalArgument itself. It looks like this now:

- builder.add_signal(Signal {
-    name: "tick_with_data",
-    args: &[SignalArgument {
-        name: "data",
-        default: Variant::new(100),
-        export_info: ExportInfo::new(VariantType::I64),
-        usage: PropertyUsage::DEFAULT,
-    }],
- });
+ builder
+    .signal("tick_with_data")
+    .arg(SignalArgument::with_default("data", Variant::new(100)))
+    .done();

or:

- builder.add_signal(Signal {
-     name: "completed",
-     args: &[SignalArgument {
-         name: "value",
-         default: Variant::nil(),
-         export_info: ExportInfo::new(VariantType::Nil),
-         usage: PropertyUsage::DEFAULT,
-     }],
- });
-
- builder.add_signal(Signal {
-     name: "resumable",
-     args: &[],
- });
+ builder
+     .signal("completed")
+     .arg(SignalArgument::untyped("value"))
+     .done();
+ builder.signal("resumable").done();

I might need a second pair of eyes on whether I understood the SignalArgument semantics correctly -- i.e. if new(), untyped() and with_default() constructors make sense. I haven't managed to find any documentation...

This could potentially be simplified to something like:

builder
    .signal("tick_with_data")
    .arg("data".default(100))
    .done();

builder
    .signal("completed")
    .arg("value")
    .done();

using impl Into<SignalArgument> and an extension trait for strings. Possibly even using generic parameter inference to generate the ExportInfo?

[Bromeon]

using impl Into<SignalArgument> and an extension trait for strings. Possibly even using generic parameter inference to generate the ExportInfo?

I'm not sure if we should pollute the &str extensions to allow something like "data".default(100) -- IDEs might then show this in every context. And the name "default" is very generic, when it's only usable for SignalArgument.

One thing I was considering, is renaming SignalArgument -> SignalArg or SignalParam (technically it's a parameter, not an argument):

builder
     .signal("completed")
     .param(SignalParam::untyped("value"))
     .done();

Or have separate param() overloads:

builder
     .signal("completed")
     .param_untyped("value")
     .param("my_int", VariantType::I64)
     .param_default("my_string", Variant::new("default"))
     .done();

A builder for the arg/param itself would likely not make things much more concise.

But I also think we shouldn't overdo it with ergonomics here; this is still a rarely-used API. I'd rather enable an #[export] macro at some point.

[Bromeon]

bors try

[bors]

try

Build succeeded:

• full-ci

[Bromeon]

Signal API is now like this:

fn my_register(builder: &ClassBuilder<MyType>) {
    // Add signal without parameters
    builder
        .signal("jumped")
        .done();

    // Add another signal with 1 parameter (untyped)
    builder
        .signal("fired")
        .with_param_untyped("weapon_type")
        .done();

    // Add third signal with int + String parameters, the latter with a default value "Kerosene"
    builder
        .signal("used_jetpack")
        .with_param("fuel_spent", VariantType::I64)
        .with_param_default("fuel_type", Variant::new("Kerosene"))
        .done();
}

Edit: reflect latest API

[Bromeon]

The builders might need another look (e.g. consistency in string parameters, with_*() naming, etc), but I'll wrap this PR up for now. Other changes are not urgent and can be done in v0.10, with deprecation if necessary.

I added #[must_use] attributes to the 3 builder structs, which gives a warning when done() is forgotten.

[Bromeon]

bors r+

[bors]

Build succeeded:

• full-ci