Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "Upgrading from Godot 4.0 to Godot 4.1" page #7611

Merged
merged 1 commit into from
Jul 10, 2023

Conversation

raulsntos
Copy link
Member

@raulsntos raulsntos commented Jul 5, 2023

Adds a page to serves as migration guide from 4.0 to 4.1.

The page currently only contains a Breaking changes section that includes whether the changes break compatibility in GDScript and whether the change breaks binary and/or source compatibility in C#. The listed changes also link to the PR that introduced the change. GDExtension compatibility is not listed, but there's a warning at the top to mention that compat in 4.1 is completely broken.

I'm only confident about whether these changes affect C# or not, I'm not sure if I got the GDScript coverage correctly. So please check if some changes that also affect GDScript are not listed as such. I'm specifically concerned about parameters that change their type (e.g.: From Array to Array[Plane]). I think GDScript will just do the conversion implicitly, if so the change won't affect GDScript.

Also, I tried to group the changes by areas/systems, I'm sure I got some of these wrong so feel free to let me know if I need to rearrange them, rename the group titles or change the groups order.

Also, area maintainers should review the changes listed for their area.

@raulsntos raulsntos added enhancement content:new page Issues and PRs related to creation of new documentation pages for new or undocumented features area:manual Issues and PRs related to the Manual/Tutorials section of the documentation cherrypick:4.1 labels Jul 5, 2023
@raulsntos raulsntos requested a review from YuriSizov July 5, 2023 11:26
Copy link
Member

@RedworkDE RedworkDE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty much just scripting interface change, if this page should cover all breaking changes, we should also mention behavioral changes, tho I don't think we have an easy way to track these, of the top of my head I only remember the navigation and particle turbulence reworks.

https://github.com/godotengine/godot/blob/master/misc/extension_api_validation/4.0-stable.expected also has a bunch more changes to the interface.

Instead of separating changes in Binary compatible and source compatible, which is a very C# view of things, I would distinguish (at least) GDScript compatible, C# (source) compatible, C++ GDextension (source) compatible, GDExtensions (binary) compatible (also add a warning that it is completely broken between 4.0 and 4.1), and C# (binary) compatible, with the option to possibly add more columns for community supported languages, if they have different compatibility requirements than any of these.

@mhilbrunner
Copy link
Member

Building the page locally to check it out. That aside:

Instead of separating changes in Binary compatible and source compatible, which is a very C# view of things, I would distinguish (at least) GDScript compatible, C# (source) compatible, C++ GDextension (source) compatible, GDExtensions (binary) compatible (also add a warning that it is completely broken between 4.0 and 4.1), and C# (binary) compatible, with the option to possibly add more columns for community supported languages, if they have different compatibility requirements than any of these.

That seems like it would get very unwieldy fast. As this page is only focusing on the 4.0 -> 4.1 transition, I think it is fine to add a single bold warning that GDExtension compatibility is broken, for example. I also don't think the official docs should be prepared to reflect the compatibility status for third-party community language bindings.

@YuriSizov
Copy link
Contributor

Not doing a full review at the moment, but this should be made into a migration guide with the current content being only one section of it. I'm not asking @raulsntos to write all of it, we just need to make sure the page (and the file) is called appropriately and there is some short introduction before the "Breaking API changes" section.

@raulsntos
Copy link
Member Author

This is pretty much just scripting interface change, if this page should cover all breaking changes, we should also mention behavioral changes, tho I don't think we have an easy way to track these

I would love to cover behavior breaking changes as well, but I have no way to easily track those. This list was mostly generated from the errors reported by ApiCompat.

Instead of separating changes in Binary compatible and source compatible, which is a very C# view of things, I would distinguish (at least) GDScript compatible, C# (source) compatible, C++ GDextension (source) compatible, GDExtensions (binary) compatible

So you are suggesting removing the "Languages affected" column and "merging" it with the compatible columns. That makes sense since I guess the concept of binary/source compatibility does not apply to GDScript (only source compatibility I guess).

As for GDExtension, I have kept it out of the list since I think 4.1 completely breaks compat and, since it's beta, I thought this would be expected, but you are right that we should add a warning about it in this page.

I also don't think the official docs should be prepared to reflect the compatibility status for third-party community language bindings.

I agree. Also, I think third-party language bindings have their own documentation pages so they can probably document it there.

