Update godot_nativescript.h (breaking changes in godot_property_hint)

[Bromeon]

GDNative API introduced breaking changes here by adding enum constants in the middle, severing compatibility of current godot-rust with Godot versions prior to 3.5(.1).

This change was already applied to api.json as part of #910 and warned about in #942, but not yet to the C API in godot_nativescript.h. This PR also updates the godot_headers directory to the tagged 3.5.1-stable release.

Fixes #1010.

[Bromeon]

I also updated the entire godot_headers including gdnative_api.json in a 2nd commit.

It looks like at least in some parts, compatibility with versions < 3.5.1 is already broken; so not sure if we lose something by using a newer version. Some feedback would be appreciated though.

[chitoyuu]

It looks like at least in some parts, compatibility with versions < 3.5.1 is already broken; so not sure if we lose something by using a newer version. Some feedback would be appreciated though.

I think this might be a good incentive for us to find a way to serve generated code for multiple versions of Godot headers at the same time. It might be safe to break compat with older versions in the year of 2023, but we still have future Godot 3 versions incoming, and we don't know what'll happen with those. Also related is the CI failure:

Unknown API type and version: ("ARVR", 1, 2)

In an ideal world where Godot devs understand the reasons behind GDNative's design, newer API versions can simply be ignored, but sadly that's not the world we are in, and I don't see how we can ensure forward compatibility with the way they make changes.

I suppose it's always a safe choice to refuse to work outright, although perhaps we can also allow non-essential modules to be skipped given how what most people want is just the core and NativeScript. That could even be the default, with other modules being opt-in by a crate feature.

---

I'm not sure if this needs to "retroactively" become a breaking version, if we decide to in fact officially cut compatibility with versions < 3.5.1.

[Bromeon]

I think this might be a good incentive for us to find a way to serve generated code for multiple versions of Godot headers at the same time.

We might also want to get some insights if this is a requested feature at all. I have the feeling most people just update both Godot and godot-rust versions from time to time, but I might be wrong.

On the other hand, once we already have such a system (e.g. proof-of-concept in the GDExtension binding), it would probably not hurt to make it official.

I'm not sure if this needs to "retroactively" become a breaking version, if we decide to in fact officially cut compatibility with versions < 3.5.1.

I guess it depends on the level of automation. If we drive it to a point where all we need to do is specify some supported versions, and tooling will download/build/generate the respective Godot artifacts, then it's not really more effort to support older ones. But I'm dreaming here 😏

---

I think I got the update working (needed a new struct definition for ARVR 1.2), but possibly bors disagrees.

bors try

[chitoyuu]

For now I think it's fine to just break compat and bump the minor version, but I can't doing this manually looks like the ideal solution to me. Maybe bindgen is a dead end and we should instead look for some way to use the jsons directly?

Anyway, I don't intend to block this PR -- just thinking about how we can go forward to minimize the chances of manual intervention and compat breaks being necessary.

[bors]

try

Build failed:

• full-ci

[Bromeon]

This officially breaks compat with Godot 3.2, according to CI. Maybe we should really defer the 2nd commit to later; I could move it to a separate in-draft PR.

Is the gdnative_json.api + godot_headers update necessary to address the problem in #1010?

[chitoyuu]

Even the first commit alone would break compat with older versions, just like how its absence is breaking newer versions.

Maybe it's just better to drop the older engines and release a breaking version for now. Optionally we can try to blame the headers and see when exactly are the culprits added to make our compatibility list more precise.

Support can always be added back later when we have the automation to do that.

---

Enums in the headers have always been a bit questionable honestly. The usage flags enum has been outdated for quite some time I think. I wonder if there's benefit in decoupling our own enums from those and instead doing translations at run-time, with values grabbed from the engine source code.

We can for example ship multiple versions of those translation tables and switch between them at init depending on the actual version we get from the running engine.

[Bromeon]

If we break with everything < 3.5.1, this has quite some implications. For example, the custom-godot feature loses a lot of its usefulness.

And I also don't think now is a "special" threshold -- breaking changes will happen again, so we either only support the latest version or we have a sophisticated model to support multiple.

---

Enums in the headers have always been a bit questionable honestly. The usage flags enum has been outdated for quite some time I think. I wonder if there's benefit in decoupling our own enums from those and instead doing translations at run-time, with values grabbed from the engine source code.

To be fair, it would be a complete non-issue if certain rules were followed, like appending new enumerators only at the end or using explicit numbering.

I also reported this very issue in October 2022 to Godot developers, but it got a bit lost in discussion:

Maybe my point of view from a Rust binding perspective:
Godot 3.* has broken compatibility quite a few times. Subtle things like enum reordering and parameter type change between patch versions caused rather annoying bugs, and meant that one binding version had to match precisely with one Godot version (otherwise the api.json would create problems at runtime). As such, I would appreciate that once 4.0 is released, such changes are getting more consideration. But I also think we're heading in the right direction, with e.g. function hashes to compare.

I would also happily assist regarding tooling to check compatibility between two extension JSON files; things like this could then be part of CI to avoid accidental breakage.

And I think my last point is important: with dozens of people + external contributors, it's easy to make such mistakes, and I wouldn't blame anyone for overlooking the consequences. While I don't think it's our responsibility to write such tooling, I'd still prefer that over workarounds on godot-rust side like translation tables. At least it would benefit the entire ecosystem. But maybe it's not realistic in the first place and we need it anyway for other things (default parameters...). Hard to anticipate...

It also seems to me that GDNative is much more complex in that regard than GDExtension. While in the latter, you have one JSON + one C header, here is an entire godot_headers directory. There is a relationship between gdnative_api.json, api.json, gdnative_api_struct.gen.h and all the other headers.

