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.

Sign up for GitHub

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

Jump to bottom

Rename Node3D's property translation to position #44198

Merged
merged 1 commit into from
Jun 4, 2021
Merged

Rename Node3D's property translation to position #44198

merged 1 commit into from
Jun 4, 2021

Conversation

madmiraal
Copy link
Contributor

@madmiraal madmiraal commented Dec 8, 2020 •
edited by aaronfranke
Loading

Currently, Node3D has a helper property called translation, which would be better called position as exists in Node2D. This PR renames Node3D's translation property, position.
Part of #16863.

Bugsquad edit: Fixes godotengine/godot-proposals#1557

@madmiraal madmiraal added this to the 4.0 milestone Dec 8, 2020
@madmiraal madmiraal requested a review from a team as a code owner December 8, 2020 18:06
@madmiraal madmiraal changed the title Rename Node3D's property translation to positon Rename Node3D's property translation to position Dec 8, 2020
@madmiraal madmiraal force-pushed the rename-translation-position branch from 29fd012 to 2e05bcf Compare December 8, 2020 18:07
@madmiraal madmiraal mentioned this pull request Dec 8, 2020
Closed
83 tasks
aaronfranke
aaronfranke requested changes Dec 9, 2020
View reviewed changes
doc/classes/Node3D.xml Outdated Show resolved Hide resolved
@madmiraal madmiraal force-pushed the rename-translation-position branch from 2e05bcf to bcab4c7 Compare December 9, 2020 08:04
aaronfranke
aaronfranke reviewed Dec 9, 2020
View reviewed changes
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

I support this change and the code looks good. Would also be nice to update the docs for Node2D.position to match though.

@aaronfranke aaronfranke mentioned this pull request Dec 17, 2020
Closed
@bluenote10
Copy link
Contributor

See my concerns in the proposal godotengine/godot-proposals#1557.

@madmiraal madmiraal force-pushed the rename-translation-position branch from bcab4c7 to f18c4e3 Compare February 13, 2021 08:23
@madmiraal
Copy link
Contributor Author

Rebased following merge of #45879.

@madmiraal madmiraal force-pushed the rename-translation-position branch from f18c4e3 to 84cdac7 Compare April 5, 2021 18:26
@madmiraal madmiraal requested review from a team as code owners April 5, 2021 18:26
@madmiraal
Copy link
Contributor Author

Updated documentation with --doctool ..

@akien-mga
Copy link
Member

We discussed this in a PR preview meeting today, and it had been discussed on and off in chat in previous months too. With the contributors present, we finally reached a consensus that this rename might indeed be a good idea to make things consistent.

translation is more "accurate" for experienced 3D developers, but Godot aims at being user-friendly also to less savvy developers, and position is still a correct description of what this represents. So we prefer standardizing on position for both 2D and 3D than the other way around, assuming that translation in 2D would confuse beginners even more. Other issues with translation is the homonymy with text/language translation (localization), and the similarity with transform which also starts with trans-.

akien-mga
akien-mga approved these changes Jun 3, 2021
View reviewed changes
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Good to merge after a rebase.

@madmiraal madmiraal force-pushed the rename-translation-position branch from 84cdac7 to a6e44bd Compare June 4, 2021 08:55
@akien-mga akien-mga merged commit 95ff547 into godotengine:master Jun 4, 2021
@akien-mga
Copy link
Member

Thanks!

@madmiraal madmiraal deleted the rename-translation-position branch June 4, 2021 09:56
@aaronfranke
Copy link
Member

@Calinou Surprisingly I don't actually think this is a problem. Exactly zero of the scenes in the demo projects have translation = in them, because 3D nodes currently store a Transform3D for their transformations. The only cases where translation exists in the scene files is with animation tracks that animate the translation property.

@Calinou
Copy link
Member

Calinou commented Jun 4, 2021 •
edited
Loading

@Calinou Surprisingly I don't actually think this is a problem. Exactly zero of the scenes in the demo projects have translation = in them, because 3D nodes currently store a Transform3D for their transformations. The only cases where translation exists in the scene files is with animation tracks that animate the translation property.

Indeed, I was mistaken as only transform is saved in scene files for nodes. I removed my earlier comment due to this.

@Calinou Calinou mentioned this pull request Aug 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga approved these changes

@aaronfranke aaronfranke aaronfranke approved these changes

Assignees
No one assigned
Labels
breaks compat enhancement topic:core
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

Change name of ' Translation ' to ' Position' in Transforms . .
5 participants
@madmiraal @bluenote10 @akien-mga @aaronfranke @Calinou