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

Project Manager: Fix hacky code for project rename #69338

Merged
merged 1 commit into from
Nov 29, 2022

Conversation

akien-mga
Copy link
Member

Instantiating a new ProjectSettings is not the way to go. ConfigFile works just fine to read/change a single value.

Fixes memory leaks as the instantiated ProjectSettings was never freed.

@akien-mga akien-mga added cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release labels Nov 29, 2022
Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

PR council approves

Instantiating a new ProjectSettings is *not* the way to go.
ConfigFile works just fine to read/change a single value.

Fixes memory leaks as the instantiated ProjectSettings was never freed.
Forbid doing this to prevent such problems.

Fixes godotengine#25661.
@akien-mga
Copy link
Member Author

akien-mga commented Nov 29, 2022

So I checked after someone mentioned it in the PR meeting, and indeed there's a potential risk of losing information by going through the ConfigFile API. Here's the diff after rename for a test project with a few global script classes:

--- project.godot.bkp   2022-11-29 15:43:40.739102066 +0100
+++ project.godot       2022-11-29 15:43:47.488103630 +0100
@@ -1,13 +1,4 @@
-; Engine configuration file.
-; It's best edited using the editor UI and not directly,
-; since the parameters that go here are not all obvious.
-;
-; Format:
-;   [section] ; section goes between []
-;   param=value ; assign values to parameters
-
 config_version=5
-
 _global_script_classes=[{
 "base": "Node",
 "class": &"MyClass",
@@ -26,7 +17,7 @@
 
 [application]
 
-config/name="New Game Project test"
+config/name="New Game Project"
 run/main_scene="res://node_2d.tscn"
 config/features=PackedStringArray("4.0")
 config/icon="res://icon.svg"

So it removes comments, and the separation line between config_version and _global_script_classes.
Those are possibly the only elements in project.godot which differ from a proper ConfigFile. Might be worth changing that...

Edit: But then when editing the project, Godot restores the comment and separation line so the diff becomes:

--- project.godot.bkp   2022-11-29 15:43:40.739102066 +0100
+++ project.godot       2022-11-29 15:46:43.913144506 +0100
@@ -26,7 +26,7 @@
 
 [application]
 
-config/name="New Game Project test"
+config/name="New Game Project"
 run/main_scene="res://node_2d.tscn"
 config/features=PackedStringArray("4.0")
 config/icon="res://icon.svg"

So it's pretty good? I don't know if there are more complex project.godot files where there's actual risk of data loss though.

@YuriSizov
Copy link
Contributor

Might be worth changing that...

I'd go with "Allow ConfigFile to preserve formatting (spacing, comments)", but that'd take some effort. I guess if someone complains, asking them to open the project once is okay.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Tested it with a realistic project and it didn't break anything else. I think it's okay as an immediate solution, though it'd be nice to preserve formatting with ConfigFile. Task for another day.

@YuriSizov YuriSizov merged commit cdd99e9 into godotengine:master Nov 29, 2022
@YuriSizov
Copy link
Contributor

Let's see if this causes problems with the next beta. Thanks for the quick turnaround!

@akien-mga akien-mga deleted the pm-fix-hacky-project-rename branch November 30, 2022 14:23
@akien-mga
Copy link
Member Author

Cherry-picked for 3.6.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Nov 30, 2022
@timothyqiu
Copy link
Member

Cherry-picked for 3.5.2

@timothyqiu timothyqiu removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A lot of warnings "Property not found:" when trying to rename project
5 participants