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

Retain configuration meta data on ProjectSettings #67579

Closed
wants to merge 1 commit into from

Conversation

BastiaanOlij
Copy link
Contributor

This is a brute force fix for #25661 (and similar bug reports). It's probably good enough but it's worth having a discussion whether we want to make much more work out of this.

The core problem here is that when we select a project from the list and do something with it, we instantiate a new instance of ProjectSettings. At this point in time that projects settings takes over as the singleton in memory and we loose access to the original singleton created at startup that contains our full set of registered settings especially now that we only save settings that the user has changed from default.

This results in two sets of behaviors:

  1. when we popup dialogs we check for settings but we're now taking settings from the last loaded project files, not the settings for our object for our project manager
  2. we get a lot of error spam as when we access settings that aren't present because they were left on default and never saved

(and then a 3rd potential issue is that we could destroy our new ProjectSettings object and have our singleton pointer becomes a nullptr)

A brute way to solve this is to check if we already have a singleton in our constructor, make a copy of our properties and set everything to default, this is what I'm doing here but it only solves issue number 2 and not in a consistent way.

The nice way to solve it would be that only our object constructed in main becomes our singleton, and the additional objects don't, but our whole API for accessing settings is based around our singleton access. This would be a major undertaking to ensure that our macros are only used for accessing the settings loaded at startup, and any access to settings within the class itself and when the project manager access settings for other projects, we do not rely on the singleton logic.

We could still merge this as an interim solution as it makes nothing worse than it already is, and look at doing the work to make it nicer some time in the future.

@BastiaanOlij BastiaanOlij added this to the 4.0 milestone Oct 18, 2022
@BastiaanOlij BastiaanOlij self-assigned this Oct 18, 2022
@BastiaanOlij BastiaanOlij requested a review from a team as a code owner October 18, 2022 14:49
@akien-mga
Copy link
Member

akien-mga commented Nov 29, 2022

I think this is the wrong approach. If I understand the issue correctly, it happens only in the Project Manager's "Rename" dialog, due to very hacky code handling MODE_RENAME:

editor/project_manager.cpp
439:                    ProjectSettings *current = memnew(ProjectSettings);
697:                    ProjectSettings *current = memnew(ProjectSettings);

There's no reason to instantiate a new ProjectSettings here.

We should fix the Project Manager so that it doesn't do that (and ProjectSettings's constructor should have an ERR_FAIL_COND(singleton != nullptr); to prevent such hacky code in the future).

@YuriSizov
Copy link
Contributor

You could probably use ConfigFile for the rename functionality.

@akien-mga
Copy link
Member

Superseded by #69338.

@BastiaanOlij
Copy link
Contributor Author

Thanks @akien-mga , indeed I wasn't sure about the fix so good to see we have something better :)

@BastiaanOlij BastiaanOlij deleted the retain_config branch December 4, 2022 23:39
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.

3 participants