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 constants for commonly used colors #61202

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Geometror
Copy link
Member

@Geometror Geometror commented May 19, 2022

This PR aims to improve readability by introducing constants for the most used/basic colors (red, green, blue, white, black, transparent white and transparent black).

  • Replace Color(1.0, 1.0, 1.0, 1.0), Color(1.0, 1.0, 1.0), Color(1, 1, 1) and Color(1, 1, 1, 1) with Color::WHITE
  • Replace Color(0.0, 0.0, 0.0, 1.0), Color(0.0, 0.0, 0.0), Color(0, 0, 0) and Color(0, 0, 0, 1) with Color::BLACK
  • Replace Color(1.0, 1.0, 1.0, 0.0) and Color(1, 1, 1, 0) with Color::TRANSPARENT_WHITE
  • Replace Color(0.0, 0.0, 0.0, 0.0) and Color(0, 0, 0, 0) with Color::TRANSPARENT_BLACK
  • Replace Color(1.0, 0.0, 0.0, 1.0), Color(1.0, 0.0, 0.0), Color(1, 0, 0) and Color(1, 0, 0, 1) with Color::RED
  • Replace Color(0.0, 1.0, 0.0, 1.0), Color(0.0, 1.0, 0.0), Color(0, 1, 0) and Color(0, 1, 0, 1) with Color::GREEN
  • Replace Color(0.0, 0.0, 1.0, 1.0), Color(0.0, 0.0, 1.0), Color(0, 0, 1) and Color(0, 0, 1, 1) with Color::BLUE

@Geometror Geometror requested review from a team as code owners May 19, 2022 22:15
@Geometror Geometror added this to the 4.0 milestone May 19, 2022
@Calinou
Copy link
Member

Calinou commented May 19, 2022

I agree on principle, but shouldn't we make named colors accessible from C++ instead? We already have a set of over 200 named colors accessible via constants in GDScript.

Also, it might be worth adding named colors for "transparent black" (Color(0, 0, 0, 0)) and "transparent white" (Color(1, 1, 1, 0)).

@Geometror
Copy link
Member Author

Geometror commented May 19, 2022

I actually forgot that there are already named colors (and they are accesible from C++).
However, Color::get_named_color("WHITE") is significantly longer than Color::white() and the access is not as fast since the compiler can optimize the second method. so it basically acts like a true constant. In this regard this might be still relevant.

EDIT: Adding "transparent black" and "transparent white" would definitely make sense, there are over 120 occurrences of Color(0, 0, 0, 0) and Color(1, 1, 1, 0) together.

@Calinou
Copy link
Member

Calinou commented May 19, 2022

However, Color::get_named_color("WHITE") is significantly longer than Color::white() and the access is not as fast since the compiler can optimize the second method. so it basically acts like a true constant. In this regard this might be still relevant.

Could we make those actual constants with some C++ macro/template magic? I agree we should avoid strings here (even StringNames, as they don't have autocompletion and are prone to typos).

@AaronRecord
Copy link
Contributor

Why are they methods instead of just constexprs?

@Geometror Geometror requested a review from a team as a code owner May 21, 2022 20:59
@Geometror Geometror changed the title Add basic color constant factory methods Add constants for commonly used colors May 21, 2022
@Geometror
Copy link
Member Author

@LightningAA Unfortunately, just using constexpr wouldn't work. As Color is not an integral type defining constants can be problematic (due to the way C++ handles this). I initially used methods for this as that is considered to be a safe way to avoid linker errors.

However, as I wasn't happy with it too, I changed it to the approach used by SFML (static const declaration and initialization in the implementation file). See the updated top comment for details.

@aaronfranke
Copy link
Member

aaronfranke commented May 26, 2022

Why have Color::TRANSPARENT_WHITE when you can have Color(Color::WHITE, 0)? It's even slightly fewer characters long (22 instead of 24). #36379

@@ -90,7 +99,7 @@ struct _NO_DISCARD_ Color {

bool is_equal_approx(const Color &p_color) const;

Color clamp(const Color &p_min = Color(0, 0, 0, 0), const Color &p_max = Color(1, 1, 1, 1)) const;
Color clamp(const Color &p_min = Color::TRANSPARENT_BLACK, const Color &p_max = Color(1, 1, 1, 1)) const;
Copy link
Member

Choose a reason for hiding this comment

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

The latter could be Color::WHITE, but also I don't think it actually improves readability in this case, so I think keeping the numbers makes more sense.

@@ -2027,7 +2027,7 @@ void RasterizerSceneGLES3::render_scene(RID p_render_buffers, const CameraData *
bool keep_color = false;

if (get_debug_draw_mode() == RS::VIEWPORT_DEBUG_DRAW_OVERDRAW) {
clear_color = Color(0, 0, 0, 1); //in overdraw mode, BG should always be black
clear_color = Color::BLACK; //in overdraw mode, BG should always be black
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
clear_color = Color::BLACK; //in overdraw mode, BG should always be black
clear_color = Color::BLACK; // In overdraw mode, BG should always be black.

You could fix the comment if you are editing this.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 22, 2022

The PR makes sense, but the list of colors is to be assessed. White color is by far the most commonly used one, but e.g. blue seems to be used 8 times. Although it's a primary color, so maybe makes sense to have it anyway, idk.

Also I support the idea that transparent colors can be simply achieved with Color(Color::WHITE, 0) instead of having their own constants (it's actually shorter to write).

@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Feb 10, 2023
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.

None yet

6 participants