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

Add new setting for theme selection #637

Merged
merged 4 commits into from Apr 23, 2020
Merged

Conversation

Basewq
Copy link
Contributor

@Basewq Basewq commented Mar 27, 2020

PR Details

Add ability to choose a new (hardcoded) theme (color scheme).
Current options are 'Expression Dark' (the current color scheme), and 'Dark Steel' (the color scheme that's work in progress for this draft).
NOTE: This does not remove the existing color scheme, and that will remain the default option for new users.

Description

Unfortunately with how the xaml code is architectured, it doesn't seem like we can change the (color) brush bindings from StaticResource to DynamicResource.
I believe this is a limitation in WPF with the usage of templates, where WPF must 'freeze' the brush/bindings inside a template, so they can't be changed (and the majority of Xenko's xaml code is placed inside templates).
Due to my lack of expertise in WPF, the only solution I've found is to inherit ResourceDictionary and get this new ThemeResourceDictionary to intercept and immediately change the xaml source path of the "dynamic" theme colors so that the app can be forced to read the correct theme file.
And yes, there's a couple of static fields that are (seemingly) unavoidable due to how the resource source must be intercepted.

Second problem is because we're stuck with StaticResource bindings, the editor must be restarted for the new theme to appear.

This pull request is specifically for color scheme changes. Things like rounded corners, larger icons, etc are out of scope of this pull request.
The new dark theme will probably be closer to Unity/Unreal/Blender, or even the old Xenko version (which is currently still featured in the github readme file!).

