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

Implement a system to contextualize global themes #81130

Merged

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Aug 29, 2023

This PR introduces a concept of global theme contexts.

Global themes we have now

For the purposes of this change I define as a global theme any theme that we use as a fallback when looking up items, regardless of the scene tree structure. This means that currently we have 2 global themes: the default one (scene/theme/default_theme.cpp, previously scene/resources/default_theme/default_theme.cpp), and the custom project one (if it is defined by the user).

Whenever we cannot find a theme item, we first check in the project theme, and then in the default theme. This works regardless of what other themes affect the node and what parents of the node look like (i.e. if they can affect theming or not). Thus these two are global.

In the editor we have the editor theme, but it's not global. It applies directly to Control/Window nodes, just like you would do in your project. Thus the editor theme must have an uninterrupted chain of themable nodes between its holder node and any GUI node in the editor UI that needs to respect it. If the chain breaks, or if the target node is not in a branch that has one of the themed node in it, then styling break.

At the same time, in the editor we don't need nor want to have the project theme applied as global. If there are missing definitions in the editor theme, the project theme will leak into the editor UI. Historically we've patched many such holes on the case-by-case basis. And vice versa, the editor theme can leak into the 2D preview viewport or the theme editor preview tabs, if the default theme has missing definitions.

⛔⛔⛔ THIS HAS TO STOP ⛔⛔⛔

Global themes, but contextualized

I propose a new feature, with a sidenote that it's currently not exposed to users, but it may be made public in the future. The feature is Theme Contexts (maybe a better name exists).

The idea is simple: define areas/branches in the scene tree that have their own sets of global themes. By default, in the running project you would have the same two themes as you have now, the project one and the default one. However, in the editor, we would define several areas.

The default context would only contain the default theme, and no project theme, to prevent it leaking into the editor. Then the editor window and the EditorNode root of all would have a custom context consisting of the editor theme and the default theme. The editor theme would not be applied to nodes directly, instead it would become one of the fallback themes for the entire editor GUI.

To help users we would also add a custom context for the 2D viewport (project + default) and the theme editor tabs (just default). As a cherry on top we can also make the 2D viewport context controllable from the toolbar and add a way to switch between the project, the editor, and the default theme. The editor theme preview is very powerful for the editor plugin developers.

godot.windows.editor.dev.x86_64_2023-08-29_14-26-10.mp4

Each theme context replaces any previous context. Unlike themes themselves, contexts do not propagate and merge. They are authoritative and act as the final say in their controlled branch. This prevents theme leaks completely. To use contexts safely, you need to always add the default theme to each of them, or a theme that is equally complete. Otherwise things would turn unstyled.

Contexts can be attached to any node, not just GUI/Window nodes. This allows to put them before any themable node, and it also makes them truly global. Despite that, contexts are capable of propagating changes in their stored themes to the affected nodes (ignoring non-themable nodes). This means that with theme contexts GUI can react to global theme changes in real time. This is very useful when editing the project theme and looking at the 2D viewport to preview the changes.

godot.windows.editor.dev.x86_64_2023-08-29_15-37-14.mp4

In short (TL;DR)

This PR does the following changes:

  • Adds scopes for global themes called Theme Contexts.
  • Keeps global themes for running project the same as before this PR.
  • Adjusts global themes in the editor using contexts to prevent themes leaking.
  • Adds NOTIFICATION_THEME_CHANGED propagation when relevant global themes change.
  • Adds a switch to preview different themes in the 2D viewport (the setting is remembered and stored per project).
  • Does a bit of theming related cleanup in EditorNode and non-themable nodes that also use global themes (cc @bruvzg).

There are many issues that this addresses, though most of them are probably closed with fixes to themes and workarounds. Here are some that I can find:

Fixes (for real) the 4.x part of #60930 (not sure if we want to keep it open for 3.x).
Fixes #79753 (the other linked PR may still have merits).
Fixes #74762.
Fixes #73154.
Fixes #63889 (aside from the requirement to restart the editor, this can be reworked, but it's not a bug per se).
Fixes #33902 (duplicates with #63889, same note).
Fixes #81146.

@YuriSizov
Copy link
Contributor Author

YuriSizov commented Aug 29, 2023

I suspected that this might happen so I decided to test the performance. 10 runs to open and close the editor with a specific project now takes several seconds more:

Benchmark 1: .\new-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0
  Time (mean ± σ):     16.940 s ±  0.370 s    [User: 12.364 s, System: 0.316 s]
  Range (min … max):   16.739 s … 17.961 s    10 runs

  Warning: Ignoring non-zero exit code.

Benchmark 2: .\old-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0
  Time (mean ± σ):      9.432 s ±  0.111 s    [User: 6.700 s, System: 0.238 s]
  Range (min … max):    9.310 s …  9.564 s    10 runs

  Warning: Ignoring non-zero exit code.

Summary
  .\old-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0 ran
    1.80 ± 0.04 times faster than .\new-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0

This seems to affect both startup and shutdown. I will investigate this further.

@YuriSizov YuriSizov force-pushed the theme-context-for-global-themes branch from b3ff823 to 6c88b67 Compare August 29, 2023 18:03
@YuriSizov
Copy link
Contributor Author

YuriSizov commented Aug 29, 2023

Okay, I identified what was causing slow times, both on startup and shutdown. These were extra notifications trying to update the theme on many nodes in the tree. A few of those were my mistakes that I fixed, but there were also a couple of curious ones which my PR had uncovered but wasn't directly responsible for.

So, the default theme has a couple of GradientTexture2Ds as icons for the color picker. These have a peculiar property of being updated with a deferred call whenever you change their properties. This means that when we set the icon in the default theme, it comes with a pending update. On the following frame the icon does update and causes the theme itself to update. Which, thanks to my changes, propagates through the entire scene tree.

The rationale for this behavior is unclear to me. Even stranger is the fact that some methods have been changed from a deferred call to an immediate call (#60361). And we are calling this method too. So in the default theme we have the following code:

		Ref<GradientTexture2D> hue_texture;
		hue_texture.instantiate();
		hue_texture->set_width(800); // <-- defers an update
		hue_texture->set_height(6); // <-- defers an update
		hue_texture->set_gradient(hue_gradient); // <-- updates immediately

This means that the texture both, updates immediately and does a deferred update. I've discussed this with @KoBeWi and he suggested a potential improvement to the entire thing (#81137). For the time being I've patched this issue in my PR by checking for the pending flag in the _update method.

void GradientTexture2D::_update() {
	if (!update_pending) {
		return; // Might've processed already, no need to do extra work.
	}
	update_pending = false;

	...

This ensures that we don't do a deferred update, so we won't update the theme after the fact (twice, because we have two such textures!), and so we won't trigger two waves of theme changed notification propagation. This patch can be reverted when the proper change is available.


Benchmarks now show little to no difference.

Benchmark 1: .\new-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0
  Time (mean ± σ):     10.073 s ±  0.631 s    [User: 7.042 s, System: 0.301 s]
  Range (min … max):    9.706 s … 11.752 s    10 runs

  Warning: Ignoring non-zero exit code.
  Warning: The first benchmarking run for this command was significantly slower than the rest (11.752 s). This could be caused by (filesystem) caches that were not filled until after the first run. You should consider using the '--warmup' option to fill those caches before the actual benchmark. Alternatively, use the '--prepare' option to clear the caches before each timing run.

Benchmark 2: .\old-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0
  Time (mean ± σ):      9.501 s ±  0.097 s    [User: 6.656 s, System: 0.259 s]
  Range (min … max):    9.340 s …  9.593 s    10 runs

  Warning: Ignoring non-zero exit code.

Summary
  .\old-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0 ran
    1.06 ± 0.07 times faster than .\new-patch\godot.windows.editor.dev.x86_64.exe --quit -e --path C:/Projects/godot-debug-projects/sandbox-4.0

@YuriSizov YuriSizov force-pushed the theme-context-for-global-themes branch from 6c88b67 to 3564274 Compare August 29, 2023 21:57
@YuriSizov
Copy link
Contributor Author

Adjusted the changes related to GradientTexture2D following #81137.

@YuriSizov YuriSizov force-pushed the theme-context-for-global-themes branch from 3564274 to 3df1451 Compare August 30, 2023 10:03
@YuriSizov YuriSizov requested a review from a team as a code owner August 30, 2023 10:03
@YuriSizov
Copy link
Contributor Author

I noticed a visual bug due to the editor theme being accessed too early and never updated on theme changes in the ScriptEditorPlugin. Prior to this PR it was hidden, but with it a visible white outline appears in the script editor UI (twice). The latest push should fix it.

scene/theme/theme_db.cpp Outdated Show resolved Hide resolved
scene/theme/theme_db.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Aug 31, 2023

Editor theme changes do not propagate to the scene.

  1. Set View -> Preview Theme to editor
  2. Add a Button with toggle_mode enabled and make it pressed
  3. It will display text with accent color
  4. Change accent color

Although it might be the same issue as with changing project theme requiring editor restart, i.e. a new Theme is created.

@YuriSizov
Copy link
Contributor Author

Although it might be the same issue as with changing project theme requiring editor restart, i.e. a new Theme is created.

Indeed, but it should be fixable. I already update theme contexts for the editor itself, it just slipped my mind to update the preview one as well.

scene/resources/font.cpp Outdated Show resolved Hide resolved
scene/resources/font.cpp Outdated Show resolved Hide resolved
scene/resources/font.cpp Outdated Show resolved Hide resolved
@YuriSizov YuriSizov force-pushed the theme-context-for-global-themes branch from 3df1451 to 042c5b4 Compare September 1, 2023 14:54
@YuriSizov
Copy link
Contributor Author

Moved the switcher's property to the constructor. It wasn't quite as straightforward, because clear would conveniently happen after the EditorNode set its defaults, and the constructor of the plugin happens before that. So I refactored the code a bit. And also fixed the propagation issue. Changes to the editor theme should now be reflected in previews:

godot windows editor dev x86_64_2023-09-01_16-45-49

@KoBeWi
Copy link
Member

KoBeWi commented Sep 3, 2023

Selecting theme does not close the base popup.

godot.windows.editor.dev.x86_64_4qtnObGg2z.mp4

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Looks fine, aside from that minor thing with popup menu.

You could also implement switching default theme without restarting the editor. It requires a couple of lines, just like you update the editor theme, no?

@YuriSizov
Copy link
Contributor Author

YuriSizov commented Sep 4, 2023

You could also implement switching default theme without restarting the editor. It requires a couple of lines, just like you update the editor theme, no?

For the preview at least, yeah, it should be simple. I'm concerned that there may be other places that use the project theme and won't update automatically. I'll look into it in future, but it shouldn't block this PR for now.

Selecting theme does not close the base popup.

Hmm, didn't realize that this wasn't intended. I kind of like that you can swap them quickly like that, but I can change that — seems to be a one line change.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 4, 2023

If you want quick swapping then the submenu should also stay open I think. Currently it looks like a bug.

@YuriSizov YuriSizov force-pushed the theme-context-for-global-themes branch from 042c5b4 to 171b74b Compare September 4, 2023 10:35
@YuriSizov
Copy link
Contributor Author

Fair enough. Made it disappear for now, for consistency with the grid submenu at least. I don't really expect it to require to be hot accessible, so we can see if there is any demand for quick swapping. If there is, maybe it shouldn't be in the submenu so deep in the first place.

@YuriSizov
Copy link
Contributor Author

YuriSizov commented Sep 6, 2023

Rebased following the merge of #80573.

Given the nature of conflicting changes, I basically recreated most changes from #80573 in editor_node.cpp. I made sure to first rebase on top of the previous commit, and then rebase on top of #80573, so I only work with conflicts from that PR. So it should be relatively safe to assume that everything is in order. But I would appreciate a double-check, of course!

Edit: Another push to fix a leak in the tests (only relevant there because it behaves differently from the normal engine workflow).
Edit2: For real, this time...

@YuriSizov YuriSizov force-pushed the theme-context-for-global-themes branch from 886be25 to 6667ca7 Compare September 6, 2023 15:48
This commit adds the default theme context, which replaces
the need to manually check the project and the default theme
all the time; simplifies related code.

It also adds framework for custom theme contexts, to be used
by the editor. Custom contexts can be attached to any node,
and not necessarily a GUI/Window node. Contexts do no break
theme inheritance and only define which global themes a node
uses as a fallback.

Contexts propagate NOTIFICATION_THEME_CHANGED when one of their
global themes changes. This ensures that global themes act just
like themes assigned to individual nodes and can be previewed
live in the editor.
This change defines additional theme contexts for editor
branches to prevent theme leaking between the default
theme, the project theme, and the editor theme.

- Both editor window and EditorNode define an editor-specific
context with the editor theme and the default theme.
- The 2D viewport defines a project-specific context with
the project theme and the default theme.
- Theme editor preview tabs define the default-only context
with the default theme.

Additionally, the default theme context now only includes
the project theme for running projects (both export and debug).
This prevents the project theme from leaking into the editor.

This commit also does a little clean up on the theming aspects
of the EditorNode.
This commit adds a new View submenu that allows switching
between the project theme (default), the editor theme, and
the default theme. The last selected option is stored per
project and is restored when reloading the project.
@YuriSizov YuriSizov force-pushed the theme-context-for-global-themes branch from 6667ca7 to fc01e2e Compare September 6, 2023 17:41
@YuriSizov YuriSizov requested a review from a team as a code owner September 6, 2023 17:41
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks great! Let's get this tested further ahead of dev5.

@akien-mga akien-mga merged commit 8dc15e8 into godotengine:master Sep 7, 2023
25 of 26 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment