GDNative default parameters cause breaking changes

[Bromeon]

The dodge_the_creeps example in godot-rust, which uses Input::is_action_pressed(), no longer compiles with Godot 3.4.

Reason is an API change in GDNative:

• Class Input for Godot 3.3:

• Class Input for Godot 3.4:

The extra parameter exact is optional, which is a non-breaking change in GDScript and C++, but a breaking one in Rust, since the language does not support default parameters.

There are likely more places in the GDNative API with such changes. At the moment, they make it impossible for godot-rust to support multiple Godot minor versions simultaneously. Either we find a way to support such APIs in Rust, or we can only support the latest minor version.

[Bromeon]

Related: https://github.com/godot-rust/godot-rust/issues/291 https://github.com/godot-rust/godot-rust/issues/264

[chitoyuu]

The extra parameter exact is optional, which is a non-breaking change in GDScript and C++, but a breaking one in Rust, since the language does not support default parameters.

There are likely more places in the GDNative API with such changes. At the moment, they make it impossible for godot-rust to support multiple Godot minor versions simultaneously. Either we find a way to support such APIs in Rust, or we can only support the latest minor version.

There was once a proposal to introduce call-builders on Discord, but it appears that it has never been formalized as a feature proposal on GitHub. The idea being:

• A builder type is generated for all methods with optional arguments. Calling is_action_just_pressed would look something like input.is_action_just_pressed_ex("action_name").exact(false).call().
• The non-builderized version of the method will always contain only the required arguments (so the addition of an optional argument won't be breaking).

However, there were a few issues for which no consensus was found:

• What the method name should look like, how collisions (however unlikely) should be handled, and whether "raw" versions with full optional arguments should be kept.
• What should happen if a builder is dropped unused. The safest option is to just "do nothing", but some have argued that the method should get called on drop, even if that would result in a call order that's probably not the intention. #[must_use] can help prevent this in practice, but it doesn't make the type linear.
• Whether binary compatibility should be part of this new improvement. Failure to provide an optional argument when calling a method through ptrcall is UB. Claiming binary compatibility would require abandoning ptrcalls.
• What happens when optional arguments are provided out of order (e.g. when the second optional argument is provided while the first is not). One simple way to handle this, that could be used in an initial implementation, would be to simply panic whenever this is detected, but that's not what is usually expected from a builder pattern. Eventually, this should be changed, but doing so requires parsing the highly irregular serialization format that is used for default values in api.json.

[chitoyuu]

From a broader view, there is the question of what "engine compatibility" actually means. For reference, there are currently two definitions of a compatible engine in the README:

• The minimum compatible engine version (MCEV), with api.json replacement. This is currently Godot 3.2. Changing this version would require a breaking semver. The specific problem described in this issue does not require a change to the MCEV.
• The default API version (DAEV), which is currently Godot 3.2.3-stable. What would happen when this version changes does not appear to be clearly defined, but if a change is expected to break user code, I would assume that it would require a breaking semver. The specific problem described in this issue requires a change to the DAEV.

It seems to me that if current policies are followed, changing the DAEV to Godot 3.4 would:

• Require a breaking semver release.
• Not require reverting the compatibility promises to "only the latest minor version" (in the sense that the MCEV would remain at Godot 3.2), as the top comment suggested.

[Bromeon]

Thanks for the insights, great that some thought already went into it!

Call-builders sound like a highly sophisticated solution, although I have some more basic concerns:

1. Is compatibility with multiple Godot versions a feature that many users benefit from? Obviously it's nice to have, but when it comes at a cost (more cumbersome function calls), we'd need to consider the trade-offs. Maybe it turns out that most people either pin a godot-rust version or are able to freely update Godot minor versions, and we would make our sometimes already verbose API even more awkward to use.

2. The suggestion works for default parameters, but what about other breaking changes, like change of types (often not breaking in C++), function names, etc? The Godot versioning guide only says:

   The minor version is incremented for feature releases which do not break compatibility in a major way. Minor compatibility breakage in very specific areas may happen in minor versions, but the vast majority of projects should not be affected or require significant porting work.

   In other words, best-effort -- breaking changes can happen at any time.

3. There are two scenarios, I don't know if it makes sense to treat them differently?

   • GDNative API exposed through bindings changes in a breaking way. This may be the result of the user deliberately selecting a different engine version, but may leave godot-rust working otherwise.
   • GDNative API used by godot-rust itself breaks. This mostly affect the central classes Object, Reference etc. This would need some feature-gating or separate release.

I agree with you that changing the DAEV would require a new minor release of godot-rust.

[chitoyuu]

1. Is compatibility with multiple Godot versions a feature that many users benefit from?

This is one of those things that are very hard to tell, due to selection bias. We don't see anyone benefit from the feature, not because it truly isn't useful, but because at this point in time, nobody has had to maintain a godot-rust codebase over a long period yet. User polls are also unlikely to be useful in this case, because developers who have released, or plan to release commercial games, that might possibly in some cases actually benefit from the feature, are far outnumbered by developers that never even plan to release anything serious. If user demographics are followed blindly, you'd end up with something that appeals entirely to novices and of dubious value to veterans -- like the YouTube tutorial scene.

All in all, this is a difficult decision to make indeed. I do think, however, that the current commitment is unlikely to cause any major maintenance effort if constrained to the 3.x range.

2. The suggestion works for default parameters, but what about other breaking changes, like change of types (often not breaking in C++), function names, etc?

Those would be changes to DAEV, but not MCEV, and thus in line with the current commitment described in README.

An approach like #496, but limited to minor versions, used to select different pre-generated -bindings crates, could be used as a way to hit a stable 1.0 even if such changes occur.

There are two scenarios, I don't know if it makes sense to treat them differently?

• GDNative API exposed through bindings changes in a breaking way. This may be the result of the user deliberately selecting a different engine version, but may leave godot-rust working otherwise.
• GDNative API used by godot-rust itself breaks. This mostly affect the central classes Object, Reference etc. This would need some feature-gating or separate release.

I'm not sure I understand how the first scenario differs from breaking api.json changes, that would be a simple DAEV increment which doesn't affect the current compatibility commitment.

The second one would require some feature-gating, and is the only scenario where real maintenance effort is required to keep the current commitment, I agree.

[robert-w-gries]

It looks like there still isn't a consensus on how to handle this issue. In the meantime, you could add a note in the README stating that godot-rust is not compatible with godot 3.4 unless you build from source with latest api.json.

Additionally, you could create a 3.4 branch with the latest api.json + changes needed for doge_the_creeps to compile and point to the branch in the master branch README.

[Bromeon]

Sorry, forgot to update this issue.

Over the next days, I'll work on making master compatible with stable Godot 3.4. There are likely more people who are willing to upgrade and benefit from latest Godot features/bugfixes, than downgrade. Those who want to stay with 3.2 or 3.3 can still manually generate the api.json.

In a next step, we should think about simplifying the custom api.json generation step. https://github.com/godot-rust/godot-rust/issues/640 offers a very interesting idea, for example.

[Bromeon]

@robert-w-gries Latest master is now up-to-date with Godot 3.4.

I will keep this issue open to discuss the handling of default parameters in the GDNative API.

[hydrolarus]

Using "varcalls" instead of "ptrcalls" should make those additions of optional parameters non-breaking changes, at the expense of having slower method calls into engine classes.

Using "varcall" is essentially the same mechansim that GDScript or VisualScript uses, so also shares the performance characteristics.

Doing a "ptrcall" is basically the same as taking an untyped function pointer that points directly (more or less) to the in-engine implementation of a method. When doing such a call the number and types of arguments and return values must match exactly, because it's like calling a function pointer.

Because the performance is better with ptrcalls, the bindings generator tries to select those wherever possible (there are some conditions where a varcall is necessary).

For more "used across different versions" projects, such as plugins that build on GDNative and ship with binaries, it might make sense to switch every method binding to use varcalls. That would mean performance might be a little worse, but it would be guaranteed to be compatible across minor versions, even with the same binary.

We could add a feature flag like generate_only_varcalls (or with a different name) that overrides this selection process to never generate ptrcalls, in case a plugin author wants to guarantee compatibility across minor versions.

https://github.com/godot-rust/godot-rust/blob/42a6e768a02ed32068ae1dfe5a5e655181ea2e6f/bindings_generator/src/methods.rs#L124-L135

Would that be interesting for users of this crate or does this seem like a niche case and not requiring an extra feature flag and another knob to turn or not turn?

Would it make sense to make varcalls the default and recommend users to use ptrcalls only in release builds?

Let me know what you think!

[chitoyuu]

Sounds like a good idea to me! This should deal with ABI compatibility issues to the extent that is possible on our side, although ideally we'd still want to (separately) make API compatibility easier in some way.

[Hexorg]

If we're exporting methods based on pointer, can we just check Option<T> arguments and generate n^2 methods with slightly different names where n is Option<T> count?

[chitoyuu]

The ABI part is addressed in #973. I'll try to implement the call-builder solution for the API part for v0.12. The plans to deal with previously unresolved questions (https://github.com/godot-rust/gdnative/issues/814#issuecomment-963308724) are:

• Builderized methods are named {original_name}_ex. "Raw" versions with full optional args are no longer generated. Currently, there are no methods with a _ex suffix in the Godot API. To guard against name collisions in the future, any Godot methods with names that end in _ex(_ex|_godot)+ will have an extra _godot appended to their names (and _godot_ex appended to their builderized versions).

Examples of collision avoidance

Given the following Godot methods, all containing optional args:

foo
foo_ex

// Implausible names
foo_ex_godot
foo_ex_godot_ex
foo_ex_godot_ex_ex
foo_ex_godot_ex_ex_godot

The following Rust methods will be generated:

foo (minimal version of `foo`)
foo_ex (builderized version of `foo`)
foo_ex_godot (minimal version of `foo_ex`)
foo_ex_godot_ex (builderized version of `foo_ex`)

// Implausible names
foo_ex_godot_godot (minimal version of `foo_ex_godot`)
foo_ex_godot_godot_ex (builderized version of `foo_ex_godot`)
foo_ex_godot_ex_godot (minimal version of `foo_ex_godot_ex`)
foo_ex_godot_ex_godot_ex (builderized version of `foo_ex_godot_ex`)
foo_ex_godot_ex_ex_godot (minimal version of `foo_ex_godot_ex_ex`)
foo_ex_godot_ex_ex_godot_ex (builderized version of `foo_ex_godot_ex_ex`)
foo_ex_godot_ex_ex_godot_godot (minimal version of `foo_ex_godot_ex_ex_godot`)
foo_ex_godot_ex_ex_godot_godot_ex (builderized version of `foo_ex_godot_ex_ex_godot`)

This may seem horrifying at first, but really what we're hoping for here is for the engine to never introduce _ex and much less _ex_godot suffixes to their methods in the first place. It doesn't make much sense from an Engine point of view to create such variants, because they can leverage optional arguments for GDScript and C#.

• Call-builders do nothing when dropped. However, they will be annotated with #[must_use], which should cause Rust to emit warnings unless they are explicitly ignored (probably not what the user wants).
• For the initial implementation, optional parameters must be provided in the correct order. This will hopefully be enforced statically (see below).
• For the initial implementation, ptrcall will only be used if all optional parameters are provided, and if the ptrcall feature is enabled. Non-builderized versions of methods will always use varcall under the hood, if any optional arguments exist, even if ptrcall is enabled as a feature.
• The type will have to be nameable for the sake of doc generation, but stability promises will be explicitly limited.

I expect the initial API to be something like:

#[must_use]
struct IsActionPressedEx<T1, T2 = ()> {
    action: T1,
    exact: T2,
}

impl SomeType {
    fn is_action_pressed_ex<T1>(&self, action: T1) -> IsActionPressedEx<T1, ()>
    where
        T1: Into<GodotString>,
    {
        IsActionPressedEx {
            action,
            exact: (),
        }
    }
}

impl<T1> IsActionPressedEx<T1, ()>
where
    T1: Into<GodotString>,
{
    fn exact(self, exact: bool) -> IsActionPressedEx<T1, bool> {
        IsActionPressedEx {
            action: self.action,
            exact,
        }
    }

    fn call(self) -> bool {
        todo!("use varcall")
    }
}

impl IsActionPressedEx<T1, bool>
where
    T1: Into<GodotString>,
{
    // shouldn't cause a conflict with the `()` impl above
    fn call(self) -> bool {
        todo!("use varcall or ptrcall depending on feature flags")
    }
}

[Bromeon]

Very nice! I like the use of () to signal "no argument present".

One thing I was thinking about -- in our previous discussions, we always assumed that default parameters should be provided as named parameters. However, in both GDScript and C++ they are simply positional ones.

While it's useful to skip some default parameters (e.g. only provide a value for the last one), things to consider:

• It's not a feature that C++ and GDScript provide and is thus expected by users.
• Changing a parameter name now becomes a breaking change in Rust, while it wasn't in GDScript/C++.
• On the other hand, an order change of optional parameters is no longer breaking, but this is very unlikely to happen due to still being breaking in GDScript/C++.

That said, it could still prove a very useful addition, especially compared to the current approach.

---

For GDExtension, I'd like to experiment a bit with positional arguments. One idea I had:

// Machinery
trait OptionalArgs2<P0, P1> { ... }

impl<P0, P1> OptionalArgs2<P0, P1> for () // not strictly needed
    { ... }
impl<P0, P1, A0> OptionalArgs2<P0, P1> for (A0,) 
    where A0: Into<P0> { ... }
impl<P0, P1, A0, A1> OptionalArgs2<P0, P1> for (A0, A1)
    where A0: Into<P0>, A1: Into<P1> { ... }

// API

// *Every* method has this signature for all required parameters:
fn some_method(arg: i64);

// Those with default ones additionally have this:
fn some_method_ext(arg: i64, optionals: impl OptionalArgs2<String, bool>);

// Usage
some_method(32);
some_method_ext(32, ()); 
some_method_ext(32, ("hello")); 
some_method_ext(32, ("hello", true)); 

Pros:

• Syntax is as close as we can get to GDScript calls (at least without using macros).
• No forgetting of call() at the end.
• Compatible if a first default parameter is added to method (method + method_ext overloads).
• Compatible if another default parameter is added (OptionalArgs3 would still have impl for a 2-tuple).

Cons:

• Syntax may not feel very natural/idiomatic for some Rust devs.
• You can only omit arguments from the end of the list, not skip some.
• Not compatible if a required parameter becomes optional. This is also true for the named approach.
• Can be less readable for long argument lists, due to not having named arguments (IDE might help).

[chitoyuu]

• It's not a feature that C++ and GDScript provide and is thus expected by users.

It can also be argued that it's a feature that Rust libraries commonly provide, and is thus expected by users.

• Changing a parameter name now becomes a breaking change in Rust, while it wasn't in GDScript/C++.

This is something I haven't considered about. It should be however be possible to "remember" all the past names of an argument with automation, and keep any old named methods around (with #[deprecated]). This only needs to apply to crates builds, of course.

We might need to dig into past api.jsons to find out the actual probability of this happening, but I do not anticipate it to be very high.

• Not compatible if a required parameter becomes optional. This is also true for the named approach.

This neither. For the named approach it's possible to force all parameters to be specified through the builder interface (i.e. no arguments in the initial is_action_pressed_ex call), but this still leaves open the question of non-builderized versions.

One option is again to memorize the original required parameter count, and generate a new version each time a parameter is optional-ized and deprecate the original. I'm not sure how ugly this will get though, since this scenario may have a higher chance of actual occurrence compared to the previous one, and the impact is higher (new method on the base type vs. new method on a builder). Might need to dig into past api.jsons to find out.

I'm not sure I can come up with something better for the positional API.

---

Overall I feel like that the proposed pros of the positional API do not outweigh the ability to have a idiomatic Rust interface. Realistically call() is very unlikely to be forgotten due to #[must_use] -- the lack of strict linearity being only a theoretical issue. The last two a named approach does as well. So ultimately, it comes down to a stylistic choice, and I sense that this is where the two libraries must diverge.

Thanks a lot for your input!

[Bromeon]

It should be however be possible to "remember" all the past names of an argument with automation

That's actually an interesting suggestion, I could also imagine other cases where fallbacks can be useful. Although it might be quite a bit of effort.

Overall I feel like that the proposed pros of the positional API do not outweigh the ability to have a idiomatic Rust interface. So ultimately, it comes down to a stylistic choice, and I sense that this is where the two libraries must diverge.

I agree that it's a stylistic choice. Might also need some experimenting to see if the positional API works well enough or not -- I'm open to use named arguments in GDExtension too, if the other one proves to be a hassle 🙂

---

I'm not sure I can come up with something better for the positional API.

The tricky one is that the base method breaks whenever a required parameter becomes optional, regardless of named/positional approach. Even with awareness of previous versions and changes, we might in the worst case be forced to keep legacy versions around for years:

obj.method(many, no, longer, required, arguments);

// which would, as a fresh method, just be written as:
obj.method(many);

Some brainstorming how to handle that with the "ex" APIs:

// named: support additional conversion from ()
obj.method_ex(many, (), (), (), ()).call(); // positional
// note: I didn't want to suggest having *every* required parameter as a builder method

// positional -- ex method *always* takes exactly one tuple.
// the trait would have (min=1, max=5) arity, and enlarging this range is backwards-compatible
obj.method_ex((many, no, longer, required, arguments));  // initial
obj.method_ex((many,));                                  // later

Or, we just add a new base method:

obj.method(many, no, longer, required, arguments); // now deprecated
obj.method_v2(many); // new one

TLDR: every approach is ugly 😬 it's a matter of how seriously we take compatibility (even across 0.minor versions).

[chitoyuu]

I don't think we want to jump to conclusions here, before looking at empirical data of how often did any of these "exotic" breaking patterns actually happen for the whole duration of Godot 3, if ever. Every approach here is indeed ugly, but not in equal ways so:

Or, we just add a new base method:

obj.method(many, no, longer, required, arguments); // now deprecated
obj.method_v2(many); // new one

The nightmare case is, of course, as you have said, becoming forced to keep multiple legacy versions for years. Docs get clogged with v1, v2, v3 and v4s of multiple functions and everything is terrible. The best case here, however, is just perfectly natural Rust.

This would be ideal in a world where optionalizations are few and far-between, and when they do happen, multiple arguments become optional at once (so fewer versions are necessary).

// positional -- ex method *always* takes exactly one tuple.
// the trait would have (min=1, max=5) arity, and enlarging this range is backwards-compatible
obj.method_ex((many, no, longer, required, arguments));  // initial
obj.method_ex((many,));                                  // later

This avoids the nightmare case, at the cost of making every single method call slightly, but uniformly ugly, regardless of which world we're living in.

This would be ideal in a world where arguments become optionalized, one by one, all the time, as such events do not generate extra permanent ugliness under this design.

[mio991]

This is a blocking issue for me as I described in godot-rust/gdextension#162.

If you have a prefered solution I am happy to give it a try implemeting it.

[Bromeon]

@mio991 Note that this is the gdnative project, so implementing it here will not immediately help your use case.

Also, while inconvenient, I would dispute the "blocking" nature of this: this issue has existed for years, and there's a simple workaround of specifying arguments explicitly. It's definitely not pretty, but definitely not a dealbreaker either. This just to keep in mind when it comes to priorization of different features/bugfixes.

I'll try to make some room in the next few weeks for looking at default parameters in gdextension, but there are a few more important things I'd like to tackle first (such as online documentation).

[mio991]

Also, while inconvenient, I would dispute the "blocking" nature of this: this issue has existed for years, and there's a simple workaround of specifying arguments explicitly.

I can not specifiy the last argument for ArrayMesh::add_surface_from_arrays because the default value 0 is not a defined ArrayFormat.

[Bromeon]

Ah, I see, that's about bitsets though (they're conceptually similar but not the same as enums in Godot)! I will implement something for them 👍