this should be made into a migration guide with the current content being only one section of it

Sure, do you want me to follow the naming from the Upgrading from Godot 3 to Godot 4 page? I would then call the page Upgrading from Godot 4.0 to Godot 4.1 and the file would be called upgrading_to_godot_4.1.rst.

@YuriSizov
Copy link
Contributor

Sure, do you want me to follow the naming from the Upgrading from Godot 3 to Godot 4 page? I would then call the page Upgrading from Godot 4.0 to Godot 4.1 and the file would be called upgrading_to_godot_4.1.rst.

Yes, please 🙏

@mhilbrunner
Copy link
Member

Just FYI, the page builds fine and the formatting seems to work, although the tables are a bit unwieldy. Once the above comments are adressed and we're sure we're happy with the URL/file name, we can fast track a merge for this and update it iteratively.

At least during an initial review, I didn't spot anything else that wasn't already adressed.

@raulsntos raulsntos changed the title Add "Breaking changes in Godot 4.1" page Add "Upgrading from Godot 4.0 to Godot 4.1" page Jul 6, 2023
@Sauermann
Copy link
Contributor

I would love to cover behavior breaking changes as well, but I have no way to easily track those.

This kind of knowledge is in the heads of the people, who wrote the PRs. I know for example of a behavioral change in one of my PRs. What would be the best way to gather and publish them?

@YuriSizov
Copy link
Contributor

I would love to cover behavior breaking changes as well, but I have no way to easily track those.

This kind of knowledge is in the heads of the people, who wrote the PRs. I know for example of a behavioral change in one of my PRs. What would be the best way to gather and publish them?

I think we should start with a skeleton of the page with whatever info we already have, and then you can PR a suggestion for the stuff that you know needs a mention. The docs team will help you shape and phrase and format it.

@dsnopek
Copy link
Contributor

dsnopek commented Jul 6, 2023

I think it is fine to add a single bold warning that GDExtension compatibility is broken

Is there a good place to add details about the source code changes needed to update a GDExtension from 4.0 to 4.1? I'd be happy to make a follow-up PR to add it to this new page, if that is the appropriate place?

@YuriSizov
Copy link
Contributor

@dsnopek Yes, that would be a good place to add it.

@felbecker
Copy link

For me personally it would help to have a section about the new thread guards and how they affect custom thread usage. There seems to be some confusion about it. In particular, it would be helpful to have guidelines under which conditions to use set_thread_safety_checks_enabled(false) and when to debug the thread guard errors instead.

@raulsntos
Copy link
Member Author

Since 4.1 adds thread guards to a lot of APIs, it's a good idea to add a note about it in the migration page. Note that we already have a documentation page about thread safety: Thread-safe APIs.

@felbecker
Copy link

Thats true. I remember that I checked this page some time ago not finding the information I was looking for there. In this case it is a good idea to update Thread-safe APIs and just link to it in the migration guidelines instead.

@raulsntos
Copy link
Member Author

I have reviewed the 4.0-stable.expected file mentioned by @RedworkDE.

I didn't add any of the breaking changes that only change the const modifier to the list. AFAIK, this kind of change only affects GDExtension which already has a warning about completely breaking compatibility at the top of the page.

There were a couple of API changes that weren't in the list, I'm not sure why ApiCompat didn't complain about them1 but they are definitely breaking changes so I have added them to the list.

I include commentary here about each of the changes mentioned in the 4.0-stable.expected file and whether I include them or not and why:

GH-78517

Since these changes are just a serialization bug and the actual API didn't change.
I didn't add these to the breaking changes list.

GH-78237

This was missing from the breaking changes list, I'm not sure why ApiCompat didn't complain about it1.
I have added it to the list.

GH-77757

Only the const modifier changed. I didn't consider this a breaking change so I didn't add it to the list.

GH-74736

This property seems to have already been generated with the correct type in C#, unsure if this affects GDScript.
I haven't added it to the breaking changes list.

GH-74671

I don't think native structs are exposed to scripting, at least they don't seem to be generated in C#.
Types that end with Extension are usually meant to be consumed from GDExtension which is already mentioned as breaking compatibility everywhere, so I didn't add this to the breaking changes list.

GH-74600

Since the API doesn't seem to have changed and would only affect GDExtension, which is already mentioned as breaking compatibility everywhere. I didn't add these to the breaking changes list.

GH-76413

Already in the breaking changes list.

