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: #291 #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. #640 offers a very interesting idea, for example.