Rename color constants

[Vivraan]

Fix naming of certain constants and color name strings in C++ and C#.

Advise on how to parse constants in UPPERCASE_NAMING style for gdscript Color Constants.

[bojidar-bg]

Advise on how to parse constants in UPPERCASE_NAMING style for gdscript Color Constants.

Color constants are registered here:

godot/core/variant_call.cpp

Lines 2334 to 2337 in ec678c5

	_populate_named_colors();
	for (Map<String, Color>::Element *color = _named_colors.front(); color; color = color->next()) {
	_VariantCall::add_variant_constant(Variant::COLOR, color->key(), color->value());
	}

In order for the color name to be uppercase, the second argument to add_variant_constant (currently color->key()) should be an uppercase string.

To do this you might (a) modify _populate_colors() to use upper-case color names or (b) use String::to_upper to transform the color name.

[Vivraan]

I just added the entire bunch of names as new entries. Old code needn't break. Okay well there is some breaking because cornflower is cornflowerblue and CORNFLOWER_BLUE instead. Fixing that. Added that back, as well as the incorrect versions of the corrections I had provided. Shouldn't break compat, @Calinou.

Spaces are marked as underscores in C++ and C# pascalcasing is updated as per X11 color names, Wikipedia.

My other option was to entirely replace the constants and introduce new to_upper() calls, breaking old code and introducing inconsistencies with the C# color name access API.

[Vivraan]

However this doesn't follow the consistency that @aaronfranke wanted to provide (Colors.Red in C# and Color.RED in GDScript).

While obviously better done in C++ for performance reasons, I don't see why Colors.RED or for that matter all of these constants can't be added in as a plugin instead - apart from use in VSScript. Maybe it's better to be pedantic about this now so that this oddity doesn't propagate into the future?

[aaronfranke]

I don't see why Colors.RED or for that matter all of these constants can't be added in as a plugin instead

They could be, but this is a very simple feature so it's easy enough to just have it built-in, and arguably commonly used (the colors are used less othen than the constants like Vector2.UP, but they are both sets of standardized constants so we should treat them the same).

When it comes to Color.RED vs Colors.RED, the former isn't terrible, but IMO with the sheer amount of constants it makes sense to separate them. In C# there is also the concern of Color.Red etc obscuring unrelated methods like Color.Color8, and in GDScript these are just defined as global methods, but perhaps they should be moved to Color in GDScript too (and if so, that's a strong case for Colors.RED).

[Vivraan]

When it comes to Color.RED vs Colors.RED, the former isn't terrible, but IMO with the sheer amount of constants it makes sense to separate them. In C# there is also the concern of Color.Red etc obscuring unrelated methods like Color.Color8, and in GDScript these are just defined as global methods, but perhaps they should be moved to Color in GDScript too (and if so, that's a strong case for Colors.RED).

Would this be done by adding another Variant Type in variant_call.cpp?

[aaronfranke]

As for the constants, I'm not sure of the best way to implement them. Take a look at the PR that added them. If you can't figure something out, I'll try myself later.

[Vivraan]

Should I reintroduce the old names for 3.2 compat? I guess a commit with that can go into the 3.2 branch but one without it can go into 4.0 only.

[Vivraan]

@Vivraan Sorry for the delay, I forgot about this. Anyway, this needs to be rebased on the latest master before it can be reviewed.

@aaronfranke Looks like the files have been moved to core/math and the types have been renamed without any changes to the colour names. I guess I'll have to redo some of this...

[Vivraan]

@reduz could you review this? Seems you were the author of the moving change.

Editing your change will definitely break compat for 3.2, which I wish to target but in practice I'm not.

[Vivraan]

As for the constants, I'm not sure of the best way to implement them. Take a look at the PR that added them. If you can't figure something out, I'll try myself later.

I believe @reduz has addressed this in the latest changes relating to code reorganisation.

[Vivraan]

Looks like my transformation code is wasteful. I'll need to provide the names directly in color_names.inc, or wherever the colour names are registered.

[aaronfranke]

@Vivraan This can't be done to 3.2 because it breaks compatibility.

What you have here is a merge, but you need a rebase and ideally squash, where it says "Commits 29" it should say "Commits 1". See this article for more information.

[Vivraan]

@Vivraan This can't be done to 3.2 because it breaks compatibility.

What you have here is a merge, but you need a rebase and ideally squash, where it says "Commits 29" it should say "Commits 1". See this article for more information.

It looks like this because the files I was trying to change have been (re)moved. Atm I'm waiting for Reduz to review this change, since it looks like the way these constants are defined or introduced has been completely redone.

[dalexeev]

@Vivraan I opened alternative PR #45144 and listed you as a co-author.

[Vivraan]

@Vivraan I opened alternative PR #45144 and listed you as a co-author.

Amazing! I'm glad the named corrections have been updated to the latest API!
Are the C# colours handled in the same way as GDScript or are they named in PascalCase?

[dalexeev]

Are the C# colours handled in the same way as GDScript or are they named in PascalCase?

GDScript: Color.LIGHT_SEA_GREEN
C#: Colors.LightSeaGreen

[aaronfranke]

For the record: #45144, which replaces this PR, has been merged.