search

godot-rust / gdnative

Rust bindings for Godot 3
https://godot-rust.github.io
MIT License
3.61k stars 210 forks source link

GDNative default parameters cause breaking changes #814

Open Bromeon opened 2 years ago

Bromeon commented 2 years ago

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:

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 commented 2 years ago

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

chitoyuu commented 2 years ago

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:

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

chitoyuu commented 2 years ago

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:

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

Bromeon commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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.

cuddlefishie commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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:

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#.

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 commented 1 year ago

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:

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:

Cons:

chitoyuu commented 1 year ago
  • 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 👍