Add type variations to Theme

[YuriSizov]

Continuation of #47544, lays ground work for a remake of #49336. With this PR the property theme_custom_type was renamed to theme_type_variation to better reflect its role.

---

This PR makes it possible to mark theme types as variations of other theme types. The benefits of this are:

• Clear dependency chains between theme types;
• Ability to fetch appropriate theme type variations for a Control opened in the inspector;
• Base type's theme items can be listed in the theme editor when editing a variation.

2021-07-05_00-07-41.mp4

Variations can depend on node classes and on each other, so they can be nested. When theme items are fetched from the theme these chains are respected. Note that variations can only be chained within a single theme resource (i.e. your theme resource cannot add a variation for a variation from the default theme; but it can extend the same base class).

Only the default theme provided by Godot and the theme set as project default are used to fetch options for the inspector. This limitation is in place to make the system simpler and saner, while still allowing user customization. Variations are more meta than individual properties of types used throughout the project, so it makes sense to me to define them at the project level like that. To be clear: they still can be redefined/customized by any theme in the project, but that won't make them visible in the Inspector.

The list of variations provided by the Inspector is not exclusive, and it is possible to enter an arbitrary value for the type variation. If that value exists in a theme resource available to the control in question, it will work exactly like it did before this PR (refer to the documentation of the property).

This PR also includes some fixes to the Theme resource, mostly related to handling and saving font sizes.

---

This PR doesn't include a way to set variations up using the Theme editor. I plan to do another PR improving some of the aspects of the Theme editor and I will add some UI for it there. The necessary methods are already available in the API, but variations currently won't save with the resource and can only exist at the runtime, which shouldn't be a problem for our own use case with the editor. Theme editor changes are now included in this PR.

With this PR #49336 can finally be redone in a manner that satisfies everyone.

[KoBeWi]

How do I test the variations? I tried creating Theme, using set_type_variation and saving it, but the variation is not saved with the resource.

[YuriSizov]

@KoBeWi For now you have to add them to the default theme before compiling locally.

Something like this should give you the same options as in the clip.

theme->set_type_variation("Header1", "Label");
theme->set_font_size("font_size", "Header1", 48);

theme->set_type_variation("RedHeader1", "Header1");
theme->set_color("font_color", "RedHeader1", Color(1, 0, 0));

theme->set_type_variation("Header2", "Label");
theme->set_font_size("font_size", "Header2", 36);

theme->set_type_variation("Header3", "Label");
theme->set_font_size("font_size", "Header3", 24);

And "BlueHeader1" was just a custom type defined on the project theme without marking it as a variation.

[KoBeWi]

What is the purpose of options.size() + 1000? Seems like the argument doesn't have any effect. (same above)

[YuriSizov]

Just so we don't conflict with the default options which for convenience have ids that match their original indices. It can just as well be + 1, but a bigger number gives a nice offset, and I don't think it matters.

[KoBeWi]

This is pretty minor, but editing the variation moves the text one pixel down:

As for the implementation:
Do we need variation_map and variation_base_map? From what I see you keep them in sync, but the base map could be re-created from variation map, so it could be removed to avoid potential bugs. I guess it was added for some performance reasons, right? 🤔

Also, you are planning to allow modifying the variations in theme editor, so that means you probably need to serialize them for it to make sense, no? You could add the serialization in this PR, so it's at least possible to set the values via script for now.

[YuriSizov]

This is pretty minor, but editing the variation moves the text one pixel down:

Hmm, this is likely a minor difference in how OptionButton and LineEdit position the text. Maybe I can play around with some alignment settings, but not sure if it would help.

Do we need variation_map and variation_base_map? From what I see you keep them in sync, but the base map could be re-created from variation map, so it could be removed to avoid potential bugs. I guess it was added for some performance reasons, right?

