Skip to content

Add Color.get_named_colors()#67752

Open
Mickeon wants to merge 1 commit intogodotengine:masterfrom
Mickeon:named_color_dict
Open

Add Color.get_named_colors()#67752
Mickeon wants to merge 1 commit intogodotengine:masterfrom
Mickeon:named_color_dict

Conversation

@Mickeon
Copy link
Copy Markdown
Member

@Mickeon Mickeon commented Oct 22, 2022

See also #64830.

This PR implements the static method Color.get_named_colors(). It returns a Dictionary, where each key is a name corresponds to a Color from the exposed engine API.

The idea is that using the Dictionary over find_named_color(), get_named_color_count(), get_named_color_name(), get_named_color is favourable.
The advantage is that it is much more accessible, because unlike them, no integer index is required or returned. It also opens up the door for more interesting manipulation. It needs to be said, however, that not everything can be replicated. Namely, find_named_color() does some string manipulation to be case-insensitive and support spaces (the Color(String) constructor does the same thing)...

However, find_named_color() also returns a numerical index! And Color.from_string() is the better equivalent, just returning the Color directly! Bah... Look at #64830.

If an user, for some reason, needs an integer index, it's worth reminding that Dictionaries remember the insertion order. Therefore:

	var named_colors = Color.get_named_colors()
	
	# Color.get_named_color_count() -> int
	var count = named_colors.size()
	
	# Color.get_named_color_name(1) -> String
	var color_name = named_colors.keys()[1]
	
	# Color.get_named_color(1) -> Color
	var colour_from_idx = named_colors[color_name]

@Mickeon Mickeon requested review from a team as code owners October 22, 2022 13:55
@dalexeev
Copy link
Copy Markdown
Member

It seems to me that there should be a method in ClassDB or somewhere else to get a list of class constants (and their values). Then it will be a universal solution, and not just for the Color class.

@Mickeon

This comment was marked as outdated.

@Mickeon
Copy link
Copy Markdown
Member Author

Mickeon commented Oct 22, 2022

If ClassDB.class_get_integer_constant_list() would work for built-in Variant Types, not just classes, there would be almost no need for this. In theory, because a PackedStringArray is returned from it, it'd still be laborious to get all of the Colors matching all of the constant names. So... nope.

@kleonc kleonc added this to the 4.x milestone Oct 22, 2022
Copy link
Copy Markdown
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

PR review meeting: we'd like to gauge demand for this feature if its wanted, and the implementation isn't optimal (re-creating the dictionary on each call), but it may be hard to improve with current GDScript constraints, especially considering this dict may change at runtime due to e.g. GDExtension.

@jtnicholl
Copy link
Copy Markdown
Contributor

Didn't see this PR or #64830 until it was merged and one of my projects broke. I was using the named color functions to get random colors, as just picking purely random RGB or HSV values tends to give unappealing results.
I could probably figure out an alternative, but since you said you're gauging interested I thought I'd mention that I'd use this.

the implementation isn't optimal (re-creating the dictionary on each call)

That would be fine IMO if you just mention in the docs description that it can be slow, and suggest that users store the dictionary in a variable if they're going to reuse it.

@Mickeon
Copy link
Copy Markdown
Member Author

Mickeon commented Nov 2, 2022

Oh, the problem is not actually that it is slow, it's that it's a new Dictionary every time. In the PR meeting it was discussed that the Dictionary should be initialised once when the function is called, then every call should return a reference to the same Dictionary. To have that, however, there are some complications. Namely, the dictionary would have to be read-only, while still supporting GDExtension(?).

@dalexeev
Copy link
Copy Markdown
Member

dalexeev commented Nov 3, 2022

I was using the named color functions to get random colors, as just picking purely random RGB or HSV values tends to give unappealing results.

You can limit the range of possible component values and optionally round them to the grid.

Details
func random_color_rgb() -> Color:
    return Color(randf(), randf(), randf())

func random_color_hsv() -> Color:
    return Color.from_hsv(randf(), randf(), randf())

func my_random_color() -> Color:
    return Color.from_hsv(
        snapped(randf(), 1.0 / 12.0),
        snapped(randf_range(0.6, 0.8), 0.1),
        snapped(randf_range(0.8, 1.0), 0.1),
    )

@jtnicholl
Copy link
Copy Markdown
Contributor

jtnicholl commented Nov 3, 2022

I was thinking of just using something like randfn to make the blacks, grays, and off-whites less common, I hadn't thought of using snapped. I'll keep it in mind, thanks.

@dalexeev
Copy link
Copy Markdown
Member

dalexeev commented May 3, 2025

Note that after all this time, typed dictionaries have appeared (method should return Dictionary[StringName, Color]). Also, you can use make_read_only() to avoid populating the dictionary on every call and still be sure that nobody changes the dictionary contents.

But I still wonder if we should introduce an API for getting member information for built-in types (since they are not registered in ClassDB). We definitely need not only a unified type system, but also a class representation (reflection).

@Mickeon
Copy link
Copy Markdown
Member Author

Mickeon commented May 3, 2025

Note that after all this time, typed dictionaries have appeared (method should return Dictionary[StringName, Color]). Also, you can use make_read_only() to avoid populating the dictionary on every call and still be sure that nobody changes the dictionary contents.

Lack of general interest means this won't be rebased, at least for now.

But I still wonder if we should introduce an API for getting member information for built-in types (since they are not registered in ClassDB). We definitely need not only a unified type system, but also a class representation (reflection).

Most definitely. Sounds like something that these would cover, even if indirectly:

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.

5 participants