Allow some non-integer built-in constants in gdscript #14704

poke1024 · 2017-12-15T15:57:13Z

I'm always missing named compile-time color constants for Colors in the editor. This adds them:

Since adding full fledged static constants to gdscript (I started) might entail some longer discussions due to various side effects (time of initialization, change of byte code for singletons, etc.), this PR's implementaiton is just adding some syntactic sugar by expanding Color.xyz into Color(r, g, b, a) in the gdscript parser.

akien-mga · 2017-12-15T16:06:34Z

That feature is already implemented as ColorN("red"), I don't think we should add another way to do the same thing: http://docs.godotengine.org/en/latest/classes/class_@gdscript.html#class-gdscript-colorn

poke1024 · 2017-12-15T16:11:10Z

ColorN doesn't have code completion.

Calinou · 2017-12-15T20:37:17Z

Considering that you might not want to use "pure" color values (which look unpleasant to the human eye), is this worth implementing? You could define constants using the const keyword in a script instead.

leoddd · 2017-12-16T01:17:23Z

Is there a way to add custom entries to ColorN in-game?
If not, then I'd say that this PR could be used instead of ColorN, just because it's actually more intuitive to access different built-in colors from the Color class, rather than a built-in function; and the code-completion is helpful as well, because I would've looked for "lime" instead of "yellowgreen".
If adding custom entries to ColorN is a thing that could be considered then I'd rather go that route though. Would make consistent color scheming across the game easier, at least.

poke1024 · 2017-12-17T09:04:36Z

@leoddd That's my stance, basically. If there's a static, curated list of colors Godot ships with, I'd like to have it as editor hint in some way.

bojidar-bg · 2017-12-17T09:32:33Z

What about adding code completion to ColorN instead?
Also, I doubt such constants' resolution should be put in GDScript, it would be better if they are accessed similar to the numeric constants (to allow for, e.g. Vector3.up and the list as well).

poke1024 · 2017-12-17T11:06:53Z

How would you add code completion to ColorN? It takes a string parameter - you'd need to add a completely different kind of code completion not available yet.

I agree Vector3.up (and others) would be nice. But that does suggest adding full static non-integer constants to gdscript, which I originally started but then stopped as it's a change I'm probably not fit to do in a way that will match Godot's compiler design; it probably involves changing Godot's byte code as it needs access to some sort of globals/singleton management, the only global being self AFAIK. One relatively simple approach would be having static x = 3 internally rewrite into static x = func return 3 and then internally rewrite abc.x into abc.x(); but that would not be a constant. It might be ok if we use this only internally and do not open definition of static constants to gdscript users.

On the other hand, if we do not expose this feature (definition on non-numeric static constants), one could generalize the approach of this PR to add Vector.up and similar builtin constants - I actually think that this approach wouldn't be so bad, as it's a small change and would allow for a set of non-integer static constants for some built-in classes.

poke1024 · 2017-12-17T13:53:12Z

PR is now cleaned up and much generalized (changed PR title to reflect this).

To sum up this new PR: instead of making all static built-in constants integers (as in current master), built-in constants may now be other types (basically any Variant types that do not bloat the constant table, see below).

As an example, this implements Vector3.UP, Vector3.DOWN, and so on (besides Color names).

All non-color constants show in the docs just as integer constants do:

Internally, in GDScriptParser, all constants are represented by a ConstantNode which AFAIK embeds the Variant in a constant table in the byte code - if this PR is followed, then there should be a guard to not allow any Variant but only some basic types like Vector2, Vector3, String that are ok to embed in this way.

What this is not: a full-fledged static constants solution for having any expression or object type as a static constant or that lets users define their own static constants.

bojidar-bg · 2017-12-17T15:20:25Z

core/variant.h

-	static void get_numeric_constants_for_type(Variant::Type p_type, List<StringName> *p_constants);
-	static bool has_numeric_constant(Variant::Type p_type, const StringName &p_value);
-	static int get_numeric_constant_value(Variant::Type p_type, const StringName &p_value, bool *r_valid = NULL);
+    static void get_constants_for_type(Variant::Type p_type, List<StringName> *p_constants);


Wrong indentation here, BTW.

ghost · 2017-12-18T00:35:03Z

This is just nitpicking, but shouldn't Vector3.BACK and Vector3.FORWARD be swapped around? I got the impression that in a right-handed coordinate system, from a perspective where x-positive would be "right" and y-positive "up", z-negative would be "forward". Here's a reference that I guess supports this.

ghost · 2017-12-18T00:49:16Z

Your Vector2 constants also seem to have this problem; in the 2D view, Vector2(0, -1) is "up" and Vector2(0, 1) is "down".

akien-mga · 2017-12-18T06:33:17Z

This is just nitpicking, but shouldn't Vector3.BACK and Vector3.FORWARD be swapped around? [...] Your Vector2 constants also seem to have this problem; in the 2D view, Vector2(0, -1) is "up" and Vector2(0, 1) is "down".

Proved the point I made every time such constants were asked for that they're not improving code quality. If people don't understand how the coordinate system works, having such helpers just makes code less readable and more error prone; whatever is Vector3.RIGHT when you're looking towards (5.24, -12.13, -2.0)?

Still, if people really want those, why not, they've been requested enough times to show that some people think differently from me ;) But I wouldn't extend GDScript features like non-integer built-in constants just for those.

poke1024 · 2017-12-18T13:35:39Z

@Homer666 Not an expert on this one, but here's some thoughts on this confusing topic.

If you'd pick a right-handed coordinate system and you'd define forward in a negative numeric direction, then forward is not a good name for decreasing in z IMHO.

The reason is, I believe, that most people tend to think in left-handed systems; and even though OpenGL is right-handed in object space, you often find camera views in OpenGL that are configured to left-handed systems (https://stackoverflow.com/questions/4124041/is-opengl-coordinate-system-left-handed-or-right-handed).

I agree with @akien-mga: all these terms lose their meaning if you're not in a standard camera frame. Only if you're in a standard camera in OpenGL object space, forward is -z.

I think you wantright, up and forward to correspond to numerically increasing values in the respective axes, because that is what most people intuitively think. And that's left-handed.

Unity also follows this convention:
https://docs.unity3d.com/ScriptReference/Vector2.html
https://docs.unity3d.com/ScriptReference/Vector3.html

EDIT To substantiate that claim a bit, think of a person knowing no math in front of a paper with a point. Now tell that person to go - from that point - one step right, one step up, and one step forward, and see where the person ends up. This will always be left-handed.

poke1024 · 2017-12-18T13:37:53Z

Added a few more constants for illustration of what might make sense.

ghost · 2017-12-18T18:03:22Z

@poke1024 I do agree that left-handed would come naturally to most people, but I don't know if deviating from the way Godot works is really a good idea.

From what I can tell, Unity follows this convention because it actually uses a left-handed coordinate system for everything, so having forward be +z makes perfect sense there. Its 2D view also seems to just be the 3D world with the z-axis flattened, so +y is still up as it was in 3D.

Godot's two different coordinate spaces don't adhere to these two things. Identifying and understanding these little coordinate quirks have always been a requirement to using Godot beyond just plopping down Control nodes, so this won't really change anything beyond having to remember that some of the Vector constants are backwards...

poke1024 · 2017-12-18T19:14:52Z

I understand, and indeed Unity seems to be consistenty left-handed. So, Vector2.UP should be what Godot uses as "up" in the standard 2d view, i.e. (0, -1, 0). But then I also really get @akien-mga's point now.

I guess I should change the constants to the Godot way and users could be informed that these constants only make sense if they consistently set up their local transformation frames in a way that adheres to these convention.

The main question remains though if this PR will ever see the light of day :-)

ghost · 2017-12-18T19:41:46Z

I hope it does! It's a good PR!!!

Anyway, regarding Vector3 specifically.... wrapping your head around the coordinate space is a bump that people will probably run into even without the constants; when I started playing around with 3D, my characters were facing z+ which caused them to move "left" when x+ increased, which tripped me up a little. I think that having this explicitly stated as a constant or something would help more than it would hinder honestly.

poke1024 · 2017-12-19T19:10:10Z

Apart from experimenting with a few other constants (more like a brainstorm for anyone evaluating this PR) I've turned the color constants to upper case for consistency with other constants and made them appear in the docs:

akien-mga · 2018-01-04T14:03:25Z

Thanks a lot for your contribution!
We are now entering a strict release freeze for Godot 3.0 and will only consider major bug fixes. We won't merge new features and enhancements anymore until 3.0 is released.

Moving this PR to the 3.1 milestone, to be reviewed once the release freeze is lifted. It could eventually be cherry-picked for a future 3.0.1 maintenance release if it doesn't change the user-facing APIs and doesn't compromise the engine's stability.

aaronfranke · 2018-06-16T20:45:58Z

core/variant_call.cpp

+	_VariantCall::add_variant_constant(Variant::VECTOR3, "UP", Vector3(0, 1, 0));
+	_VariantCall::add_variant_constant(Variant::VECTOR3, "DOWN", Vector3(0, -1, 0));
+	_VariantCall::add_variant_constant(Variant::VECTOR3, "FORWARD", Vector3(0, 0, -1));
+	_VariantCall::add_variant_constant(Variant::VECTOR3, "BACK", Vector3(0, 0, 1));


Great work, although I'd also like to suggest adding "ONE" and maybe "NEGONE" for feature parity with the C# built-ins I added: https://github.com/godotengine/godot/blob/master/modules/mono/glue/Managed/Files/Vector3.cs#L280

akien-mga · 2018-07-25T10:01:30Z

This needs a rebase after #19264, but we should likely decide whether we want the feature or not. WDYT @vnen @reduz?

Edit: We should also decide about the conflicting PR #15435.

reduz · 2018-07-27T12:39:41Z

core/color.h

@@ -187,6 +188,7 @@ struct Color {
 	static Color hex(uint32_t p_hex);
 	static Color html(const String &p_color);
 	static bool html_is_valid(const String &p_color);
+	static const Map<String, Color> &names();


I would simply not add this here, its not going to be used anyway, just put them together with the other constants inside Variant

reduz · 2018-07-27T12:40:45Z

This is very cool, if you can change what mentioned in the review, we can merge it

vnen · 2018-07-28T15:05:45Z

modules/gdscript/gdscript_parser.cpp

-				cn->datatype = _type_from_variant(cn->value);
-				expr = cn;
+				cn->value = Variant::get_constant_value(bi_type, identifier);
+				cn->datatype = _type_from_variant(cn->value);expr = cn;


Slight nitpick: there shouldn't be 2 statements in the same line.

karroffel · 2018-07-31T09:12:10Z

Bump, there is a very small formatting issue, but I would love to see this merged! 💙

akien-mga · 2018-08-20T08:48:35Z

I agree with the feature itself, but I think the list of constants that was added is way overkill, this should be reviewed before 3.1 is out. Things like BLACK_CIRCLE seem hardly useful, quite arbitrary (I don't think we want to add constants for all of Unicode), and risky (they break generating RST documentation from XML currently).

They were introduced in godotengine#14704 but need more discussion IMO, they don't strike me as core features that would have to be registered in Variant directly. Moreover, they currently break the documentation XML as string constants end up encoded as e.g. `value=""..""`.

poke1024 · 2018-08-22T05:19:47Z

@akien-mga You're right, some constants are a pure demo thing "look what you can do with constants", I forgot about them.

aaronfranke · 2020-01-28T18:11:27Z

Why aren't the Color constants UPPERCASE?

Also, in C# they are in a Colors static class, so that they are separate from the Color struct's other static members (like the Color8 constructor). While there are no other static members in GDScript's Color to warrant this change, it's worth considering Colors.RED etc for consistency.

bojidar-bg · 2020-01-28T18:54:23Z

(Note that changing the case of the constants might warrant a change in ColorN's documentation, due to #35581)

Vivraan · 2020-08-04T13:57:37Z

I can do some of these changes, I'm already invested in this.
#41019

akien-mga added feature proposal topic:gdscript labels Dec 15, 2017

akien-mga added this to the 3.0 milestone Dec 16, 2017

poke1024 force-pushed the colorconstants branch from 59e5bc0 to 27a7f42 Compare December 17, 2017 13:41

poke1024 changed the title ~~Static color constants for gdscript~~ Allow some non-integer built-in constants in gdscript Dec 17, 2017

bojidar-bg reviewed Dec 17, 2017

View reviewed changes

poke1024 force-pushed the colorconstants branch from 27a7f42 to bb0462e Compare December 17, 2017 15:49

poke1024 force-pushed the colorconstants branch from bb0462e to 7fbabb0 Compare December 18, 2017 13:37

poke1024 force-pushed the colorconstants branch 2 times, most recently from 148cac2 to 9a25997 Compare December 19, 2017 19:01

poke1024 force-pushed the colorconstants branch from 9a25997 to 9c5f930 Compare December 20, 2017 05:52

akien-mga removed this from the 3.0 milestone Jan 4, 2018

This was referenced Jan 7, 2018

Exposing names in color_names.inc #11164

Closed

Add standardised color names to doc and Color constructor accepting t… #15435

Closed

Nutriz mentioned this pull request Jan 7, 2018

Add standardised color names to doc #15455

Merged

akien-mga mentioned this pull request Jan 16, 2018

color HSV constructor #15782

Closed

aaronfranke reviewed Jun 16, 2018

View reviewed changes

reduz requested changes Jul 27, 2018

View reviewed changes

poke1024 force-pushed the colorconstants branch from 9c5f930 to 996fbae Compare July 28, 2018 10:17

poke1024 requested a review from vnen as a code owner July 28, 2018 10:17

vnen reviewed Jul 28, 2018

View reviewed changes

poke1024 force-pushed the colorconstants branch from 996fbae to 6a434dc Compare July 29, 2018 08:26

Allow some non-integer built-in constants in gdscript

ba974b8

poke1024 force-pushed the colorconstants branch from 6a434dc to ba974b8 Compare July 31, 2018 15:57

reduz merged commit c76f444 into godotengine:master Aug 10, 2018

KellyThomas mentioned this pull request Aug 11, 2018

mono: add constants to transform and vector structs #20901

Merged

akien-mga mentioned this pull request Aug 20, 2018

Remove circle/diamond String constants #21248

Merged

ghost mentioned this pull request Oct 6, 2018

"ONE" constant for Vector3/Vector2 #22806

Closed

aaronfranke mentioned this pull request Mar 6, 2019

Basis is missing the IDENTITY and ortho constants #26682

Closed

bojidar-bg mentioned this pull request Jun 20, 2019

Fix #2526: Rewrite GDScript Signals section with gamedev examples godotengine/godot-docs#2543

Merged

Calinou mentioned this pull request Jan 29, 2020

Proposal for renaming nodes for Godot 4.0 #30736

Closed

bojidar-bg mentioned this pull request Aug 4, 2020

Add named colors to GDScript/Visual Script/core. #7093

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow some non-integer built-in constants in gdscript #14704

Allow some non-integer built-in constants in gdscript #14704

poke1024 commented Dec 15, 2017

akien-mga commented Dec 15, 2017

poke1024 commented Dec 15, 2017

Calinou commented Dec 15, 2017 •

edited

Loading

leoddd commented Dec 16, 2017

poke1024 commented Dec 17, 2017

bojidar-bg commented Dec 17, 2017

poke1024 commented Dec 17, 2017

poke1024 commented Dec 17, 2017

bojidar-bg Dec 17, 2017

ghost commented Dec 18, 2017 •

edited by ghost

Loading

ghost commented Dec 18, 2017

akien-mga commented Dec 18, 2017

poke1024 commented Dec 18, 2017 •

edited

Loading

poke1024 commented Dec 18, 2017

ghost commented Dec 18, 2017

poke1024 commented Dec 18, 2017

ghost commented Dec 18, 2017 •

edited by ghost

Loading

poke1024 commented Dec 19, 2017

akien-mga commented Jan 4, 2018

aaronfranke Jun 16, 2018 •

edited

Loading

akien-mga commented Jul 25, 2018 •

edited

Loading

reduz Jul 27, 2018

poke1024 Jul 31, 2018

reduz commented Jul 27, 2018

vnen Jul 28, 2018

karroffel commented Jul 31, 2018

akien-mga commented Aug 20, 2018

poke1024 commented Aug 22, 2018

aaronfranke commented Jan 28, 2020

bojidar-bg commented Jan 28, 2020

Vivraan commented Aug 4, 2020 •

edited

Loading

Allow some non-integer built-in constants in gdscript #14704

Allow some non-integer built-in constants in gdscript #14704

Conversation

poke1024 commented Dec 15, 2017

akien-mga commented Dec 15, 2017

poke1024 commented Dec 15, 2017

Calinou commented Dec 15, 2017 • edited Loading

leoddd commented Dec 16, 2017

poke1024 commented Dec 17, 2017

bojidar-bg commented Dec 17, 2017

poke1024 commented Dec 17, 2017

poke1024 commented Dec 17, 2017

bojidar-bg Dec 17, 2017

Choose a reason for hiding this comment

ghost commented Dec 18, 2017 • edited by ghost Loading

ghost commented Dec 18, 2017

akien-mga commented Dec 18, 2017

poke1024 commented Dec 18, 2017 • edited Loading

poke1024 commented Dec 18, 2017

ghost commented Dec 18, 2017

poke1024 commented Dec 18, 2017

ghost commented Dec 18, 2017 • edited by ghost Loading

poke1024 commented Dec 19, 2017

akien-mga commented Jan 4, 2018

aaronfranke Jun 16, 2018 • edited Loading

Choose a reason for hiding this comment

akien-mga commented Jul 25, 2018 • edited Loading

reduz Jul 27, 2018

Choose a reason for hiding this comment

poke1024 Jul 31, 2018

Choose a reason for hiding this comment

reduz commented Jul 27, 2018

vnen Jul 28, 2018

Choose a reason for hiding this comment

karroffel commented Jul 31, 2018

akien-mga commented Aug 20, 2018

poke1024 commented Aug 22, 2018

aaronfranke commented Jan 28, 2020

bojidar-bg commented Jan 28, 2020

Vivraan commented Aug 4, 2020 • edited Loading

Calinou commented Dec 15, 2017 •

edited

Loading

ghost commented Dec 18, 2017 •

edited by ghost

Loading

poke1024 commented Dec 18, 2017 •

edited

Loading

ghost commented Dec 18, 2017 •

edited by ghost

Loading

aaronfranke Jun 16, 2018 •

edited

Loading

akien-mga commented Jul 25, 2018 •

edited

Loading

Vivraan commented Aug 4, 2020 •

edited

Loading