Add EditorStringNames singleton #80573

KoBeWi · 2023-08-13T00:40:37Z

This PR adds EditorStringNames singleton, complementary to CoreStringNames and SceneStringNames. It includes some most common theme-related StringNames: Editor, EditorFonts, EditorIcons and EditorStyles. More strings can be added later (e.g. most common types or most common constant/stylebox names etc.).

This results in lower memory usage of the editor (I didn't do any benchmark though) and slightly better performance in some cases (especially initializing the theme might be faster I think).

I also included a macro EditorStringName(name), which is a shorthand for EditorStringNames::get_singleton()->name, making the code much less verbose. We could add such macro to other StringName singletons too.

editor/editor_build_profile.cpp

editor/editor_feature_profile.cpp

editor/action_map_editor.cpp

Calinou · 2023-08-31T23:38:02Z

Benchmark

Startup/shutdown times

❯ hyperfine -iw1 "bin/godot.linuxbsd.editor.x86_64 --quit" "bin/godot.linuxbsd.editor.x86_64.master --quit"
Benchmark 1: bin/godot.linuxbsd.editor.x86_64 --quit (this PR)
  Time (mean ± σ):      2.740 s ±  0.056 s    [User: 1.390 s, System: 0.225 s]
  Range (min … max):    2.619 s …  2.841 s    10 runs
 
Benchmark 2: bin/godot.linuxbsd.editor.x86_64.master --quit
  Time (mean ± σ):      2.690 s ±  0.066 s    [User: 1.394 s, System: 0.220 s]
  Range (min … max):    2.591 s …  2.768 s    10 runs
 
Summary
  bin/godot.linuxbsd.editor.x86_64.master --quit ran
    1.02 ± 0.03 times faster than bin/godot.linuxbsd.editor.x86_64 --quit

❯ hyperfine -iw1 "bin/godot.linuxbsd.editor.x86_64 /tmp/4/project.godot --quit" "bin/godot.linuxbsd.editor.x86_64.master /tmp/4/project.godot --quit"
Benchmark 1: bin/godot.linuxbsd.editor.x86_64 /tmp/4/project.godot --quit (this PR)
  Time (mean ± σ):      4.768 s ±  0.425 s    [User: 3.847 s, System: 0.380 s]
  Range (min … max):    4.169 s …  5.171 s    10 runs
 
Benchmark 2: bin/godot.linuxbsd.editor.x86_64.master /tmp/4/project.godot --quit
  Time (mean ± σ):      4.808 s ±  0.376 s    [User: 3.851 s, System: 0.388 s]
  Range (min … max):    4.402 s …  5.173 s    10 runs
 
Summary
  bin/godot.linuxbsd.editor.x86_64 /tmp/4/project.godot --quit ran
    1.01 ± 0.12 times faster than bin/godot.linuxbsd.editor.x86_64.master /tmp/4/project.godot --quit

No meaningful difference, outside of margin of error.

Binary size

Stripped Linux x86_64 editor binary (optimize=none, no LTO):

150,776,656  godot.linuxbsd.editor.x86_64 (this PR)
151,126,640  godot.linuxbsd.editor.x86_64.master

That's a reduction of 350 KB, nice 🙂

Peak memory usage

Average of 3 runs after a warmup run, opening editor on empty project then quitting:

# Measured using: /usr/bin/time -f "%M" bin/godot.linuxbsd.editor.x86_64 /tmp/4/project.godot --quit
829,358,000 godot.linuxbsd.editor.x86_64 (this PR)
829,777,000 godot.linuxbsd.editor.x86_64.master

Memory usage seems just a tad lower, but it may be within margin of error (each run has slightly different peak memory usage, ±0.1 %). sharkdp/hyperfine#86 could help us get more reliable benchmarking if it's implemented 🙂

editor/editor_node.cpp

AThousandShips · 2023-09-03T15:43:42Z

editor/editor_node.cpp

-				} else if (singleton->gui_base->has_theme_icon(p_editor->get_name(), SNAME("EditorIcons"))) {
-					tb->set_icon(singleton->gui_base->get_theme_icon(p_editor->get_name(), SNAME("EditorIcons")));
+				} else if (singleton->gui_base->has_theme_icon(p_editor->get_name(), EditorStringName(EditorIcons))) {
+					tb->set_icon(singleton->gui_base->get_editor_theme_icon(""));


Will this actually get the correct icon with just "" as the name?

No, I autoreplaced all occurrences with the new method and seems like some of them are broken now .-.
I guess I should double-check all changed lines.

AThousandShips

Think this might need a restart and the search/replace applied more carefully

Not an exhaustive list

editor/debugger/editor_debugger_tree.cpp

editor/debugger/script_editor_debugger.cpp

editor/editor_help.cpp

editor/editor_node.cpp

editor/gui/scene_tree_editor.cpp

KoBeWi · 2023-09-03T16:18:02Z

Think this might need a restart and the search/replace applied more carefully

I used a script to do that. I don't see any other way to do a non-trivial replacement in 1600+ lines. Luckily GitHub's diff makes it easy to spot wrong replacements. It's still 176 files though -_-

AThousandShips · 2023-09-03T16:19:34Z

Meant to check different cases in the replace script, one of the issues was probably caused by greedy search, as it grabbed all between quotes somehow

AThousandShips · 2023-09-03T16:21:43Z

I'd say to use a regex to replace all the trivial cases, checking that it's exactly a string in the area, and then use some search method to identify all the cases that aren't trivial

So strictly replace get_theme_icon((SNAME("(something)"), SNAME("EditorIcons"))) automatically

scene/main/window.cpp

YuriSizov · 2023-09-04T12:16:40Z

editor/debugger/editor_visual_profiler.cpp

@@ -158,7 +159,7 @@ void EditorVisualProfiler::_update_plot() {
 	}

 	uint8_t *wr = graph_image.ptrw();
-	const Color background_color = get_theme_color("dark_color_2", "Editor");
+	const Color background_color = get_theme_color("dark_color_2", EditorStringName(Editor));


It's always tricky with these to understand if SNAME is skipped on purpose here or by mistake. 😕 Feel free to leave it as is, there are probably many more of these.

I tried to not add or remove existing SNAMEs, as it's unrelated.

YuriSizov

I won't check every file, and I think you've already identified and fixed some automation issues together. So assuming it's clean now, I'm going to approve this. (I skimmed through a few still.)

It's definitely an improvement for icons with the new method. For the rest, especially given Calinou's testing, I'm not so sure. It doesn't really improve code clarity, IMO. But it doesn't make it worse either. Perhaps we could add dedicated methods for all data types?

I'm also working on a system to declare theme items per class which, together with theme cache, should reduce the need for this. But I think we can go for it for now.

YuriSizov · 2023-09-06T13:12:36Z

Thanks! Now rebasing begins 😬

KoBeWi added enhancement topic:editor topic:codestyle performance labels Aug 13, 2023

KoBeWi added this to the 4.x milestone Aug 13, 2023

KoBeWi requested review from a team as code owners August 13, 2023 00:40

KoBeWi force-pushed the 2k_lines_of_changes_created_at_2AM branch from c79a212 to d48613a Compare August 13, 2023 00:43

AThousandShips reviewed Aug 13, 2023

View reviewed changes

editor/editor_build_profile.cpp Show resolved Hide resolved

AThousandShips reviewed Aug 13, 2023

View reviewed changes

editor/editor_feature_profile.cpp Show resolved Hide resolved

KoBeWi force-pushed the 2k_lines_of_changes_created_at_2AM branch from d48613a to 7cbb462 Compare August 13, 2023 09:22

KoBeWi mentioned this pull request Aug 13, 2023

Remove some control spacers from editor. #80578

Draft

akien-mga reviewed Aug 14, 2023

View reviewed changes

editor/action_map_editor.cpp Outdated Show resolved Hide resolved

KoBeWi force-pushed the 2k_lines_of_changes_created_at_2AM branch from 7cbb462 to 4759283 Compare August 31, 2023 19:30

Calinou approved these changes Aug 31, 2023

View reviewed changes

KoBeWi force-pushed the 2k_lines_of_changes_created_at_2AM branch from 4759283 to 74fb37b Compare September 3, 2023 15:04

KoBeWi requested review from a team as code owners September 3, 2023 15:04

KoBeWi force-pushed the 2k_lines_of_changes_created_at_2AM branch from 74fb37b to 156a960 Compare September 3, 2023 15:10

AThousandShips reviewed Sep 3, 2023

View reviewed changes

editor/editor_node.cpp Outdated Show resolved Hide resolved

KoBeWi force-pushed the 2k_lines_of_changes_created_at_2AM branch 2 times, most recently from 6d7c6f1 to 7e13297 Compare September 3, 2023 15:38

AThousandShips reviewed Sep 3, 2023

View reviewed changes

KoBeWi force-pushed the 2k_lines_of_changes_created_at_2AM branch 2 times, most recently from 06c3429 to c952376 Compare September 3, 2023 16:00

AThousandShips reviewed Sep 3, 2023

View reviewed changes

KoBeWi force-pushed the 2k_lines_of_changes_created_at_2AM branch from c952376 to 61b7fb7 Compare September 3, 2023 16:13

KoBeWi force-pushed the 2k_lines_of_changes_created_at_2AM branch from 61b7fb7 to ab47f2f Compare September 3, 2023 16:24

Add EditorStringNames singleton

6de34fd

KoBeWi force-pushed the 2k_lines_of_changes_created_at_2AM branch from ab47f2f to 6de34fd Compare September 3, 2023 17:58

KoBeWi requested a review from YuriSizov September 3, 2023 17:58

YuriSizov reviewed Sep 4, 2023

View reviewed changes

scene/main/window.cpp Show resolved Hide resolved

YuriSizov reviewed Sep 4, 2023

View reviewed changes

YuriSizov approved these changes Sep 4, 2023

View reviewed changes

KoBeWi mentioned this pull request Sep 4, 2023

Add shorthand for using singleton string names #81303

Merged

YuriSizov mentioned this pull request Sep 4, 2023

Register theme properties with ThemeDB #81312

Merged

YuriSizov modified the milestones: 4.x, 4.2 Sep 5, 2023

YuriSizov merged commit 3c63dce into godotengine:master Sep 6, 2023

KoBeWi deleted the 2k_lines_of_changes_created_at_2AM branch September 6, 2023 13:13

YuriSizov mentioned this pull request Sep 6, 2023

Implement a system to contextualize global themes #81130

Merged

AThousandShips mentioned this pull request Sep 9, 2023

Some editor icons are broken in the ui (master branch) #81492

Closed

KoBeWi mentioned this pull request Nov 6, 2023

Remove CoreStringNames and SceneStringNames #50941

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EditorStringNames singleton #80573

Add EditorStringNames singleton #80573

KoBeWi commented Aug 13, 2023

Calinou commented Aug 31, 2023 •

edited

Loading

AThousandShips Sep 3, 2023

KoBeWi Sep 3, 2023 •

edited

Loading

AThousandShips left a comment •

edited

Loading

KoBeWi commented Sep 3, 2023 •

edited

Loading

AThousandShips commented Sep 3, 2023

AThousandShips commented Sep 3, 2023 •

edited

Loading

YuriSizov Sep 4, 2023

KoBeWi Sep 4, 2023

YuriSizov left a comment •

edited

Loading

YuriSizov commented Sep 6, 2023

Add EditorStringNames singleton #80573

Add EditorStringNames singleton #80573

Conversation

KoBeWi commented Aug 13, 2023

Calinou commented Aug 31, 2023 • edited Loading

Benchmark

Startup/shutdown times

Binary size

Peak memory usage

AThousandShips Sep 3, 2023

Choose a reason for hiding this comment

KoBeWi Sep 3, 2023 • edited Loading

Choose a reason for hiding this comment

AThousandShips left a comment • edited Loading

Choose a reason for hiding this comment

KoBeWi commented Sep 3, 2023 • edited Loading

AThousandShips commented Sep 3, 2023

AThousandShips commented Sep 3, 2023 • edited Loading

YuriSizov Sep 4, 2023

Choose a reason for hiding this comment

KoBeWi Sep 4, 2023

Choose a reason for hiding this comment

YuriSizov left a comment • edited Loading

Choose a reason for hiding this comment

YuriSizov commented Sep 6, 2023

Calinou commented Aug 31, 2023 •

edited

Loading

KoBeWi Sep 3, 2023 •

edited

Loading

AThousandShips left a comment •

edited

Loading

KoBeWi commented Sep 3, 2023 •

edited

Loading

AThousandShips commented Sep 3, 2023 •

edited

Loading

YuriSizov left a comment •

edited

Loading