Note: I am not currently looking at implementing a light theme, mainly due to the possibility of this pull not being accepted because of crappy coding (and there's a possibility additional minor changes may be required to fix it in light mode). If the pull is accepted, it should be "easy" (famous last words) for someone to add a new theme.
Also if not accepted, someone can still view this commit and apply to their own fork and make their own changes, or find a much better way to achieve this.

Where the settings appear in the Editor:
Theme_Settings

The 'Dark Steel' theme will look like the following:
Theme_v5

While the default theme is largely unchanged, a 'fix' has been made to change the brush used in the Settings sub-window in the properties section. The following is how the change will look (left = current, right = new)
Settings_Fix

The main "switch" for swapping the themes lies in the ThemeSelector.xaml (Note there are two files, one for the main color overrides, and the other for a third party control's theme).
The main color scheme data resides in the Overrides folder (eg. Overrides/DarkSteelTheme.xaml)

Motivation and Context

The current dark theme has a much stronger 'dark contrast' compared to other applications' dark themes, and the strong contrast can unpleasant to look at for extended periods of time (in my opinion). This version more subdued and should be easier on the eyes.
See Unreal, Unity, Blender, etc for examples as less harsher dark themes.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@xwellingtonx
Copy link
Collaborator

xwellingtonx commented Mar 27, 2020

Hey @Basewq, you might want to check how was implemented the theme into this project below, maybe we can do the same in stride3d.

https://github.com/MahApps/MahApps.Metro/tree/develop/src/MahApps.Metro

@xen2
Copy link
Member

xen2 commented Mar 27, 2020

Thanks for looking into it.
A few general remarks:

  1. I remember vaguely that this was discussed at some point with the team, and there was some non-negligible (even huge?) performance impact on using DynamicResource everywhere.
    (Ping @Kryptos-FR @Benlitz)

  2. If I understood, your current approach is to keep StaticResource and force a GS restart on theme change?
    If yes, I am totally fine with that (versus the complexity/performance of handling live switch, esp. if DynamicResource is slow).

  3. I was also wondering if you think 100% of the theme change could be covered only with brush changes, or some other stuff might be necessary? (asking mostly to evaluate additional workload to add this feature, and cost for future support/WPF/UX changes)

@tebjan
Copy link
Member

tebjan commented Mar 27, 2020

Ping @0xAryan since you worked on the previous theme. If your time allows, it would be super helpful if you could have a quick look/evaluation of this. It doesn't have to be the final solution as a first step. Thanks!

@Basewq
Copy link
Contributor Author

Basewq commented Mar 27, 2020

Thanks for looking into it.
A few general remarks:

  1. I remember vaguely that this was discussed at some point with the team, and there was some non-negligible (even huge?) performance impact on using DynamicResource everywhere.
    (Ping @Kryptos-FR @Benlitz)

  2. If I understood, your current approach is to keep StaticResource and force a GS restart on theme change?
    If yes, I am totally fine with that (versus the complexity/performance of handling live switch, esp. if DynamicResource is slow).

  3. I was also wondering if you think 100% of the theme change could be covered only with brush changes, or some other stuff might be necessary? (asking mostly to evaluate additional workload to add this feature, and cost for future support/WPF/UX changes)

1 & 2. Well, I tried DynamicResource but it doesn't seem possible in the current code's state. If it was possible then we could've evaluated the performance, but it's a non-starter.
3. Sorry, I don't fully understand your question but my guess to your question:
Add a new theme requires:
Add a new ThemeType enum value
Add a new xaml file to Themes/Overrides folder
Add a new field/case in ThemeResourceDictionary
Set the new field in the xaml declarations where ThemeResourceDictionary exists (currently only 3 files). This could potentially be refactored so ThemeResourceDictionary only exists in single xaml file to, but currently I don't want to mess with too much stuff.

I believe things like corner radius, font sizes can potentially be part of the theme settings, it just means pushing more keys into the Overrides/*Themes.xaml, and ensuring a binding is defined on the specific controls.
Another problem with the current way is that if a new override key/setting is introduced, all files need to have it in order to display correctly, as there's no proper fallback mechanism. Game Studio shouldn't crash, but it'll be whatever the default value is for that field (eg. transparent color, zero, etc)

@avestura
Copy link
Contributor

This does the job for the most part if I remember correctly, but there are some hardcoded colors for some borders and stuff that are in different places in the code (unfortunately I don't remember where).
Also, this theming feature is all fun and games until you want to bring Light themes into the scene, then you should change all the {StaticResource IconResourceName} to either Image or ThemedResource markup extensions who know how to change the icon colors based on the luminosity of a given theme (both are defined in Xenko.Core.Presentation.MarkupExtensions). Microsoft uses this approach (having one set of vector icons and dynamically changing its colors based on theme) but I don't know how they optimized it to not affect the performance.

@xen2
Copy link
Member

xen2 commented Mar 27, 2020

@Basewq OK perfect for 1/2, my point was actually to stick with StaticResource to keep things faster/simpler (at the cost of a necessary restart).

About 3. I was mostly curious about which kind of changes were more complex than color, such as the round corners or the icons thing @0xAryan mentionned. Thanks for giving actual exemples.

@Basewq
Copy link
Contributor Author

Basewq commented Mar 27, 2020

Thanks for the info @0xAryan. Yes, light themes will bring in some extra work which is why I'm trying to avoid it for now. Off the top of my head, maybe binding with a converter?
Assuming we're ok with hardcoded themes, we can possibly get the converter to check against the hacky static field which holds the current theme, so we'll know if it's a light/dark theme.
BTW, I split out the ExpressionDark/Theme.xaml file in this commit. Was this originally hand written, or did you use some sort of generator? Wondering if I may need to revert this split, and just duplicate the entire file for new theme.

@Kryptos-FR
Copy link
Member

@xen2 Yeah DynamicResource gets bad really quickly, the more resources you have. As @Basewq pointed out in the description, one important thing to improve performance is to freeze some of the brush so that the rendering pass doesn't have to recalculate every time.

Now, it has been a while since I had a closer look to that, but that's what I remember. It is totally fine in my opinion to have to restart the GameStudio when changing the theme.

@Basewq
Copy link
Contributor Author

Basewq commented Mar 29, 2020

First off, I'm going to 'fix' the settings background color from HoverBrush to BackgroundBrush.
This affects every theme, ie. the 'classic' theme will have the settings subwindow change from the left to the right version. I hope this is ok.
Settings_Fix

This is 'option 0' of the new color scheme.
Theme_v0

This is 'option 1' of the new color scheme, as given by @Tonedus
Theme_v1

Also, be aware I'm not overly fussed over the new color scheme, so it can be edited/adjusted/fixed over time.

This screenshot is merely a helpful reference to see which brush key is affecting the most common items.
Theme_Color_Help

@tebjan
Copy link
Member

tebjan commented Mar 29, 2020

i like the one that looks like it's using the same colors as visual studio. since people would use both side-by-side often, it makes sense to use the same colors.

@PizzaConsole
Copy link

+1 for option 0

@tebjan
Copy link
Member

tebjan commented Apr 2, 2020

just saw one thing, the help tooltip of the intelli sense window looks a bit funny, not sure whether that is easy to fix:
image

@Basewq
Copy link
Contributor Author

Basewq commented Apr 2, 2020

just saw one thing, the help tooltip of the intelli sense window looks a bit funny, not sure whether that is easy to fix:

Thanks, I'll look into it.

@Basewq
Copy link
Contributor Author

Basewq commented Apr 3, 2020

@tebjan Ok, as far as I'm aware the tooltip can't be changed here.
Digging really deep, but assuming we're using aelij/RoslynPad, there's this thing called an InsightWindow which is the tooltip for the auto-completion popup, which generates a QuickInfoContent, then at some point it goes into this line:
https://github.com/aelij/RoslynPad/blob/7f63cb27ae47fddd56666694785526539c6c8aef/src/RoslynPad.Roslyn.Windows/SymbolDisplayPartExtensions.cs#L31-L58
I'm not 100% sure, but I believe these are the colors used in the tooltip as the color hexes match.
It looks like these colors are hard-coded for the tooltip. (ie. it completely bypasses the ClassificationHighlightColors which is used for the standard text editor's color scheme)

@xen2
Copy link
Member

xen2 commented Apr 4, 2020

@Basewq I wanted to quickly check with you about the Stride renaming.
It is probably going to happen soon in master branch.
I was wondering if you had any other branch than this one pending?

It might be good to freeze everything for a couple of days while I do the final changes and merge them. During that time, PR/new branch are better avoided to avoid unnecessary rebase work (automatic rebase don't work so well with so many file renaming & content change).

If an existing branch (not ready to be merged yet) needs to be ported after the renaming, please rest assured it's possible (either by careful cherry-picking/merging, otherwise by applying the same renaming logic on the master branch, which is currently done with a small C# project I will publish soon).

Thanks again and sorry for the trouble/inconvenience!

@Basewq
Copy link
Contributor Author

Basewq commented Apr 4, 2020

@xen2 Thanks for the heads up. No, I don't have any other branches. I am happy to wait until you do the renaming, as I am still doing some touch ups, and I'm ok with handling the rebase on my own side.

@Basewq
Copy link
Contributor Author

Basewq commented Apr 19, 2020

New dark theme should be considered completed (the new colors are shown in the original post).

I have also included an experimental light theme, mainly "inspired" by the Visual Studio light theme:
Theme_vlight

It's done for the most part but there's a few issues.
The main issue is that light themes tend to flip text colors from dark to light for selected items.
This is mostly completed for text, however trying to do this for icons is harder (since WPF doesn't have a tinting mechanism, it'll require an image swap). This problem is present in the main tabs (eg. see the 'X' button in 'MainScene' tab).
A workaround would be to just not make the active tab's background color change enough to require a text color flip.
I think another issue is the asset thumbnails (as seen in the screenshots). The white icons, and the dark background for things like the EffectCompileLog.

There's probably a few more display issues that I haven't spotted. I am willing to fix up minor color issues if they exists for the light theme, but any bigger work to get it working further is probably not on my to-do list (eg. fixing the thumbnails, flipping images from dark to light when selected/hovered).

I'm not truly championing the light theme so the light theme commit is intentionally separate so it can be excluded from being merged. I can just keep this branch open for any future devs who want to develop it further.

@Basewq Basewq marked this pull request as ready for review April 19, 2020 22:41
DarkSteel,

// Light themes
[Display("Light Steel Blue")]
Copy link
Member

Choose a reason for hiding this comment

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

If the light theme is not perfect/complete, maybe we could change display name to Light Steel Blue (Experimental)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I'm ok with that. Done and rebased on top of the latest (this was the only change, apologies if you didn't want me to rebase again).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for rebasing, it's helpful!

@Aggror
Copy link
Member

Aggror commented Apr 22, 2020

Looking good @Basewq

@xen2 xen2 merged commit 2575fbf into stride3d:master Apr 23, 2020
@Aggror
Copy link
Member

Aggror commented Apr 23, 2020

Perfect, right on time for the recording of new tutorials.

@Tonedus
Copy link

Tonedus commented Apr 23, 2020

I liked it! It's a nice color scheme @Basewq

@tebjan
Copy link
Member

tebjan commented Apr 23, 2020

nice one!

@Basewq Basewq deleted the multitheme branch May 2, 2020 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants