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

Allow customizing imgui colors more flexibly. #77985

Merged
merged 5 commits into from
Nov 21, 2024

Conversation

moxian
Copy link
Contributor

@moxian moxian commented Nov 19, 2024

Summary

Interface "Allow fine user customization of ImGui colors"

Purpose of change

Give user more control over the way their game looks by allowing them to customize ImGui element colors.

#75699 / #77842 made it so that user-defined base_colors are used throughout the ImGui menus. This works, but given that base_colors are restricted to the 8 (or perhaps 16) colors of terminal era, I find the solution to be deeply unsatisfying and a downgrade from the default ImGui styling (which can afford to be more subtle in color gradation and thus be more pleasing to the eye).

So this PR allows user to customize individual ImGui element colors and/or fall back to base_colors.json if they choose so.

The defaults are that of pre-#77842, default ImGui style.

Describe the solution

Create config/imgui_style.json file with the default contents of:

{
  "inherit_base_colors": false,
  "colors": {
  }
}

The inherit_base_colors makes imgui follow base_colors.json scheme, thus giving #77842-behavior.

The colors is a mapping from ImGui element name to the color value of such element as seen in the ImGui Demo Screen. For example

  "colors": {
    "ImGuiCol_WindowBg": [150, 50, 10, 255]
  }

makes the background color very orange.

This config file is loaded during the client initialization, and sets ImGui colors as directed.

The documentation, along with the full list of ImGui element names is included in docs/COLORS.md.

Describe alternatives you've considered

Much internal bikeshedding was had over the defaults. Ultimately I believe that

  1. the defaults should NOT inherit base_colors, because they are just ugly (personal opinion). The default ImGui theme works very nicely with the rest of the game, and if someone wants to change base_colors, then they should be motivated enough to also apply the change to one more place. Hence "inherit_base_colors": false as default.
  2. the default files in the config/ directory must be as empty as possible (so that if we come with more pleasing default colors and hardcode those, then they would be distributed to the users with zero effort). Hence not writing the entire imgui color specification in the default config (but still providing it in the docs).

Given that I'm not one to customize my client, I do not have the motivation to integrate the in-game Color manager with the ImGui customization thingy.

I considered making it easier/possible to copy-paste the "Export" colors from imgui demo screen to somewhere in order to easily apply them, but I couldn't find an easy way, so that got canceled.

Testing

Default install:
20241118-221640-cataclysm-tiles
20241118-221719-cataclysm-tiles

As above + "inherit_base_colors": true:
20241118-221745-cataclysm-tiles
20241118-221845-cataclysm-tiles

Default install + different theme chosen through color manager (solarized, I believe):
20241118-222008-cataclysm-tiles
20241118-222040-cataclysm-tiles

As above + inherit_base_colors:
20241118-222103-cataclysm-tiles
20241118-222136-cataclysm-tiles

As above + changing background color to orange, just to demo:

{
  "inherit_base_colors": true,
  "colors": {
    "ImGuiCol_WindowBg": [150, 50, 10]
  }
}

20241118-222244-cataclysm-tiles
20241118-222316-cataclysm-tiles

Additional context

This does incorporate #77928 , except it uses c_light_blue instead of c_blue to make the titlebar a bit less.. err.. acidic.

@github-actions github-actions bot added <Documentation> Design documents, internal info, guides and help. Info / User Interface Game - player communication, menus, etc. [JSON] Changes (can be) made in JSON [C++] Changes (can be) made in C++. Previously named `Code` [Markdown] Markdown issues and PRs labels Nov 19, 2024
@moxian moxian marked this pull request as draft November 19, 2024 06:49
@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions and removed astyled astyled PR, label is assigned by github actions labels Nov 19, 2024
@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Nov 19, 2024
@moxian moxian marked this pull request as ready for review November 19, 2024 07:02
@harakka
Copy link
Member

harakka commented Nov 19, 2024

the defaults should NOT inherit base_colors, because they are just ugly (personal opinion). The default ImGui theme works very nicely with the rest of the game, and if someone wants to change base_colors, then they should be motivated enough to also apply the change to one more place. Hence "inherit_base_colors": false as default.

This feels backwards, why would we want two separate ways of customizing colors? Imgui is going to be the UI framework going forward instead of some completely separate thing, it should obey other game settings.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Nov 19, 2024
@moxian
Copy link
Contributor Author

moxian commented Nov 19, 2024

TL;DR: We can't use existing color config system for this because the existing system is limiting and cannot be extended to meaningfully accomodate the desired level of control.