GH-69988

Already in the breaking changes list.

GH-?????

These types don't seem to have been exposed to scripting, they are definitely not generated in C#.
Since they are not exposed, I didn't add these to the breaking changes list.

GH-76176

Only the const modifier changed. I didn't consider this a breaking change so I didn't add it to the list.
Already in the list (although I think the changes seem to be listed under GH-76026).

GH-76026

Only the const modifier changed. I didn't consider this a breaking change so I didn't add it to the list.

GH-76418

These were missing from the breaking changes list, I'm not sure why ApiCompat didn't complain about them1.
I have added them to the list.

GH-72749

These were missing from the breaking changes list, I'm not sure why ApiCompat didn't complain about them1.
I have added them to the list.

GH-72152

Already in the breaking changes list.

GH-75759

Already in the breaking changes list.

GH-75017

Already in the breaking changes list.

GH-76794

Already in the breaking changes list.

GH-75777

Only the const modifier changed. I didn't consider this a breaking change so I didn't add it to the list.

GH-75250 & GH-76401

Already in the breaking changes list.

GH-77143

This was missing from the breaking changes list, I'm not sure why ApiCompat didn't complain about it1.
I have added it to the list.

GH-64628

Already in the breaking changes list.

GH-75746

Already in the breaking changes list.

GH-74242

This method contains a pointer parameter so it's not exposed in C# (see godotengine/godot#71535).
Types that end with Extension are usually meant to be consumed from GDExtension which is already mentioned as breaking compatibility everywhere, so I didn't add this to the breaking changes list.

GH-74707

This type is not exposed to C# (see godotengine/godot#59286).
Types that end with Extension are usually meant to be consumed from GDExtension which is already mentioned as breaking compatibility everywhere, so I didn't add this to the breaking changes list.

GH-72842

Already in the breaking changes list.

GH-77413

It seems like this property was already exposed with the "Skin" type, at least in C#.
Since there's no change I didn't add it to the breaking changes list.

GH-76082

The changes to Basis and Transform3D weren't made in C# (see godotengine/godot#79082).
Since it's an optional parameter, it doesn't break GDScript either.
I added these changes to the breaking changes list but it doesn't break any of the listed languages.

The changes to Node3D were already in the breaking changes list.

GH-77411

Only the const modifier changed. I didn't consider this a breaking change so I didn't add it to the list.

GH-75260

These were missing from the breaking changes list, I'm not sure why ApiCompat didn't complain about them1.
I have added them to the list.

GH-76688

Already in the breaking changes list.

Footnotes

  1. Specifically, it seems to be methods where the only thing that changes is the return type. I'm not sure if this isn't supported by ApiCompat yet or if I misconfigured it. 2 3 4 5 6

@Syogren
Copy link

Syogren commented Jul 9, 2023

My apologizes if you have already answered this question, but after making a copy of my 4.0 project and trying to import it into 4.1, it appears that both the Git Plugin and the Collada importer have stopped working properly.

Specifically, I am getting the following error for the Git plugin:
Failed loading resource: res://addons/godot-git-plugin/git_plugin.gdextension. Make sure resources have been imported by opening the project in the editor at least once.

Reinstalling it does not seem to work, as it then complains that it conflicts with several of my files.

As for the Collada importer not working, I'm not sure if it's actually broken or if the test .dae files i imported are just broken and 4.0 just so happened not to notice. That one isn't really a high priority issue and I don't have any valuable .dae files in my project, but it may be a concern for anyone who does?

@dsnopek
Copy link
Contributor

dsnopek commented Jul 10, 2023

My apologizes if you have already answered this question, but after making a copy of my 4.0 project and trying to import it into 4.1, it appears that both the Git Plugin and the Collada importer have stopped working properly.

GDExtensions compiled for Godot 4.0 aren't compatible with Godot 4.1 - a tiny change to the source code is required, and they need to be recompiled. As soon as this PR is merged, I'm planning to make a follow-up PR that adds detailed information on what folks need to do to get their GDExtensions working in 4.1.

@mhilbrunner mhilbrunner merged commit 3c2412d into godotengine:master Jul 10, 2023
1 check passed
@mhilbrunner
Copy link
Member

Thank you! Merged, a solid start for this page 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation cherrypick:4.0 content:new page Issues and PRs related to creation of new documentation pages for new or undocumented features enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants