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

Tweak error and warning colors and fix StatusWarning icon visibility on light themes #88058

Merged
merged 1 commit into from
May 10, 2024

Conversation

rsubtil
Copy link
Contributor

@rsubtil rsubtil commented Feb 7, 2024

Fixes #87987

Originally this PR focused on fixing visibility of the StatusWarning, but after successive iterations and positive feedback, a few more relevant alterations were made:

  • Removed StatusWarning and StatusError icons from the exclusion list to be able to convert its colors
  • Set error_color to the same color used in Node3D (#BF5A50 -> #CD3838)
  • Shift warning_color to a more orange saturated hue (#A69042 -> #D18F19)
  • Tweak EditorLogFilterButton appearance (more comparison screenshots at Tweak error and warning colors and fix StatusWarning icon visibility on light themes #88058 (comment)):
    • Increased icon normal color opacity from 40% to 80%
    • Removed icon hover color modulate, making it now the original color, aka 100%
    • Lightened gray brackground on pressed state by 50%

Comparison screenshots
Before After
1_old 1_new
2_old 2_new
3_old 3_new
4_old 4_new
5_old 5_new
6_old 6_new
7_old 7_new

@rsubtil rsubtil requested review from a team as code owners February 7, 2024 14:06
@KoBeWi KoBeWi added this to the 4.3 milestone Feb 7, 2024
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

As simple as that! Thanks @rsubtil !

@akien-mga
Copy link
Member

On the second "After" screenshot, the error color also seems to lack contrast when the button is not toggled.

The toggle color itself seems to create contrast issues IMO. So I think we might need a bit more work than just special casing StatusWarning.

@Calinou
Copy link
Member

Calinou commented Feb 7, 2024

The toggle color itself seems to create contrast issues IMO. So I think we might need a bit more work than just special casing StatusWarning.

The icon's opacity is decreased when the button isn't pressed – we probably need to lessen this effect, especially on light theme.

@rsubtil rsubtil force-pushed the fix_warning_icon_light_theme branch from 0ba0bd5 to 09c65f4 Compare February 8, 2024 12:55
@rsubtil
Copy link
Contributor Author

rsubtil commented Feb 8, 2024

On the second "After" screenshot, the error color also seems to lack contrast when the button is not toggled.

I've tried to remove the exclusion for this icon (StatusError) too, but the effect is too subtle to make a significant impact:

Before After
image image

But tweaking the icon's opacity as suggested by @Calinou does seem to fix the contrast issue. Here's the changes I've made (only on light mode and for the EditorLogFilterButton type), and comparison shots:

  • Increased icon normal color opacity from 40% to 80%
  • Reduce icon hover color modulate from 145% to 125%

(I've also included the hover state as I noticed it looked bad for the yellow icon)

Normal Hover Pressed
Before image image image
After image image image

@akien-mga
Copy link
Member

akien-mga commented Feb 9, 2024

(I've also included the hover state as I noticed it looked bad for the yellow icon)

Yeah I was about to point out that this is still lacking contrast. Overall the colors for both StatusError and StatusWarning on the light theme seem too washed out / desaturated IMO.

And the pressed button color (blue-grayish) also seems too dark / dull IMO, at least from close up, maybe in context it feels more appropriate.

@rsubtil
Copy link
Contributor Author

rsubtil commented Feb 9, 2024

Yeah I was about to point out that this is still lacking contrast. Overall the colors for both StatusError and StatusWarning on the light theme seem too washed out / desaturated IMO.

  • For StatusError we could make a color conversion for the same color being used for Node3D, which is a more saturated red.

  • For StatusWarning, the color is already being converted. Increasing its saturation doesn't work, as yellow will always be problematic on white, and it affects other elements adversely. But, I liked the effect when I shifted to a more "orange" hue.

And the pressed button color (blue-grayish) also seems too dark / dull IMO, at least from close up, maybe in context it feels more appropriate.

The effect is indeed much less pronounced on the dark theme, likely because of the already dark blue tint overall:

Dark Light
image image

I think reducing greatly the grayish tint works well for light themes, as the blue border is already a clear indicator of the button being toggled.


All in all, the changes are now as follows:

  • Removed StatusError from the exclusion list to convert its colors
  • Set error_color to the same color used in Node3D (#BF5A50 -> #CD3838)
  • Set warning_color to a more saturated color, and with a more orange tint (#A69042 -> #D18F19)
  • (On the output filter buttons only)
    • Increased icon normal color opacity from 40% to 80%
    • Removed icon hover color modulate (now original color, aka 100%)
    • Lightened gray brackground on pressed state by 50%
Normal Hover Pressed
Before image image image
1st iteration image image image
2nd iteration image image image

Warning

This changes some core colors used throughout all the editor. The error_color is subtle, but the warning_color is more noticeable:

Before After
image image

@Mickeon
Copy link
Contributor

Mickeon commented Feb 11, 2024

I dig the orange hue for the warning color in the light theme. It's more contrasted.
I am not sure about a few other changes that also affect the default theme, unless extremely small, as they may affect more places that one may think at a glance. Needs some testing.

@Calinou
Copy link
Member

Calinou commented Feb 16, 2024

The new warning color is great! I've always found the previous warning color less than ideal on a light theme, so this is a big improvement.

Comment on lines 399 to 400
p_config.warning_color = Color("#d18f19");
p_config.error_color = Color("#cd3838");
Copy link
Member

@Calinou Calinou Feb 16, 2024

Choose a reason for hiding this comment

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

I suggest using the Color(float, float, float) form which avoids storing a string in the binary, and therefore reduces binary size ever so slightly. Individually, it might not sound like much, but I did this on all colors in the codebase a while ago and it reduced binary size by a non-negligible amount.

Color::hex(0xrrggbbaa) might reduce binary size further by using 1 integer instead of 3 floats as parameters, but that should be evaluated later.

Copy link
Member

Choose a reason for hiding this comment

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

Color::hex(0xrrggbbaa) can be shortened to Color(0xrrggbbaa), no? It's a direct replacement for hex string.

Copy link
Contributor

@Mickeon Mickeon Feb 16, 2024

Choose a reason for hiding this comment

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

My assumption was that the compiler was smarter than this but I suppose that these are among the many things to keep in mind. I did not expect for the Colors to be basically generated on the spot even for constant Strings.

By the way, I may be wrong but I believe the bits are reversed in pure hex: 0xaabbggrr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with the 3-float approach then, but I agree reducing it in the future to a single hex int sounds interesting, especially for iterating on the colors and not having to constantly convert them to a [0, 1] float range.

Copy link
Member

@Calinou Calinou Feb 21, 2024

Choose a reason for hiding this comment

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

By the way, I may be wrong but I believe the bits are reversed in pure hex: 0xaabbggrr.

The ARGB (not ABGR) convention was used in Godot 3.x, but Godot 4.0 switched to RGBA.

@rsubtil rsubtil force-pushed the fix_warning_icon_light_theme branch from 16a1fc3 to db61cf8 Compare May 8, 2024 18:30
@rsubtil rsubtil changed the title Fix warning icon visibility on light themes Tweak error and warning colors and fix StatusWarning icon visibility on light themes May 8, 2024
@rsubtil
Copy link
Contributor Author

rsubtil commented May 8, 2024

Since the new changes ended up affecting the overall interface significantly, and there was very positive feedback on the warning color changes, I've edited this PR description to better describe the changes done, along with some new comparison screenshots.

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 good to me.

@akien-mga akien-mga merged commit 6342ab2 into godotengine:master May 10, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@rsubtil rsubtil deleted the fix_warning_icon_light_theme branch May 10, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Yellow atlas source warning exclamation mark is REALLY hard to see in light mode
6 participants