We don't want two separate ways to configure colors, but we have to use them since they are semantically different.

Existing color config describes how the color looks. I.e. you can make a "gray" color appear blue on the screen. This affects all instances of the color no matter the context (at least used to, before ImGui got introduced)
ImGui config describes the colors of specific elements. I.e. you can make unfocused window title bar appear blue on screen. This affects only said elements (so in this example, the scrollbars would remain gray even though formerly equally grey title bars are blue now).
I do not want to define ImGui element colors in terms of existing (legacy now?) base_colors due to the 16 color limiation of the latter, since I think (opinion) it is way too difficult to create good-looking modern UI with such limitation.

This section alone is ~8+ colors, and it's only shades of grey/blue

20241119-023830-cataclysm-tiles

whereas `base_colors` uses 16 colors *total*, for everything.

Compare the "default install" screenshot in the OP with "default install + inherit_base_colors". The latter is the best we can do when restricted to the existing palette and it looks worse (opinion). And it is, actually, the entire reason that spurred me to create this PR in the first place.

I myself am not a fan of having two different system to configure related things, but I see no way around it. Best I can do is merge the two color system configurations into one file (although I would rather not since it means either breaking people's setups, or dealing with migrations which is a pain), but even that is barely an improvement.

Even when everything is ImGui we would still have two different system, since base_colors.json is responsible for the color of explicitly colored text (e.g. "This item <red>conducts</red> electricty"), and some people might want to change those colors (make the red more pink-ish or something):

see the "green" theme for example (Also note that this is default ImGui theme, and that it has absolutely no bearing on the text coloring, but only on the menu elements - title bar and background color)

20241119-025151-cataclysm-tiles

I'm open to hear ways to meaningfully improve this situation, but I'm just not seeing any at the moment.

P.s.: sorry, got long-winded. If you're asking for defaults specifically, then the short answer is that base_colors-inherited styling looks worse so the default is set to not inherit base_colors. At the cost of needing to toggle an extra setting when changing the color palette away from the pre-defined one. Ideally said toggle would be located somewhere in the Color Manager, but I have no motivation to touch Color Manager since I'm not its target audience (I much prefer things work and look good out of the box, with no need for tweaks at all)

@esotericist
Copy link
Contributor

i'm supporting on your core premise here, moxian. we do need this stuff dialable to a fine degree due to the nature of graphical widgets, and i agree the existing color manager isn't up to the task. i'm not in a position to review the code, but the overall intent and documentation changes look fine so far to me (and i too am a fan of 'get good enough defaults' as a goal)

that said, the precipice we must do our best to avoid falling over is using extended colors for content. i.e. if someone wants to start defining colors beyond the base set that impact text, symbols, and the like, that's where we start running into "this breaks our ability to keep curses equivalence", and as of yet we are not writing off curses support (even if it is in an admittedly horrific state right now).

obviously, your PR isn't doing that, so there isn't an issue here. i'm just posting a warning sign for people who might look at this PR in the future and get excited and ask: "can we have more colors in conversation/item descriptions/map symbols/etc". the answer to that question is going to stay 'no' for the foreseeable future.

as a last note i do think getting the color manager modernized and integrated with all of the imgui specific stuff is probably worthwhile, but it's not a blocker for this. i'm sure someone will decide it's important eventually and make it happen.

Copy link
Contributor

@db48x db48x left a comment

Choose a reason for hiding this comment

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

This is great, but I do want to request one change. Please use floating‐point numbers for the color components, as using integers from 0 to 255 assumes that all colors components are eight bits wide. This is the most common case, but it’s not the only possibility; many monitors support 10‐bit color, for example.

@moxian
Copy link
Contributor Author

moxian commented Nov 21, 2024

Please use floating‐point numbers for the color components

Done. Docs updated.

Re-tested
{
  "//": "See doc/COLOR.md for documentation on how to configure this and the list of possible options",
  "inherit_base_colors": true,
  "colors": {
    "ImGuiCol_WindowBg": [0.6, 0.3, 0.05]
  }
}

20241120-161842-cataclysm-tiles

Copy link
Contributor

@db48x db48x left a comment

Choose a reason for hiding this comment

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

Thanks!

@Night-Pryanik Night-Pryanik merged commit dfab520 into CleverRaven:master Nov 21, 2024
21 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` <Documentation> Design documents, internal info, guides and help. Info / User Interface Game - player communication, menus, etc. [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions [Markdown] Markdown issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants