Fixes incorrect color brightness check #9677

EvilDragonfiend · 2023-08-14T09:23:32Z

About The Pull Request

Fixes incorrect color brightness check

color_hex2num() proc is incredibly bad because the mix of 3 colors does not mean anything. Hue? Saturation? Brightness? All wrong.
This PR fixes where this incorrect color calculation is used at by adding new procs get_color_brightness_from_hex() and get_color_saturation_from_hex()

Reference: color calculation
V: Brightness
S: Saturation
H: Hue (not really needed)

Why It's Good For The Game

Correct color calculation

Testing Photographs and Procedure

red jumpsuit

red SM

colourful lights

Changelog

🆑
code: better color calculation code
fix: single pure colors (red, green, blue) will not be considered to be too dark.
/:cl:

itsmeow · 2023-08-14T14:19:41Z

We already have a proc that does this, is_color_dark. We should either replace that or adapt it for this use

PowerfulBacon

Use or replace is_color_dark

EvilDragonfiend · 2023-09-01T05:30:49Z

I tested some, I don't like how rgb2num(color, COLORSPACE_HSL)[3] calculates brightness level. I failed to find brightness consistency from is_color_dark() proc.
It evaluates color-value more dark when it has color, This means it allows more dark color to mono(from black-white) while doesn't allow high saturation(from black-colored)

EvilDragonfiend · 2023-09-01T05:34:17Z

This is my code to test color values

#define hex2num(X) text2num(X, 16)


/// returns HSV brightness 0 to 100 by color hex
/proc/get_color_brightness_from_hex(A)
    if(!A || length(A) != length_char(A))
        return 0
    var/R = hex2num(copytext(A, 2, 4))
    var/G = hex2num(copytext(A, 4, 6))
    var/B = hex2num(copytext(A, 6, 8))
    return round(max(R, G, B)/2.55, 1)

/// returns HSV brightness 0 to 100 by color hex
/proc/get_color_saturation_from_hex(A)
    if(!A || length(A) != length_char(A))
        return 0
    var/R = hex2num(copytext(A, 2, 4))
    var/G = hex2num(copytext(A, 4, 6))
    var/B = hex2num(copytext(A, 6, 8))
    var/brightness = max(R, G, B)
    if(brightness == 0)
        return 0

    return round((brightness - min(R, G, B))/brightness*100, 1)

/proc/is_color_dark(color)
    var/hsl = rgb2num(color, COLORSPACE_HSL)
    return hsl[3]

/proc/main()
    var/col = "#550000"
    world.log << col
    world.log << "satur: [get_color_saturation_from_hex(col)]"
    world.log << "bright: [get_color_brightness_from_hex(col)]"
    world.log << "isdark: [is_color_dark(col)]"
    world.log << "---------------------------"
    col = "#330000"
    world.log << col
    world.log << "satur: [get_color_saturation_from_hex(col)]"
    world.log << "bright: [get_color_brightness_from_hex(col)]"
    world.log << "isdark: [is_color_dark(col)]"
    world.log << "---------------------------"
    col = "#333333"
    world.log << col
    world.log << "satur: [get_color_saturation_from_hex(col)]"
    world.log << "bright: [get_color_brightness_from_hex(col)]"
    world.log << "isdark: [is_color_dark(col)]"
    world.log << "---------------------------"
    col = "#552a2a"
    world.log << col
    world.log << "satur: [get_color_saturation_from_hex(col)]"
    world.log << "bright: [get_color_brightness_from_hex(col)]"
    world.log << "isdark: [is_color_dark(col)]"
    col = "#ff0000"
    world.log << col
    world.log << "satur: [get_color_saturation_from_hex(col)]"
    world.log << "bright: [get_color_brightness_from_hex(col)]"
    world.log << "isdark: [is_color_dark(col)]"
    world.log << "---------------------------"

EvilDragonfiend · 2023-09-01T06:01:26Z

this is the difference (alike)
I prefer saturation is not being checked from is_dark() for brightness.

PowerfulBacon · 2023-09-01T15:42:31Z

All dark colour checking should use a single and consistent proc

EvilDragonfiend · 2023-09-02T18:28:31Z

I will do as this way

#define BRIGHTNESS_BYOND_COLOR "brightness_byond_color"
#define BRIGHTNESS_HSV_PURE "brightness_hsv_pure"
/proc/is_color_dark(color_code, threshold = 25, method=BRIGHTNESS_BYOND_COLOR)
    switch(method)
        if(BRIGHTNESS_BYOND_COLOR)
            var/hsl = rgb2num(color, COLORSPACE_HSL)
            return hsl[3] < threshold
        if(BRIGHTNESS_HSV_PURE)
            var/brightness
            ..// does some calculation here
            return brightness < threshold

I still don't like how byond calculates brightness at its internal proc.

PowerfulBacon · 2023-09-03T07:31:59Z

If the old method is flawed, why even have it. Just have a single and consistent method for checking if the colour is dark. Any uses of the old function are wrong if it doesn't work

EvilDragonfiend · 2023-09-03T09:05:03Z

If the old method is flawed, why even have it. Just have a single and consistent method for checking if the colour is dark. Any uses of the old function are wrong if it doesn't work

it's what I showed to you. The perspective to judge if the color is dark or not is different. The byond way works but it's NOT how I don't expect it's working as because saturation interferes brightness in its calculation.
"is_dark()" proc can be interpreted as that it allows more dark color when it has no saturation and I exactly do NOT like that part.

itsmeow · 2023-09-03T09:06:43Z

In BYOND color uses matrix multiplication. This effectively means the more saturated something is the darker it is tinted. This is exactly as expected since most color application uses matrix multiply. This also depends on the base color. A gray vs white

EvilDragonfiend · 2023-09-03T09:07:14Z

In BYOND color uses matrix multiplication. This effectively means the more saturated something is the darker it is tinted. This is exactly as expected since most color application uses matrix multiply. This also depends on the base color. A gray vs white

Yes, I know. but that's the exact part I don't like.

itsmeow · 2023-09-03T09:07:53Z

Yea but we need this. For color picking

EvilDragonfiend · 2023-09-03T09:09:32Z

Yea but we need this. For color picking

That doesn't make sense in color picking even because
let's say the threshold is "25". You can simply understand the threshold is 25 when it's mono-color. but once you get saturation, threshold for brightness becomes 33 or 43 somewhere and you need to check every saturation how brightness allowance is changed.

The top image is how it works. the left side is simple to understand, but by saturation change, 'is_dark()` requires you to use higher brightness value in the color picker.

PowerfulBacon · 2023-09-03T11:11:10Z

is_color_dark should return true if the colour looks dark and false if the colour doesn't look dark. The method it uses doesn't matter but I still don't see why it can't be consistent. The use case shouldn't affect the method for getting the percieved darkness of the colour.

itsmeow · 2023-09-06T23:28:49Z

The old method is slightly confusing but accurate to the game as more saturated colors ARE darker in some cases.

itsmeow · 2023-09-06T23:30:34Z

code/__HELPERS/colors.dm

+	var/R = hex2num(copytext(A, 2, 4))
+	var/G = hex2num(copytext(A, 4, 6))
+	var/B = hex2num(copytext(A, 6, 8))
+	return round(max(R, G, B)/2.55, 1)


don't simplify the constant expression, it's confusing. do / 255 / 100

/ 255 / 100 is a lot more confusing imo as it is ambiguous if this means (x / 255) / 100 or x / (255 / 100) which could either be x/25500 or x/2.55. In the expression requested, this would be equivalent to x/25500 and not x/2.55.

It should be x / (255 / 100) or (x / 255) * 100, but I am not a fan of neither. I will put a comment instead.

EvilDragonfiend · 2023-09-21T11:18:02Z

updated. Now the proc has a method parameter.

PowerfulBacon · 2023-10-21T08:03:45Z

code/__HELPERS/colors.dm

+	return get_color_brightness_from_hex(hex_string, method) < threshold
+
+/// returns HSV brightness 0 to 100 by color hex
+/proc/get_color_brightness_from_hex(hex_string, method=BRIGHTNESS_BYOND_COLOR)


Why do we need different methods? I don't care if you change how existing things work since if they are broken and you are making new procs to fix it then just change it. Switching different methods is the same as making new functions. I don't see why there has to be at all a difference in how we are checking these.

There are 2 color check system in the code currently.

what Kat implemented. (AKA "kat color check")

weird color checking in the spray code (AKA "bad color check")

What I was requested to do was to change "Bad color check" to "Kat color check" but it's what I don't agree with. Kat color check isn't wrong and it's working as intended, but It's not still what I think it's correct to replace "Bad color check", and I've said the reasons above.
The only way to fix it here I believe is to calculate it differently rather than "Kat color check", and it's "Dragon color check" which doesn't count saturation to brightness.
That's why I am putting two methods for checking colors.

There should be 2 different procs then, is_color_dark and is_color_unsaturated. Is colour dark should only check if a colour visually looks dark, which is colours that have a low bright ness and low saturation.

I dislike procs that have 2 different behaviours for the same thing with an enum passed in as a parameter to determine how the proc should function. They should just be different procs if they are going to behave differently.

Both of those should be used to check if a color is dark because both needs to check brightness, but byond one needs brightness + saturation while mine don't use saturation - only brightness.
your suggestion is_color_unsaturated() is not correct because this exactly means it's used for the old is_color_dark() as it uses saturation, but mine don't use it.

I made changes here to make two procs: is_color_dark_with_saturation() and is_color_dark_without_saturation().
The old is_dark() is now is_color_dark_with_saturation(), and my proc is is_color_dark_without_saturation()

