Fix 3to4 conversions (thread arguments, OS.window_fullscreen and await) #63299

and3rson · 2022-07-21T20:24:24Z

Fixes --convert-3to4 incorrectly escaping quotes when using signal yields #63291
Fixes --convert-3to4 messes up thread arguments #63292
Fixes --convert-3to4 does not escape quotes when migrating params to ProjectSettings.set(...) and breaks scene file #63293

and3rson · 2022-07-21T20:25:00Z

Tagging @qarmin as per Calinou's comments

kleonc · 2022-07-21T20:49:09Z

Fixes #63291, #63292, #63293

Note you'd need to write "Fixes #issue_number" separately for each issue to make all of them be auto-linked to be closed by this PR. As above, only the first one was auto-linked (I've linked the other two manually).

Fixes godotengine#63291 Fixes godotengine#63292 Fixes godotengine#63293

and3rson · 2022-07-21T21:12:31Z

FYI: I've also added converters for intersect_ray and intersect_shape since my project uses them extensively :)

and3rson · 2022-07-21T21:14:43Z

Fixes #63291, #63292, #63293

Note you'd need to write "Fixes #issue_number" separately for each issue to make all of them be auto-linked to be closed by this PR. As above, only the first one was auto-linked (I've linked the other two manually).

Gotcha. Done!

kleonc · 2022-07-21T21:48:11Z

Gotcha. Done!

I was referring to the PR description, not the commit message (which I see you've changed). I didn't notice you had the same "Fixes ..." text as the commit message, now I see why you could have interpreted it like this.
These keywords in the commit message will result in closing the linked issues when the commit is merged but the issues won't be linked to this PR (PR containing the merged commit). Hence you rather want to link the issues in the PR description. Here you can read more details on how this linking works.
Also you didn't need to change anything, as I've mentioned previously I've already linked the other two issues manually. It was just a note for the future. I guess I could have been a little more precise. 🙂

KoBeWi · 2022-07-21T22:00:28Z

editor/project_converter_3_to_4.cpp

@@ -2803,6 +2807,98 @@ void ProjectConverter3To4::rename_gdscript_functions(String &file_content) {
 			}
 		}

+		// -- r.intersect_ray( a, b, c, d, e )  ->  var ray_query = ... ray_query.from = from ... r.intersect_ray(ray_query)         PhysicsDirectSpaceState2D & PhysicsDirectSpaceState3D


This will change after #61918 is merged.

Great, didn't know there's a WIP for this. It surely will be much more convenient to have it as a one-liner rather than the current method of creating a separate object. Should we disregard this conversion and wait for #61918?

qarmin · 2022-07-21T22:17:07Z

editor/project_converter_3_to_4.cpp

@@ -1968,11 +1968,14 @@ bool ProjectConverter3To4::test_conversion() {
 	valid = valid & test_conversion_single_additional("(Disconnect(A,B,C) != OK):", "(Disconnect(A,new Callable(B,C)) != OK):", &ProjectConverter3To4::rename_csharp_functions, "custom rename csharp");
 	valid = valid & test_conversion_single_additional("(IsConnected(A,B,C) != OK):", "(IsConnected(A,new Callable(B,C)) != OK):", &ProjectConverter3To4::rename_csharp_functions, "custom rename");

-	valid = valid & test_conversion_single_additional("OS.window_fullscreen = Settings.fullscreen", "ProjectSettings.set(\"display/window/size/fullscreen\", Settings.fullscreen)", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");
+	valid = valid & test_conversion_single_additional("OS.window_fullscreen = Settings.fullscreen", "ProjectSettings.set(\\\"display/window/size/fullscreen\\\", Settings.fullscreen)", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");


The problem happens probably only in builtin scripts, so should be fixed only there - otherwise it will broke normal .gd files(not tested yet)

Built-in scripts should be unescaped before conversion, if they aren't already.

My bad, I've totally forgotten that scripts should not be unescaped since they're plaintext. Still, I cannot confirm that built-in scripts are unescaped, i. e. @KoBeWi I can still observe #63291 in the latest master. But I agree that the best solution would be to unescape all built-in scripts before conversion.

What's the difference between rename_gdscript_functions and check_for_rename_gdscript_functions btw? Their code seems almost identical (except for the return value).

EDIT: I can see there's a check in project_converter_3_to_4.cpp:1665 which differentiates .gd and .tscn files. Should we probably introduce an extra argument to rename_gdscript_functions such as is_builtin? This way we could control the behavior of quote escaping inside rename_gdscript_functions. It would also make sense to call rename_gdscript_functions inside the .tres branch several lines below (since resources may also contain built-in scripts).

check_for_rename_gdscript_functions additionally prints exactly in which line change will happen.
At beginning it was very different than rename_gdscript_functions, but I unified this one day, so there is a lot of duplicated code, which could be extracted to another function, but when I tried to do it I failed due some strange Regex memory leaks(this shouldn't be too hard to fix)

I think that adding is_builtin is good solution, but this probably will be only needed by custom renames that use " inside regex e.g.

OS.window_fullscreen = true -> ProjectSettings.set("display/window/size/fullscreen",true)

but not with

sort_custom( a , b ) -> sort_custom(Callable( a , b ))

and3rson · 2022-07-22T18:24:32Z

Gotcha. Done!

I was referring to the PR description, not the commit message (which I see you've changed). I didn't notice you had the same "Fixes ..." text as the commit message, now I see why you could have interpreted it like this. These keywords in the commit message will result in closing the linked issues when the commit is merged but the issues won't be linked to this PR (PR containing the merged commit). Hence you rather want to link the issues in the PR description. Here you can read more details on how this linking works. Also you didn't need to change anything, as I've mentioned previously I've already linked the other two issues manually. It was just a note for the future. I guess I could have been a little more precise. slightly_smiling_face

Oh, I see now. Thank you for taking time to explain!

jtnicholl · 2022-07-30T22:07:50Z

editor/project_converter_3_to_4.cpp

@@ -2941,14 +2942,17 @@ void ProjectConverter3To4::rename_gdscript_functions(String &file_content) {
 				}
 			}
 		}
-		// -- start(a,b,c) -> start(a,Callable(b,c))      Thread
+		// -- start(a,b) -> start(Callable(a,b))      Thread
+		// -- start(a,b,c,d) -> start(Callable(a,b).bind(c,d))      Thread


Threads in 3.x can take only one userdata argument. start(a, b, c, d) should become start(Callable(a, b).bind(c), d), with d being the Thread priority.

akien-mga · 2022-08-05T22:22:28Z

Superseded by #63887.

and3rson force-pushed the convert3to4-fixes branch from 99d4285 to 5bd4e34 Compare July 21, 2022 20:26

kleonc added bug topic:editor labels Jul 21, 2022

kleonc added this to the 4.0 milestone Jul 21, 2022

This was linked to issues Jul 21, 2022

--convert-3to4 messes up thread arguments #63292

Closed

--convert-3to4 does not escape quotes when migrating params to ProjectSettings.set(...) and breaks scene file #63293

Closed

Fix 3to4 conversions (thread arguments, OS.window_fullscreen and await)

2e46c07

Fixes godotengine#63291 Fixes godotengine#63292 Fixes godotengine#63293

Add 3to4 conversion for intersect_ray and intersect_point

ffa8d0a

and3rson force-pushed the convert3to4-fixes branch from c1f1c22 to ffa8d0a Compare July 21, 2022 21:14

KoBeWi reviewed Jul 21, 2022

View reviewed changes

qarmin requested changes Jul 21, 2022

View reviewed changes

jtnicholl reviewed Jul 30, 2022

View reviewed changes

qarmin mentioned this pull request Aug 3, 2022

Various converter enhancements and bugfixes #63887

Merged

akien-mga closed this Aug 5, 2022

akien-mga added the archived label Aug 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 3to4 conversions (thread arguments, OS.window_fullscreen and await) #63299

Fix 3to4 conversions (thread arguments, OS.window_fullscreen and await) #63299

and3rson commented Jul 21, 2022 •

edited by akien-mga

Loading

and3rson commented Jul 21, 2022 •

edited

Loading

kleonc commented Jul 21, 2022

and3rson commented Jul 21, 2022

and3rson commented Jul 21, 2022

kleonc commented Jul 21, 2022

KoBeWi Jul 21, 2022

and3rson Jul 22, 2022

qarmin Jul 21, 2022

KoBeWi Jul 21, 2022

and3rson Jul 22, 2022 •

edited

Loading

and3rson Jul 23, 2022 •

edited

Loading

qarmin Jul 25, 2022

and3rson commented Jul 22, 2022

jtnicholl Jul 30, 2022

akien-mga commented Aug 5, 2022

Fix 3to4 conversions (thread arguments, OS.window_fullscreen and await) #63299

Fix 3to4 conversions (thread arguments, OS.window_fullscreen and await) #63299

Conversation

and3rson commented Jul 21, 2022 • edited by akien-mga Loading

and3rson commented Jul 21, 2022 • edited Loading

kleonc commented Jul 21, 2022

and3rson commented Jul 21, 2022

and3rson commented Jul 21, 2022

kleonc commented Jul 21, 2022

KoBeWi Jul 21, 2022

Choose a reason for hiding this comment

and3rson Jul 22, 2022

Choose a reason for hiding this comment

qarmin Jul 21, 2022

Choose a reason for hiding this comment

KoBeWi Jul 21, 2022

Choose a reason for hiding this comment

and3rson Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

and3rson Jul 23, 2022 • edited Loading

Choose a reason for hiding this comment

qarmin Jul 25, 2022

Choose a reason for hiding this comment

and3rson commented Jul 22, 2022

jtnicholl Jul 30, 2022

Choose a reason for hiding this comment

akien-mga commented Aug 5, 2022

and3rson commented Jul 21, 2022 •

edited by akien-mga

Loading

and3rson commented Jul 21, 2022 •

edited

Loading

and3rson Jul 22, 2022 •

edited

Loading

and3rson Jul 23, 2022 •

edited

Loading