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.

Sign up for GitHub

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

Fixes incorrect color brightness check #9677

Merged
merged 5 commits into from
Nov 27, 2023
Merged

Fixes incorrect color brightness check #9677

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6abce30
color change
EvilDragonfiend Aug 14, 2023
e6e758f
Fix comment
EvilDragonfiend Aug 14, 2023
22330a3
more method
EvilDragonfiend Sep 21, 2023
477c863
Merge branch 'master' into colorfix
EvilDragonfiend Nov 27, 2023
8cc5109
split procs
EvilDragonfiend Nov 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions beestation.dme
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
#include "code\__DEFINES\cleaning.dm"
#include "code\__DEFINES\clockcult.dm"
#include "code\__DEFINES\clothing.dm"
#include "code\__DEFINES\color.dm"
#include "code\__DEFINES\colors.dm"
#include "code\__DEFINES\combat.dm"
#include "code\__DEFINES\communications.dm"
Expand Down
4 changes: 4 additions & 0 deletions code/__DEFINES/color.dm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/// rgb2num(color, COLORSPACE_HSL) calculates saturation for brightness. This method will allow dark monocolor, while not allow darker chromatic color.
#define BRIGHTNESS_BYOND_COLOR "brightness_byond_color"
/// this is manual calculation to get only HSV brightness that brightness value is all the same regardless of saturation value.
#define BRIGHTNESS_HSV_PURE "brightness_hsv_pure"
33 changes: 30 additions & 3 deletions code/__HELPERS/colors.dm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,36 @@

/// Given a color in the format of "#RRGGBB", will return if the color
/// is dark.
/proc/is_color_dark(color, threshold = 25)
var/hsl = rgb2num(color, COLORSPACE_HSL)
return hsl[3] < threshold
/proc/is_color_dark(hex_string, threshold = 25, method=BRIGHTNESS_BYOND_COLOR)
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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@EvilDragonfiend EvilDragonfiend Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@PowerfulBacon PowerfulBacon Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@EvilDragonfiend EvilDragonfiend Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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()

if(!hex_string || length(hex_string) != length_char(hex_string))
return 0
switch(method)
if(BRIGHTNESS_BYOND_COLOR)
var/hsl = rgb2num(hex_string, COLORSPACE_HSL)
return hsl[3]
if(BRIGHTNESS_HSV_PURE)
var/R = hex2num(copytext(hex_string, 2, 4))
var/G = hex2num(copytext(hex_string, 4, 6))
var/B = hex2num(copytext(hex_string, 6, 8))
return round(max(R, G, B)/255, 1) // devide by 255 to make 0-100 value range.
return 0

/// returns HSV saturation 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)

/// Given a 3 character color (no hash), converts it into #RRGGBB (with hash)
/proc/expand_three_digit_color(color)
Expand Down
9 changes: 0 additions & 9 deletions code/__HELPERS/type2type.dm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,15 +473,6 @@ Takes a string and a datum. The string is well, obviously the string being check
if(var_source.vars.Find(A))
. += A

/// Converts a hex code to a number
/proc/color_hex2num(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 R+G+B

//word of warning: using a matrix like this as a color value will simplify it back to a string after being set
/proc/color_hex2color_matrix(string)
var/length = length(string)
Expand Down
4 changes: 2 additions & 2 deletions code/game/objects/items/crayons.dm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,7 @@

if(isobj(target) && !(target.flags_1 & UNPAINTABLE_1))
if(actually_paints)
if(color_hex2num(paint_color) < 350 && !istype(target, /obj/structure/window)) //Colors too dark are rejected
if(is_color_dark(paint_color, 33, BRIGHTNESS_HSV_PURE) && !istype(target, /obj/structure/window)) //Colors too dark are rejected
if(isclothing(target))
var/obj/item/clothing/C = target
if(((C.flags_cover & HEADCOVERSEYES) || (C.flags_cover & MASKCOVERSEYES) || (C.flags_cover & GLASSESCOVERSEYES)) && !HAS_TRAIT(C, TRAIT_SPRAYPAINTED))
Expand All @@ -717,7 +717,7 @@

target.add_atom_colour(paint_color, WASHABLE_COLOUR_PRIORITY)
if(istype(target, /obj/structure/window))
if(color_hex2num(paint_color) < 255)
if(is_color_dark(paint_color, 50, BRIGHTNESS_HSV_PURE))
target.set_opacity(255)
else
target.set_opacity(initial(target.opacity))
Expand Down