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

Remove circle/diamond String constants #21248

Merged
merged 1 commit into from
Aug 21, 2018

Conversation

akien-mga
Copy link
Member

They were introduced in #14704 but need more discussion IMO, they don't strike me as core features that would have to be registered in Variant directly.

What was approved in #14704 was mostly the way the new built-in constants were implemented, but the actual list of constants was not thoroughly reviewed. I'm not against adding constants to String but those 4 seem quite arbitrary to me, and set a precedent for "please add a constant for <insert favourite Unicode emoji here>" feature requests.

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=""..""`.
@akien-mga akien-mga force-pushed the remove-utf8-constants branch from 9ac4ac9 to f0b914f Compare August 20, 2018 22:24
@akien-mga
Copy link
Member Author

akien-mga commented Aug 20, 2018

Also removed the NodePath built-in constants as they broke documentation too, as they were encoded as invalid XML:

        <constants>
              <constant name="CURRENT" value=""."">
              </constant>
              <constant name="PARENT" value="".."">
              </constant>
        </constants>

I also don't see much use case for them, so it seems fine to remove for now, and reintroduce only if proven consensual and not breaking documentation.

@KellyThomas
Copy link
Contributor

KellyThomas commented Aug 21, 2018

While discussing this collection of constants I would like to know if people are happy with X, Y, Z in Plane.

The names have two issues I see:

  1. they may be confused with the instance fields x, y,z
  2. they refer to values I would normally think of as the YZPlane, XZPlane and XYPlane respectively.

Edit: I'm not sure how strongly this needs to be considered but the first point becomes a problem when porting to c# where the instance properties should be PascalCase. Given that we have an ecosystem with more than one language I encourage not choosing names that differ only by case.

@aaronfranke
Copy link
Member

aaronfranke commented Aug 21, 2018

@KellyThomas A better name would be PlaneXY etc. I'd prefer to keep the two letters on the right.

EDIT: #21253

@KellyThomas
Copy link
Contributor

Another name clash occurs between the member function Color.gray() and the constant Color.gray.

In GDScript this seems to resolve correctly but will cause issues in languages like c# where static and instance members can't share a common name.

@akien-mga
Copy link
Member Author

That Color.gray() function seems pretty useless, I'd be for removing it altogether when we can. Averaging R, G and B values does give a "grey" color, but it's hardly meaningful, a proper greyscale would use one of those methods: https://en.wikipedia.org/wiki/Grayscale#Converting_color_to_grayscale

@akien-mga
Copy link
Member Author

That Color.gray() function seems pretty useless, I'd be for removing it altogether when we can. Averaging R, G and B values does give a "grey" color, but it's hardly meaningful, a proper greyscale would use one of those methods: https://en.wikipedia.org/wiki/Grayscale#Converting_color_to_grayscale

I'll see with @reduz if we can rename it to to_grayscale() and use a proper conversion for that. Then C# could use ToGrayscale(), and we'd keep gray() as deprecated method only for GDScript.

@akien-mga akien-mga merged commit 238b70e into godotengine:master Aug 21, 2018
@akien-mga akien-mga deleted the remove-utf8-constants branch August 21, 2018 11:10
@vnen
Copy link
Member

vnen commented Aug 21, 2018

In GDScript this seems to resolve correctly but will cause issues in languages like c# where static and instance members can't share a common name.

GDScript will have this same restriction in the future, so it's really important to avoid these conflicts. Maybe even embed it in the bindings to generate a compilation error if it happens.

@akien-mga
Copy link
Member Author

See #21267 for the Color::gray() issue.

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.

4 participants