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

Remove duplicate editor settings definitions #58847

Merged
merged 1 commit into from
Mar 7, 2022

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Mar 6, 2022

I noticed that EDITOR_DEF is overused in many places. This is a very bad idea, because each time you use EDITOR_DEF, a cat starts crying somewhere you need to provide a default value. This leads to some ridiculous situations like a default value repeated 7 times. The actual value then depends on initialization order. Luckily I spotted only one setting that had inconsistent defaults across definitions.

My absolute favorite was this:
image
Same thing, defined twice few lines apart.

I went over the codebase and cleaned it up by removing duplicate defs of the same settings and changing them to EDITOR_GET. We have most of the settings defined in _load_defaults() method of EditorSettings, so if there were some additional EDITOR_DEFs, I left the ones in the singleton. Then I left stuff defined in a constructor over inlined settings. If some settings were on equivalent level (i.e. not apparent which one comes first), I moved the definition to _load_defaults().

We should have some policy for using EDITOR_DEF. Repeating defaults multiple times can lead to errors, but also IMO it's just dumb xd

Another things to consider could be:

  • moving all editor setting definitions to _load_defaults(), so they are all in one place
  • changing all usage of EditorSettings::get_singleton()->get() to EDITOR_GET (it's the same thing, except it displays error when setting is missing and also it's much shorter)

Also Project Settings likely need the same treatment.

@KoBeWi KoBeWi added this to the 4.0 milestone Mar 6, 2022
@KoBeWi KoBeWi requested review from a team as code owners March 6, 2022 21:02
@KoBeWi KoBeWi force-pushed the editor_settings_mess branch from 1b07f6b to 2057ea2 Compare March 6, 2022 21:06
@EricEzaM
Copy link
Contributor

EricEzaM commented Mar 7, 2022

  • moving all editor setting definitions to _load_defaults(), so they are all in one place

Yeah I have proposed this a couple of times but there was some push back from reduz I think. This is also relevant to this proposal/issue godotengine/godot-proposals#1339 (comment)

@akien-mga akien-mga merged commit 53cf5ef into godotengine:master Mar 7, 2022
@akien-mga
Copy link
Member

Thanks!

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