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

Rename Quat to Quaternion #45364

Merged
merged 1 commit into from
Jun 5, 2021
Merged

Conversation

madmiraal
Copy link
Contributor

I've been using quaternions for years and I didn't realise that Quat was Godot's quaternion; so I'm sure I'm not the only one. Changing it to Quaternion will eliminate any confusion.

@aaronfranke
Copy link
Member

It's called Quat in Unreal, and Quaternion in Unity. I don't really think this change is necessary, but it does make sense to me with either name. Maybe this should be discussed in a proposal.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #44989.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #45407 and #45373.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #44630 and #45847.

@BastiaanOlij
Copy link
Contributor

A rose by any other name is still a rose.

I don't like long names so I prefer Quat over Quaternion which I spell wrong 99% of the time I have to spell it. But that is a highly subjective opinion :)

@Riteo
Copy link
Contributor

Riteo commented Feb 26, 2021

I don't agree with this. This isn't the first case of an abbreviated name, just look at int for a very simple example. Also, people writing Qua will have autocomplete telling them that there's a Quat class, so this isn't a problem.

@madmiraal
Copy link
Contributor Author

This is about consistency within Godot. Godot does not use Vect or Vec, it uses Vector. Except for Quat and Rect2 (and possibly a few others) all classes use the full name of the object it represents. Note: We even have RectangleShape2D. Therefore, I believe we should have Quaternion and Rectangle or Rectangle2D instead of Quat and Rect2.

@Riteo
Copy link
Contributor

Riteo commented Feb 26, 2021

@madmiraal then this should be discussed on a broader sense IMO. Maybe you could add a proposal in #16863 if you haven't already.

@akien-mga
Copy link
Member

We discussed this on the Godot Contributors Chat #core channel a couple weeks ago, consensus seemed mostly against doing that change. Here's a permalink to that discussion but unfortunately accessing permalinks seems broken right now with Rocket.Chat: https://chat.godotengine.org/channel/core?msg=YUYmb1gtoIN4yOLhl

So here's a screenshot of the discussion (copy-paste with formatted chat is a pain :( ):

Screenshot_20210226_102301
Screenshot_20210226_102543

From a quick look at other engines:

  • Unity uses Quaternion
  • Unreal uses FQuat
  • Defold uses vmath.quat()
  • Cry Engine uses Quaternion, though they also seem to have a Quat operator
  • glm uses quat as typedef, quaternion as namespace
  • Ogre uses Quaternion
  • ThreeJS uses Quaternion()
  • Source 2 uses Quaternion
  • Irrlicht uses quaternion
  • id Tech 4 uses idQuat
  • Stride uses Quaternion
  • MonoGame uses Quaternion

I didn't check all existing engines but there does seem to be a general trend that most engines favor spelling it out fully, with a few notable exceptions: Unreal, id Tech, Defold, Godot.

@akien-mga
Copy link
Member

Maybe you could add a proposal in #16863 if you haven't already.

#16863 is not really an issue for discussion, it should mostly cater to consensual changes. Something like this needs its own proposal (or PR if we deem it's sufficient to reach a consensus) instead of another 20 comments in an issue with already several hundred of them :)

@hpvb
Copy link
Member

hpvb commented Feb 26, 2021

Thanks for the survey @akien-mga I have personally changed my mind in favor of changing the name now.

@madmiraal madmiraal requested a review from a team as a code owner February 27, 2021 10:51
@madmiraal
Copy link
Contributor Author

Rebased following merge of #45424.

@madmiraal
Copy link
Contributor Author

Rebased following 4ca1e73 and merge of #46431.

@Xrayez
Copy link
Contributor

Xrayez commented Mar 20, 2021

This is about consistency within Godot. Godot does not use Vect or Vec, it uses Vector. Except for Quat and Rect2 (and possibly a few others) all classes use the full name of the object it represents. Note: We even have RectangleShape2D. Therefore, I believe we should have Quaternion and Rectangle or Rectangle2D instead of Quat and Rect2.

I think we should strive for reducing verbosity with core types specifically, and let it be the Godot naming convention. The verbosity would also likely depend on how often a particular type is actually used throughout projects. I've previously created a proposal for renaming Rect2Rect: godotengine/godot-proposals#1671, no consensus was reached either, though...

If you ask me, I'd be in favor of Vector2/3Vec2/3 rename actually: https://github.com/godot-extended-libraries/godot-next/blob/master/addons/godot-next/2d/vec2.gd. This could also help to alleviate potential confusion with Vector container datatype, for instance: #43440 (comment). There was even a proposal for introducing Vector2/3 literals in GDScript: godotengine/godot-proposals#374.

@aaronfranke
Copy link
Member

@Xrayez Note: I created the vec2.gd and vec3.gd files for the purpose of giving users additional constants. This is not an endorsement of renaming the core types to Vec2 and Vec3, I prefer Vector2 and Vector3.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #45562.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #47470.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #44951.

@aaronfranke aaronfranke removed request for a team May 19, 2021 08:06
@akien-mga
Copy link
Member

For the reference, we discussed this rename in a PR review meeting today and couldn't really reach a consensus (like last time we tried). All contributors present seem fine with either name, and it's difficult to really estimate whether this change actually makes things better (while it's clear that it does change the API and break compat).

We figured that it's something which should likely go through a poll to see if there's any major preference for changing or keeping the name in the community. I'll give this a go.

@akien-mga
Copy link
Member

Twitter poll is at https://twitter.com/Akien/status/1400550652980387843
Also mirrored on Discord: https://discord.com/channels/212250894228652034/212637094194053121/850114470428278834

Both communities seem to be in favor of the rename with a ratio of 2:1 or more.

So I think we're edging towards doing the rename, it's more explicit and the community at large seems in favor of it (not massively, but significantly enough). There aren't a lot of compelling arguments not to do the rename, aside from avoiding a compat breakage.

@akien-mga akien-mga merged commit 6f7d45d into godotengine:master Jun 5, 2021
@akien-mga
Copy link
Member

Thanks!

@madmiraal madmiraal deleted the rename-quat branch June 5, 2021 11:58
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.

7 participants