Yeah, we need both directions of lookup for various cases, so it made sense to keep both maps for speed (and I think it was discussed like that with reduz at some point on RC). There is always a possibility of such maps going out of sync, I know, but so far the manipulations are very limited and very controlled. There is basically only one method that needs the reverse map, so a change to remove it won't be large, but it happens to be the method we need for the Inspector which is a significant use case.

So, eh, there is a trade-off, I'm fine either way, so let's wait for some more reviews maybe?

Also, you are planning to allow modifying the variations in theme editor, so that means you probably need to serialize them for it to make sense, no? You could add the serialization in this PR, so it's at least possible to set the values via script for now.

Yeah, just so. I've actually outlined it in the OP, at the bottom of it 👀 I am reluctant to add it to the serialization right now as I haven't had time yet to assess how the theme editor would need to work and if that may affect the serialization somehow (as these properties are virtual and defined by get_property_list, set and get). There is probably nothing to worry about, but I wasn't able to give it a thought yesterday at all, so decided to omit it since it doesn't block the headers and bolds rework anyway.

I'll work on the Theme editor on the upcoming weekend, and this is master branch, so I figured, not a huge deal.

[KoBeWi]

Looks ok. Resource saving could indeed be made in another PR.

Still needs fixing that text offset, but it's very minor thing.

[YuriSizov]

I've adjusted editor theme's LineEdit styles to fix that 1px shift. This should affect all LineEdits in the editor, but ever so slightly. If anything, texts should be a bit more centered vertically now. LineEdits should also retain their size after this change, and I didn't notice anything else looking weird because of the adjustment.

https://github.com/godotengine/godot/blob/1c01d9b051b606b947ead4f4d05d70c327270331/editor/editor_themes.cpp#L1007-L1009

To add to that, this 1 px shift is not present in the default theme, so it's probably an unwanted side-effect of LineEdits in the editor theme relying on one of the universally used styleboxes (as mentioned in the comment in the code above).

[YuriSizov]

Temporarily converting to draft as I'm currently implementing saving and the editor UI to manage variations. Should be ready to review again later today.

[YuriSizov]

I've implemented serialization for the variations, as well as added UI for editing them in the theme editor. Additionally, a base type of the variation is now considered when listing default properties, making the new editor even more powerful:

2021-07-12_21-37-37.mp4

Base types can be entered manually, or can be picked via the same dialog as the one used for adding types.

---

I think it's better to test this completely anew, because you can now properly work with the project-scope theme, so some issues may now become apparent.

[KoBeWi]

variation_of property looks a bit odd (maybe it could be base_type idk). Also seems like it's possible to add a core type to theme and set another core type as variation base.

Shouldn't base type be configurable only for custom types?

[YuriSizov]

Shouldn't base type be configurable only for custom types?

Since types are just a construct, I'm not sure what the cut off line would be. Presence in the ClassDB? There are some corner cases possible that are not explicitly handled by the system, but I was afraid of making it too complicated and confusing otherwise so I opted to rely on users using it reasonably.

variation_of property looks a bit odd (maybe it could be base_type idk).

I agree that it's a bit odd, but I wanted to make sure that from code and from the resource it's clear what it does. base_type is probably okay.

[KoBeWi]

Since types are just a construct, I'm not sure what the cut off line would be. Presence in the ClassDB?

Yep, anything that can be created as a node/has an icon, should be disallowed to have base type. Otherwise you can have e.g. Label and set its type variation to MarginContainer. It doesn't make much sense (but doesn't have side-effects either 🤔), these types are just incompatible. Aalthough...

I opted to rely on users using it reasonably.

...this makes sense too. If changing it would be complicated then whatevs.

[YuriSizov]

Okay, this particular thing wasn't hard to address, but I'm sure there are other cases that would be (mostly related to nesting, likely). At any rate, there are two checks now, in the API and in the UI. The disabled field doesn't look good in the tab, unfortunately, but that's an existing problem with those tabs that can probably be addressed separately.

And I've renamed the property to base_type.

[akien-mga]

Thanks!