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

Allow loading custom ProjectSettings instance #75048

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Mar 18, 2023

This PR allows loading ProjectSettings like a normal resource (even though it's an Object...). It's accomplished using a special constructor that bypasses creating singleton etc.

This fixes an issue introduced by #69338 where renaming project would remove comments from project.godot.
Also needed for #75047, because it has the same problem.

tbh the original issue could've been fixed by a simple return if a singleton already exists. All the stuff in the constructor does not need to run for the project settings to load and save correctly.

@KoBeWi KoBeWi added this to the 4.1 milestone Mar 18, 2023
@KoBeWi KoBeWi requested a review from a team as a code owner March 18, 2023 01:43
Comment on lines 479 to 480
msg->show();
return;
Copy link
Member Author

Choose a reason for hiding this comment

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

This makes the error actually displayed. Previously it did nothing.

@YuriSizov
Copy link
Contributor

tbh the original issue could've been fixed by a simple return if a singleton already exists.

Well, the idea was to never create a singleton in the first place. What you suggest would create at least one, which is still incorrect in the context of the project manager.

Anyway, why not add support for comments to the ConfigFile? We are in no hurry anymore, we can work on a more robust solution.

@dalexeev
Copy link
Member

Anyway, why not add support for comments to the ConfigFile? We are in no hurry anymore, we can work on a more robust solution.

I had this idea a long time ago. ConfigFile shouldn't remove the comment at the top of the file, plus we can add methods like get_top_comment() and set_top_comment(). The same can be done for sections and keys, although this is probably overkill. This can be useful not only for the engine, but also for users.

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 18, 2023

Well, the idea was to never create a singleton in the first place.

Ah, that makes sense.

As for ConfigFile and comments, preserving exact comments is not that simple. get_top_comment() is only a half-solution, because inline comments are also possible (and I use them in my manually-written files).

Also ProjectSettings is not even a ConfigFile. It implements its own reading and writing methods, which just happen to serialize the same strings, because they use Variant API. The original PR mentioned other differences than the comments (which don't seem to exist anymore though).

I think my solution is good. If we want to improve ConfigFile to fix this issue, the ProjectSettings should move to using ConfigFile first.

@@ -960,7 +960,7 @@ Error ProjectSettings::save_custom(const String &p_path, const CustomMap &p_cust
}
}
// Check for the existence of a csproj file.
if (_csproj_exists(get_resource_path())) {
if (_csproj_exists(p_path.get_base_dir())) {
Copy link
Member Author

@KoBeWi KoBeWi Jun 19, 2023

Choose a reason for hiding this comment

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

Not sure what was the intention here, but get_resource_path() can point to invalid directory and _csproj_exists() crash. If the previous code was correct, I could handle it in _csproj_exists() instead.

Copy link
Member

Choose a reason for hiding this comment

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

The intention was to get the path to the directory that contains project.godot (which is res://). Maybe we can just use the literal string "res://" here and it would be handled by DirAccess.

The _csproj_exists method is just meant to be FileAccess::exists("res://*.csproj") but FileAccess::exists doesn't support globbing.

Copy link
Member Author

Choose a reason for hiding this comment

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

get_resource_path() returns the loaded path. With my change, save_custom used to save to a directory different than res:// will look for .csproj inside that directory instead of the load path. Would that be correct? (I think it doesn't affect the editor, as save_custom() is normally always used with the current path)

Copy link
Member

Choose a reason for hiding this comment

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

I feel like we always look for the .csproj in the res:// directory so looking in a different directory wouldn't be correct. Are you saying the project.godot file can be in a directory other than res://? Doesn't the editor always assume this file to be in the root project directory?

In which cases is save_custom used to save to a directory different than res://? I see that this method is exposed to scripting, is this the case you are concerned about? If so, I don't think we care about this scenario. This detection is only used to add the "C#" feature tag to be used by the Project Manager, so any usage outside of that doesn't really matter.

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked how we use save_custom and it's used only for creating new projects and for saving project file for exported project. So I guess my concerns are irrelevant.

@YuriSizov YuriSizov modified the milestones: 4.1, 4.2 Jun 20, 2023
@YuriSizov YuriSizov modified the milestones: 4.2, 4.3 Oct 30, 2023
@akien-mga akien-mga merged commit c1907f2 into godotengine:master Apr 26, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the it's_ok_to_be_loaded branch April 26, 2024 13:32
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.

5 participants