Addressed

color change

6abce30

solid-octo-guacamole bot added Code Improvement Fix labels Aug 14, 2023

Fix comment

e6e758f

PowerfulBacon requested changes Aug 25, 2023

View reviewed changes

EvilDragonfiend requested a review from PowerfulBacon September 1, 2023 05:33

itsmeow requested changes Sep 6, 2023

View reviewed changes

more method

22330a3

PowerfulBacon previously requested changes Oct 21, 2023

View reviewed changes

EvilDragonfiend added 2 commits November 27, 2023 10:34

Merge branch 'master' into colorfix

477c863

split procs

8cc5109

itsmeow approved these changes Nov 27, 2023

View reviewed changes

itsmeow added this pull request to the merge queue Nov 27, 2023

Merged via the queue into BeeStation:master with commit b320f97 Nov 27, 2023
8 checks passed

ss13-beebot added a commit that referenced this pull request Nov 27, 2023

Automatic changelog generation for PR #9677 [ci skip]

723c2b4

EvilDragonfiend deleted the colorfix branch November 27, 2023 12:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes incorrect color brightness check #9677

Fixes incorrect color brightness check #9677

EvilDragonfiend commented Aug 14, 2023 •

edited

Loading

itsmeow commented Aug 14, 2023

PowerfulBacon left a comment

EvilDragonfiend commented Sep 1, 2023 •

edited

Loading

EvilDragonfiend commented Sep 1, 2023 •

edited

Loading

EvilDragonfiend commented Sep 1, 2023 •

edited

Loading

PowerfulBacon commented Sep 1, 2023

EvilDragonfiend commented Sep 2, 2023 •

edited

Loading

PowerfulBacon commented Sep 3, 2023

EvilDragonfiend commented Sep 3, 2023 •

edited

Loading

itsmeow commented Sep 3, 2023

EvilDragonfiend commented Sep 3, 2023

itsmeow commented Sep 3, 2023

EvilDragonfiend commented Sep 3, 2023 •

edited

Loading

PowerfulBacon commented Sep 3, 2023

itsmeow commented Sep 6, 2023

itsmeow Sep 6, 2023

PowerfulBacon Sep 15, 2023

EvilDragonfiend Sep 21, 2023 •

edited

Loading

EvilDragonfiend commented Sep 21, 2023

PowerfulBacon Oct 21, 2023

EvilDragonfiend Nov 2, 2023 •

edited

Loading

PowerfulBacon Nov 20, 2023 •

edited

Loading

EvilDragonfiend Nov 27, 2023 •

edited

Loading

Fixes incorrect color brightness check #9677

Fixes incorrect color brightness check #9677

Conversation

EvilDragonfiend commented Aug 14, 2023 • edited Loading

About The Pull Request

Why It's Good For The Game

Testing Photographs and Procedure

Changelog

itsmeow commented Aug 14, 2023

PowerfulBacon left a comment

Choose a reason for hiding this comment

EvilDragonfiend commented Sep 1, 2023 • edited Loading

EvilDragonfiend commented Sep 1, 2023 • edited Loading

EvilDragonfiend commented Sep 1, 2023 • edited Loading

PowerfulBacon commented Sep 1, 2023

EvilDragonfiend commented Sep 2, 2023 • edited Loading

PowerfulBacon commented Sep 3, 2023

EvilDragonfiend commented Sep 3, 2023 • edited Loading

itsmeow commented Sep 3, 2023

EvilDragonfiend commented Sep 3, 2023

itsmeow commented Sep 3, 2023

EvilDragonfiend commented Sep 3, 2023 • edited Loading

PowerfulBacon commented Sep 3, 2023

itsmeow commented Sep 6, 2023

itsmeow Sep 6, 2023

Choose a reason for hiding this comment

PowerfulBacon Sep 15, 2023

Choose a reason for hiding this comment

EvilDragonfiend Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

EvilDragonfiend commented Sep 21, 2023

PowerfulBacon Oct 21, 2023

Choose a reason for hiding this comment

EvilDragonfiend Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

PowerfulBacon Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

EvilDragonfiend Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

EvilDragonfiend commented Aug 14, 2023 •

edited

Loading

EvilDragonfiend commented Sep 1, 2023 •

edited

Loading

EvilDragonfiend commented Sep 1, 2023 •

edited

Loading

EvilDragonfiend commented Sep 1, 2023 •

edited

Loading

EvilDragonfiend commented Sep 2, 2023 •

edited

Loading

EvilDragonfiend commented Sep 3, 2023 •

edited

Loading

EvilDragonfiend commented Sep 3, 2023 •

edited

Loading

EvilDragonfiend Sep 21, 2023 •

edited

Loading

EvilDragonfiend Nov 2, 2023 •

edited

Loading

PowerfulBacon Nov 20, 2023 •

edited

Loading

EvilDragonfiend Nov 27, 2023 •

edited

Loading