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 Camera2D's *_screen_center and *_position to get_screen_center_position and get_target_position #64880

Merged
merged 1 commit into from
Sep 6, 2022

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Aug 25, 2022

As brought up in: #54161 (comment), #54161 (comment).

I am fuming, how did I forget to write this sooner, dang it! I was the one complaining about it!

These Camera2D properties have very confusing and redundant names. Most egregiously, last time I edited the documentation, I had to carefully tip-toe around get_camera_position() to make sure it wasn't being confused with Node2D.position. This PR attempts to address that.

get_camera_screen_center -> get_screen_center_position
get_camera_position -> get_target_position

Also improves the documentation of these two.

Previously this PR was get_camera_screen_center -> get_screen_center
Note: Unlike other PRs similarly to this, I left their internal names untouched. See #63773.

@Mickeon Mickeon requested review from a team as code owners August 25, 2022 10:10
@Mickeon Mickeon force-pushed the rename-camera-position branch from 1c129ca to 1e13218 Compare August 25, 2022 10:15
@KoBeWi KoBeWi added this to the 4.0 milestone Aug 25, 2022
@KoBeWi
Copy link
Member

KoBeWi commented Aug 25, 2022

I was thinking if get_camera_position() is even needed. It does return some variable, but it's not very useful. It's a weird mix between global_position() and get_camera_screen_center().

@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 25, 2022

It definitely is. I've used this method for fine-tuning, displaying the target position during debugging to better understand the internal behaviour, and in particular, I've required it to restore the camera exactly as it was after saving and reloading from a PackedScene. I really struggled to find it, however, because of the poor naming.

Definitely shouldn't be exposed as a full property, but it's really nice to have and see as is.

@akien-mga
Copy link
Member

CC @madmiraal @lawnjelly

doc/classes/Camera2D.xml Outdated Show resolved Hide resolved
@Ansraer
Copy link
Contributor

Ansraer commented Aug 25, 2022

During the meeting, we agreed to remove the camera suffix, but couldn't agree on the exact naming.
get_screen_center_global_position and get_target_global_position were proposed, but we couldn't agree on whether that was too verbose.

@madmiraal
Copy link
Contributor

I think adding global is unnecessary. The camera only works in global coordinates. There are no equivalent methods returning local coordinates.

@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 26, 2022

So it should then be get_screen_center_position? Okiedokie

@Mickeon Mickeon force-pushed the rename-camera-position branch 2 times, most recently from 151513f to 7ca9ccf Compare August 26, 2022 09:54
@Mickeon Mickeon changed the title Rename Camera2D's *_screen_center and *_position to get_screen_center and get_target_position Rename Camera2D's *_screen_center and *_position to get_screen_center_position and get_target_position Aug 26, 2022
@Mickeon Mickeon force-pushed the rename-camera-position branch from 7ca9ccf to efb43f5 Compare August 26, 2022 09:58
`get_camera_screen_center` -> `get_screen_center_position`
`get_camera_position` -> `get_target_position`
@Mickeon Mickeon force-pushed the rename-camera-position branch from efb43f5 to ec96378 Compare August 26, 2022 10:12
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.

Approved in PR review meeting.

@akien-mga akien-mga merged commit adaec83 into godotengine:master Sep 6, 2022
@akien-mga
Copy link
Member

Thanks!

@Mickeon Mickeon deleted the rename-camera-position branch September 6, 2022 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Approved
Development

Successfully merging this pull request may close these issues.

5 participants