[chitoyuu]

And I also don't think now is a "special" threshold -- breaking changes will happen again, so we either only support the latest version or we have a sophisticated model to support multiple.

I was under the impression that it was one because you expedited to debug and make a PR in just one hour from the original issue 🙂

I agree that a decision needs to be made, but I don't think that a permanent one needs to be made right now. In many senses, this is like the good vs perfect dilemma: and there is the third option of "good now, maybe perfect later" -- which would be to merge this for now, while continuing to look for a way to expand compatibility later. That's what I meant by saying:

Support can always be added back later when we have the automation to do that.

---

While I don't think it's our responsibility to write such tooling, I'd still prefer that over workarounds on godot-rust side like translation tables. At least it would benefit the entire ecosystem.

If it still matters when it gets merged, supposing it'll ever get merged, and devs won't simply make changes to the reference file to silence it in the name of something else. I personally will not sink my time into making something that after 10 rebases, maybe will make somewhat sure that starting from some version released a year later that might as well be the second to last release of Godot 3, no future versions will contain breaking changes to GDNative. No, thanks.

If you will, feel free to, and I wish you good luck. I'd prefer to do things that can more realistically make a difference. 🙂

---

It also seems to me that GDNative is much more complex in that regard than GDExtension. While in the latter, you have one JSON + one C header, here is an entire godot_headers directory. There is a relationship between gdnative_api.json, api.json, gdnative_api_struct.gen.h and all the other headers.

Yes. I think we only really need the headers for the typedefs and enums. The layouts of the structs themselves I think can be gathered from the JSON even without the .gen.h file.

[Bromeon]

I agree that a decision needs to be made, but I don't think that a permanent one needs to be made right now. In many senses, this is like the good vs perfect dilemma: and there is the third option of "good now, maybe perfect later" -- which would be to merge this for now, while continuing to look for a way to expand compatibility later.

I think that summarizes it very well 👍

What we would need to consider though: dropping support for some Godot versions is technically a breaking change. I don't think a real user is affected, and there is already some breakage. But the failing Godot 3.2 CI shows that scenarios, where previously working setups break, are possible. If we follow that strictly, merging this PR would block us from having further 0.11.x patch releases.

Or another way to look at it: this fixes real issues like #1010 and thus constitutes a patch release, and setups that relied on compatibility were already broken and just happened to work. Including our CI job 😉

---

I personally will not sink my time into making something that after 10 rebases, maybe will make somewhat sure that starting from some version released a year later that might as well be the second to last release of Godot 3, no future versions will contain breaking changes to GDNative. No, thanks. [...] If you will, feel free to, and I wish you good luck. [...]

Fully agree here. Godot 4 is likely the more promising one for such an endeavor, if at all. And this would of course need to be coordinated with Godot devs. If there's no interest, I can also use my time for other things, but it might be worth a try. The developers have been quite receptive for input regarding GDExtension.

Also, my earlier point still stands:

We might also want to get some insights if this is a requested feature at all. I have the feeling most people just update both Godot and godot-rust versions from time to time, but I might be wrong.

There is still value in not needing to exactly match Godot + godot-rust versions (manual lookup in a table), but maybe the range to support could be smaller. If that actually simplifies things in practice, I don't know.

[chitoyuu]

...merging this PR would block us from having further 0.11.x patch releases.

What's so important about that, when the 0.11 "branch" isn't intended to be a 1.0 candidate anyway?

Or another way to look at it: this fixes real issues like #1010 and thus constitutes a patch release, and setups that relied on compatibility were already broken and just happened to work. Including our CI job wink

Our readme says:

Compatibility list:

• Godot 3.5.1 (works with gdnative 0.11)
• Godot 3.4 (works with gdnative 0.10, custom build for 0.11)
• Godot 3.3 (custom build)
• Godot 3.2 (custom build)

Never were the previous versions explicitly dropped in the changelogs either. There being hidden compatibility breaks is the bug here, and the only way to fix it while staying on 0.11 is by restoring compatibility with everything. Otherwise, we need to go to 0.12.

If anyone's setup happened to work with an engine version we stated to be supporting, these setups need to continue to be happening to work -- otherwise there's no sense in maintaining the difference between breaking and non-breaking releases at all. (In which case we go to 0.12 anyway.)

Again, I don't understand the significance of this becoming a patch release in 0.11.x.

[Bromeon]

What's so important about that, when the 0.11 "branch" isn't intended to be a 1.0 candidate anyway?

Nothing, I just wanted to make clear we're on the same page. I'm not emotionally attached to 0.11 😀

I don't know what your plans were for 0.12, all I'd like to avoid is that the next release is half a year out because 0.12 should address a ton of other things. Then we rather do 0.12 soon, and then 0.13. We're not running out of numbers anytime soon 🙂

---

So... I remove support for 3.2 in CI here, then we merge this, and master will then head towards 0.12?

[chitoyuu]

I see. That's great -- I think we're on the same page now.

I don't know what your plans were for 0.12, all I'd like to avoid is that the next release is half a year out because 0.12 should address a ton of other things. Then we rather do 0.12 soon, and then 0.13. We're not running out of numbers anytime soon slightly_smiling_face

It's mostly planned to be about minor technical breaks, so this change would fit right in. A lot of the planned changes are quite easy, so unless you're especially in a hurry about releasing this fix, I'd rather like some time to work on the rest, maybe a few weeks. Certainly not going to take as long as half a year, that's for sure.

So... I remove support for 3.2 in CI here, then we merge this, and master will then head towards 0.12?

Yeah, I think that's the best way to go here 👍