GDNative default parameters cause breaking changes #814
Comments
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:
|
|
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:
|
|
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:
I agree with you that changing the DAEV would require a new minor release of godot-rust. |
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.
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
I'm not sure I understand how the first scenario differs from breaking 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. |
|
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 |
|
Sorry, forgot to update this issue. Over the next days, I'll work on making In a next step, we should think about simplifying the custom |
829: Support Godot 3.4 r=Bromeon a=Bromeon Updates api.json, CI scripts and examples. This is a **breaking change**. Some GDNative APIs added default parameters, which can currently not be modeled in a backwards-compatible way in godot-rust (Rust itself doesn't support default parameters). It is however possible to manually generate `api.json` for an older Godot version (3.2 or 3.3) and keep using godot-rust normally. Addresses #814 in the short-term 🛠️ Still on the radar: #640 📡 Co-authored-by: Jan Haller <[email protected]>
829: Support Godot 3.4 r=Bromeon a=Bromeon Updates api.json, CI scripts and examples. This is a **breaking change**. Some GDNative APIs added default parameters, which can currently not be modeled in a backwards-compatible way in godot-rust (Rust itself doesn't support default parameters). It is however possible to manually generate `api.json` for an older Godot version (3.2 or 3.3) and keep using godot-rust normally. Addresses #814 in the short-term 🛠️ Still on the radar: #640 📡 Co-authored-by: Jan Haller <[email protected]>
Bromeon
changed the title
|
@robert-w-gries Latest I will keep this issue open to discuss the handling of default parameters in the GDNative API. |
|
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. We could add a feature flag like 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! |
|
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. |
|
If we're exporting methods based on pointer, can we just check |
This implements cuddlefishie's suggestion in godot-rust#814, but with the semantics reversed to better conform with Cargo's additive combination of features. - Added the new feature flag `ptrcall`, which enables performant API calls at the cost of forward binary compatibility with the engine. - Added tests with and without the feature to the full CI suite.
This implements cuddlefishie's suggestion in godot-rust#814, but with the semantics reversed to better conform with Cargo's additive combination of features. - Added the new feature flag `ptrcall`, which enables performant API calls at the cost of forward binary compatibility with the engine. - Added tests with and without the feature to the full CI suite.
This implements cuddlefishie's suggestion in godot-rust#814, but with the semantics reversed to better conform with Cargo's additive combination of features. - Added the new feature flag `ptrcall`, which enables performant API calls at the cost of forward binary compatibility with the engine. - Added tests with and without the feature to the full CI suite.
973: Make ptrcalls opt-in r=chitoyuu a=chitoyuu This implements cuddlefishie's suggestion in #814, but with the semantics reversed to better conform with Cargo's additive combination of features. - Added the new feature flag `ptrcall`, which enables performant API calls at the cost of forward binary compatibility with the engine. - Added tests with and without the feature to the full CI suite. I believe the current semantics to be better, since by making the safer option the default, it helps reduce the surprise factor when someone tries to use the crate with a supposedly compatible version of Godot. Different opinions are welcome. Note that this only addresses binary compatibility -- #814 is also in a large part an API problem. Co-authored-by: Chitose Yuuzaki <[email protected]>
chitoyuu
added
the
breaking-change
|
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 (#814 (comment)) are:
Examples of collision avoidanceGiven the following Godot methods, all containing optional args: The following Rust methods will be generated: This may seem horrifying at first, but really what we're hoping for here is for the engine to never introduce
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")
}
} |
|
Very nice! I like the use of 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:
|
It can also be argued that it's a feature that Rust libraries commonly provide, and is thus expected by users.
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 We might need to dig into past
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 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 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 Thanks a lot for your input! |
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.
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 🙂
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,)); // laterOr, we just add a new base method: obj.method(many, no, longer, required, arguments); // now deprecated
obj.method_v2(many); // new oneTLDR: every approach is ugly 😬 it's a matter of how seriously we take compatibility (even across |
|
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:
The nightmare case is, of course, as you have said, becoming forced to keep multiple legacy versions for years. Docs get clogged with 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).
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. |
|
This is a blocking issue for me as I described in godot-rust/gdext#162. If you have a prefered solution I am happy to give it a try implemeting it. |
|
@mio991 Note that this is the 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 |
I can not specifiy the last argument for |
|
Ah, I see, that's about bitsets though (they're conceptually similar but not the same as enums in Godot)! |
322: Default parameters in Engine API + meta param/return types r=Bromeon a=Bromeon # Default parameters in Engine API Example [`RenderingServer::environment_set_ambient_light`](https://docs.godotengine.org/en/stable/classes/class_renderingserver.html#class-renderingserver-method-environment-set-ambient-light) with signature: ```gdscript void environment_set_ambient_light( RID env, Color color, EnvironmentAmbientSource ambient=0, float energy=1.0, float sky_contibution=0.0, EnvironmentReflectionSource reflection_source=0 ) ``` Previous gdext syntax: ```rs server.environment_set_ambient_light(rid, color, 0, 1.0, 0.0, 0) ``` Now: ```rs server.environment_set_ambient_light(rid, color) ``` With default arguments: ```rs // select what you like, skip out-of-order: server.environment_set_ambient_light_ex(rid, color).energy(2.0).done() // provide all: server.environment_set_ambient_light_ex(rid, color) .ambient(0) .energy(1.0) .sky_contribution(0.0) .reflection_source(0) .done() ``` See also [this commit](01ce0bb) for the real-world impact this change has, within gdext alone. --- ## Mechanics Methods accepting default parameters come in two flavors: `method` and `method_ex`. The `_ex` overload returns an "extender", a builder object providing a fluent API to accept values overriding the defaults. The arguments can be provided in any order, and arbitrary ones can be skipped. **This is different from C++ and GDScript, where you have to provide all default arguments in their positional order.** This effectively implements an API similar to the one discussed in godot-rust/gdnative#814 (comment), however in a non-generic way and without enforcing ordering. Theoretically it's possible to specify the same argument twice, but that's a risk I'm gladly taking if I can avoid type-state with its compile-time and complexity implications. --- ## Other changes This pull request comes with a few collateral features: * **Support for _meta_ fields in the JSON file.** Parameter or return types can have an extra "meta" type which constrains the domain for non-GDScript languages. For example, a parameter `int` can come with meta field `uint16`. These are translated to Rust, so that the correct type is passed (here `u16`). * **GDScript expression parser.** Implementing default parameters means we have to understand syntax like `Vector2(1, 2)` which is provided as the `default_value` field in GDScript. This one is then translated to an equivalent Rust representation when populating the extender struct. This turned out to be much more complex than anticipated, due to things like `0` meaning "enum" rather than "int", or beautiful expressions like `Array[RDPipelineSpecializationConstant]([])`. * **Refactor `class_generator.rs`.** Quite a lot of functionality in the code generator was touched. Already in #315, I added a domain representation for function arguments and parameters, distinct from the JSON models. The function to generate method definitions now accepts consolidated structs instead of a dozen parameters. I cleaned up a few other things on-the-fly. Possibly I'll do a few follow-up cleanups, but they can be done independently of this functionality. * **Implement `EngineEnum` for `Vector*Axis` enums.** This allows ordinal conversion from/to those types, just like other engine enums. Co-authored-by: Jan Haller <[email protected]>
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
Inputfor Godot 3.3:Class
Inputfor Godot 3.4:The extra parameter
exactis 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.
The text was updated successfully, but these errors were encountered: