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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e46c07
Fix 3to4 conversions (thread arguments, OS.window_fullscreen and await)
and3rson Jul 21, 2022
ffa8d0a
Add 3to4 conversion for intersect_ray and intersect_point
and3rson Jul 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
220 changes: 207 additions & 13 deletions editor/project_converter_3_to_4.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@and3rson and3rson Jul 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@and3rson and3rson Jul 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ))


valid = valid & test_conversion_single_additional("\tvar aa = roman(r.move_and_slide( a, b, c, d, e, f )) # Roman", "\tr.set_motion_velocity(a)\n\tr.set_up_direction(b)\n\tr.set_floor_stop_on_slope_enabled(c)\n\tr.set_max_slides(d)\n\tr.set_floor_max_angle(e)\n\t# TODOConverter40 infinite_inertia were removed in Godot 4.0 - previous value `f`\n\tvar aa = roman(r.move_and_slide()) # Roman", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");
valid = valid & test_conversion_single_additional("\tvar aa = roman(r.move_and_slide_with_snap( a, g, b, c, d, e, f )) # Roman", "\tr.set_motion_velocity(a)\n\t# TODOConverter40 looks that snap in Godot 4.0 is float, not vector like in Godot 3 - previous value `g`\n\tr.set_up_direction(b)\n\tr.set_floor_stop_on_slope_enabled(c)\n\tr.set_max_slides(d)\n\tr.set_floor_max_angle(e)\n\t# TODOConverter40 infinite_inertia were removed in Godot 4.0 - previous value `f`\n\tvar aa = roman(r.move_and_slide()) # Roman", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");

valid = valid & test_conversion_single_additional("\tvar aa = state.intersect_ray( a, b, c, d, e, f )", "\tvar ray_query = PhysicsRayQueryParameters2D.new() # TODOConverter40 no idea if this is 2D or 3D ray, you need to figure it out yourself!\n\tray_query.from = a\n\tray_query.to = b\n\tray_query.exclude = c\n\tray_query.collision_mask = d\n\tray_query.collide_with_bodies = e\n\tray_query.collide_with_areas = f\n\tvar aa = state.intersect_ray(ray_query)", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");
valid = valid & test_conversion_single_additional("\tvar aa = state.intersect_point( a, b, c, d, e, f )", "\tvar point_query = PhysicsPointQueryParameters2D.new() # TODOConverter40 no idea if this is 2D or 3D point, you need to figure it out yourself!\n\tpoint_query.position = a\n\tpoint_query.exclude = c\n\tpoint_query.collision_mask = d\n\tpoint_query.collide_with_bodies = e\n\tpoint_query.collide_with_areas = f\n\tvar aa = state.intersect_point(point_query, b)", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");

valid = valid & test_conversion_single_additional("list_dir_begin( a , b )", "list_dir_begin() # TODOGODOT4 fill missing arguments https://github.com/godotengine/godot/pull/40547", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");
valid = valid & test_conversion_single_additional("list_dir_begin( a )", "list_dir_begin() # TODOGODOT4 fill missing arguments https://github.com/godotengine/godot/pull/40547", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");
valid = valid & test_conversion_single_additional("list_dir_begin( )", "list_dir_begin() # TODOGODOT4 fill missing arguments https://github.com/godotengine/godot/pull/40547", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");
Expand Down Expand Up @@ -2015,7 +2018,7 @@ bool ProjectConverter3To4::test_conversion() {

valid = valid & test_conversion_single_additional("get_node(@", "get_node(", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");

valid = valid & test_conversion_single_additional("yield(this, \"timeout\")", "await this.timeout", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");
valid = valid & test_conversion_single_additional("yield(this, \\\"timeout\\\")", "await this.timeout", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");

valid = valid & test_conversion_single_additional(" Transform.xform(Vector3(a,b,c)) ", " Transform * Vector3(a,b,c) ", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");
valid = valid & test_conversion_single_additional(" Transform.xform_inv(Vector3(a,b,c)) ", " Vector3(a,b,c) * Transform ", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");
Expand Down Expand Up @@ -2043,7 +2046,8 @@ bool ProjectConverter3To4::test_conversion() {
valid = valid & test_conversion_single_additional("'.a'", "'.a'", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");
valid = valid & test_conversion_single_additional("\t._input(_event)", "\tsuper._input(_event)", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");

valid = valid & test_conversion_single_additional("(start(A,B,C,D,E,F,G) != OK):", "(start(A,Callable(B,C),D,E,F,G) != OK):", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");
valid = valid & test_conversion_single_additional("(start(A,B) != OK):", "(start(Callable(A,B)) != OK):", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");
valid = valid & test_conversion_single_additional("(start(A,B,C,D,E,F,G) != OK):", "(start(Callable(A,B).bind(C,D,E,F,G)) != OK):", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");
valid = valid & test_conversion_single_additional("(connect(A,B,C,D,E,F,G) != OK):", "(connect(A,Callable(B,C),D,E,F,G) != OK):", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");
valid = valid & test_conversion_single_additional("(connect(A,B,C) != OK):", "(connect(A,Callable(B,C)) != OK):", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");
valid = valid & test_conversion_single_additional("disconnect(A,B,C) != OK):", "disconnect(A,Callable(B,C)) != OK):", &ProjectConverter3To4::rename_gdscript_functions, "custom rename");
Expand Down Expand Up @@ -2706,7 +2710,7 @@ void ProjectConverter3To4::rename_gdscript_functions(String &file_content) {
line = reg_setget_get.sub(line, "var $1$2:\n\tget:\n\t\treturn $1 # TODOConverter40 Copy here content of $3 \n\tset(mod_value):\n\t\tmod_value # TODOConverter40 Non existent set function", true);

// OS.window_fullscreen = true -> ProjectSettings.set("display/window/size/fullscreen",true)
line = reg_os_fullscreen.sub(line, "ProjectSettings.set(\"display/window/size/fullscreen\", $1)", true);
line = reg_os_fullscreen.sub(line, "ProjectSettings.set(\\\"display/window/size/fullscreen\\\", $1)", true);

// -- r.move_and_slide( a, b, c, d, e ) -> r.set_motion_velocity(a) ... r.move_and_slide() KinematicBody
if (line.find("move_and_slide(") != -1) {
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

This will change after #61918 is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

if (line.find("intersect_ray(") != -1) {
int start = line.find("intersect_ray(");
int end = get_end_parenthess(line.substr(start)) + 1;
if (end > -1) {
String base_obj = get_object_of_execution(line.substr(0, start));
String starting_space = get_starting_space(line);

Vector<String> parts = parse_arguments(line.substr(start, end));
if (parts.size() >= 2) {
String line_new;

line_new += starting_space + "var ray_query = PhysicsRayQueryParameters2D.new() # TODOConverter40 no idea if this is 2D or 3D ray, you need to figure it out yourself!\n";

// from & to
line_new += starting_space + "ray_query.from = " + parts[0] + "\n";
line_new += starting_space + "ray_query.to = " + parts[1] + "\n";

// exclude
if (parts.size() >= 3) {
line_new += starting_space + "ray_query.exclude = " + parts[2] + "\n";
}

// collision_layer
if (parts.size() >= 4) {
line_new += starting_space + "ray_query.collision_mask = " + parts[3] + "\n";
}

// collide_with_bodies
if (parts.size() >= 5) {
line_new += starting_space + "ray_query.collide_with_bodies = " + parts[4] + "\n";
}

// collide_with_areas
if (parts.size() >= 6) {
line_new += starting_space + "ray_query.collide_with_areas = " + parts[5] + "\n";
}

line = line_new + line.substr(0, start) + "intersect_ray(ray_query)" + line.substr(end + start);
}
}
}

// -- r.intersect_point( a, b, c, d, e ) -> var point_query = ... point_query.position = position ... r.intersect_point(point_query) PhysicsDirectSpaceState2D & PhysicsDirectSpaceState3D
if (line.find("intersect_point(") != -1) {
int start = line.find("intersect_point(");
int end = get_end_parenthess(line.substr(start)) + 1;
if (end > -1) {
String base_obj = get_object_of_execution(line.substr(0, start));
String starting_space = get_starting_space(line);

Vector<String> parts = parse_arguments(line.substr(start, end));
if (parts.size() >= 1) {
String line_new;

String max_results;

line_new += starting_space + "var point_query = PhysicsPointQueryParameters2D.new() # TODOConverter40 no idea if this is 2D or 3D point, you need to figure it out yourself!\n";

// position
line_new += starting_space + "point_query.position = " + parts[0] + "\n";

// max_results
if (parts.size() >= 2) {
max_results = ", " + parts[1];
}

// exclude
if (parts.size() >= 3) {
line_new += starting_space + "point_query.exclude = " + parts[2] + "\n";
}

// collision_layer
if (parts.size() >= 4) {
line_new += starting_space + "point_query.collision_mask = " + parts[3] + "\n";
}

// collide_with_bodies
if (parts.size() >= 5) {
line_new += starting_space + "point_query.collide_with_bodies = " + parts[4] + "\n";
}

// collide_with_areas
if (parts.size() >= 6) {
line_new += starting_space + "point_query.collide_with_areas = " + parts[5] + "\n";
}

line = line_new + line.substr(0, start) + "intersect_point(point_query" + max_results + ")" + line.substr(end + start);
}
}
}

// -- sort_custom( a , b ) -> sort_custom(Callable( a , b )) Object
if (line.find("sort_custom(") != -1) {
int start = line.find("sort_custom(");
Expand Down Expand Up @@ -2862,7 +2958,7 @@ void ProjectConverter3To4::rename_gdscript_functions(String &file_content) {
if (end > -1) {
Vector<String> parts = parse_arguments(line.substr(start, end));
if (parts.size() == 2) {
line = line.substr(0, start) + "await " + parts[0] + "." + parts[1].replace("\"", "").replace("\'", "").replace(" ", "") + line.substr(end + start);
line = line.substr(0, start) + "await " + parts[0] + "." + parts[1].replace("\\\"", "").replace("\'", "").replace(" ", "") + line.substr(end + start);
}
}
}
Expand Down Expand Up @@ -2941,14 +3037,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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if (line.find("start(") != -1) {
int start = line.find("start(");
int end = get_end_parenthess(line.substr(start)) + 1;
if (end > -1) {
Vector<String> parts = parse_arguments(line.substr(start, end));
if (parts.size() >= 3) {
line = line.substr(0, start) + "start(" + parts[0] + ",Callable(" + parts[1] + "," + parts[2] + ")" + connect_arguments(parts, 3) + ")" + line.substr(end + start);
if (parts.size() == 2) {
line = line.substr(0, start) + "start(Callable(" + parts[0] + "," + parts[1] + "))" + line.substr(end + start);
} else if (parts.size() >= 3) {
line = line.substr(0, start) + "start(Callable(" + parts[0] + "," + parts[1] + ").bind(" + connect_arguments(parts, 2).substr(1) + "))" + line.substr(end + start);
}
}
}
Expand Down Expand Up @@ -3148,7 +3247,7 @@ Vector<String> ProjectConverter3To4::check_for_rename_gdscript_functions(Vector<
line = reg_setget_get.sub(line, "var $1$2:\n\tget:\n\t\treturn $1 # TODOConverter40 Copy here content of $3 \n\tset(mod_value):\n\t\tmod_value # TODOConverter40 Non existent set function", true);

// OS.window_fullscreen = true -> ProjectSettings.set("display/window/size/fullscreen",true)
line = reg_os_fullscreen.sub(line, "ProjectSettings.set(\"display/window/size/fullscreen\", $1)", true);
line = reg_os_fullscreen.sub(line, "ProjectSettings.set(\\\"display/window/size/fullscreen\\\", $1)", true);

// -- r.move_and_slide( a, b, c, d, e ) -> r.set_motion_velocity(a) ... r.move_and_slide() KinematicBody
if (line.find("move_and_slide(") != -1) {
Expand Down Expand Up @@ -3245,6 +3344,98 @@ Vector<String> ProjectConverter3To4::check_for_rename_gdscript_functions(Vector<
}
}

// -- r.intersect_ray( a, b, c, d, e ) -> var ray_query = ... ray_query.from = from ... r.intersect_ray(ray_query) PhysicsDirectSpaceState2D & PhysicsDirectSpaceState3D
if (line.find("intersect_ray(") != -1) {
int start = line.find("intersect_ray(");
int end = get_end_parenthess(line.substr(start)) + 1;
if (end > -1) {
String base_obj = get_object_of_execution(line.substr(0, start));
String starting_space = get_starting_space(line);

Vector<String> parts = parse_arguments(line.substr(start, end));
if (parts.size() >= 2) {
String line_new;

line_new += starting_space + "var ray_query = PhysicsRayQueryParameters2D.new() # TODOConverter40 no idea if this is 2D or 3D ray, you need to figure it out yourself!\n";

// from & to
line_new += starting_space + "ray_query.from = " + parts[0] + "\n";
line_new += starting_space + "ray_query.to = " + parts[1] + "\n";

// exclude
if (parts.size() >= 3) {
line_new += starting_space + "ray_query.exclude = " + parts[2] + "\n";
}

// collision_layer
if (parts.size() >= 4) {
line_new += starting_space + "ray_query.collision_mask = " + parts[3] + "\n";
}

// collide_with_bodies
if (parts.size() >= 5) {
line_new += starting_space + "ray_query.collide_with_bodies = " + parts[4] + "\n";
}

// collide_with_areas
if (parts.size() >= 6) {
line_new += starting_space + "ray_query.collide_with_areas = " + parts[5] + "\n";
}

line = line_new + line.substr(0, start) + "intersect_ray(ray_query)" + line.substr(end + start);
}
}
}

// -- r.intersect_point( a, b, c, d, e ) -> var point_query = ... point_query.position = position ... r.intersect_point(point_query) PhysicsDirectSpaceState2D & PhysicsDirectSpaceState3D
if (line.find("intersect_point(") != -1) {
int start = line.find("intersect_point(");
int end = get_end_parenthess(line.substr(start)) + 1;
if (end > -1) {
String base_obj = get_object_of_execution(line.substr(0, start));
String starting_space = get_starting_space(line);

Vector<String> parts = parse_arguments(line.substr(start, end));
if (parts.size() >= 1) {
String line_new;

String max_results;

line_new += starting_space + "var point_query = PhysicsPointQueryParameters2D.new() # TODOConverter40 no idea if this is 2D or 3D point, you need to figure it out yourself!\n";

// position
line_new += starting_space + "point_query.position = " + parts[0] + "\n";

// max_results
if (parts.size() >= 2) {
max_results = ", " + parts[1];
}

// exclude
if (parts.size() >= 3) {
line_new += starting_space + "point_query.exclude = " + parts[2] + "\n";
}

// collision_layer
if (parts.size() >= 4) {
line_new += starting_space + "point_query.collision_mask = " + parts[3] + "\n";
}

// collide_with_bodies
if (parts.size() >= 5) {
line_new += starting_space + "point_query.collide_with_bodies = " + parts[4] + "\n";
}

// collide_with_areas
if (parts.size() >= 6) {
line_new += starting_space + "point_query.collide_with_areas = " + parts[5] + "\n";
}

line = line_new + line.substr(0, start) + "intersect_point(point_query" + max_results + ")" + line.substr(end + start);
}
}
}

// -- sort_custom( a , b ) -> sort_custom(Callable( a , b )) Object
if (line.find("sort_custom(") != -1) {
int start = line.find("sort_custom(");
Expand Down Expand Up @@ -3295,7 +3486,7 @@ Vector<String> ProjectConverter3To4::check_for_rename_gdscript_functions(Vector<
if (end > -1) {
Vector<String> parts = parse_arguments(line.substr(start, end));
if (parts.size() == 2) {
line = line.substr(0, start) + "await " + parts[0] + "." + parts[1].replace("\"", "").replace("\'", "").replace(" ", "") + line.substr(end + start);
line = line.substr(0, start) + "await " + parts[0] + "." + parts[1].replace("\\\"", "").replace("\'", "").replace(" ", "") + line.substr(end + start);
}
}
}
Expand Down Expand Up @@ -3370,14 +3561,17 @@ Vector<String> ProjectConverter3To4::check_for_rename_gdscript_functions(Vector<
}
}
}
// -- 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
if (line.find("start(") != -1) {
int start = line.find("start(");
int end = get_end_parenthess(line.substr(start)) + 1;
if (end > -1) {
Vector<String> parts = parse_arguments(line.substr(start, end));
if (parts.size() >= 3) {
line = line.substr(0, start) + "start(" + parts[0] + ",Callable(" + parts[1] + "," + parts[2] + ")" + connect_arguments(parts, 3) + ")" + line.substr(end + start);
if (parts.size() == 2) {
line = line.substr(0, start) + "start(Callable(" + parts[0] + "," + parts[1] + "))" + line.substr(end + start);
} else if (parts.size() >= 3) {
line = line.substr(0, start) + "start(Callable(" + parts[0] + "," + parts[1] + ").bind(" + connect_arguments(parts, 2).substr(1) + ")" + line.substr(end + start);
}
}
}
Expand Down