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

Reverse Camera2D.rotating to ignore_rotation #64359

Merged
merged 1 commit into from
Sep 21, 2022

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Aug 13, 2022

As partially brought up in #54161 (comment).

rotating is misleading, as Camera2D is affected by rotation and global_rotation like any other Node2D.

This PR reverses the boolean outright, and makes it clearer the Camera2D's view ignores its own rotation when it is now enabled.

This PR has been refactored. It used to simply rename rotating to rotating_view.

@golddotasksquestions
Copy link

What does this property do anyway and how do I use it?

The description does not really help. From this description I would assume this property locks the Camera2D onto a target and makes sure the Camera2D rotation follows the target. But there is no property to set a target.

If this would be a method instead and would take a target argument, I would also better understand, but that's not the case here either.

So all in all I don't see how rotating_view is any more helpful than rotating. Both names do nothing to help me understand intrinsically and the description is not helping either.

@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 15, 2022

I was considering overhauling Camera2D's descriptions (again) and names in general because they honestly feel misleading. Indeed, there's no such thing as a "camera target" because Camera2D merely inherits its transform from its parent, like any other Node2D. Renaming the property is a step in the right direction, as should be possibly renaming its other properties as well.

In this PR #55699 the explanations were heavily limited by how badly named some properties are, as well as how they've been previously described. There's no consistency now.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 15, 2022

You could update the description in this PR, to make it more complete.

@Mickeon Mickeon force-pushed the rename-camera-rotating branch from 9dd0c5d to f1a2bb8 Compare August 15, 2022 15:37
@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 15, 2022

Updated this specific description to be more explicit, too. Tell me what you may think.

@golddotasksquestions
Copy link

If true, the camera's rendered view is affected by its Node2D.rotation and Node2D.global_rotation.

Yeah that's a lot better! Thanks!

@golddotasksquestions
Copy link

golddotasksquestions commented Aug 15, 2022

On second thought, maybe also add rotation_degrees? Since this is the only one exposed in the Editor Inspector, and therefore maybe easier to find for people who want to test this property.
If [code]true[/code], the camera's rendered view is affected by its [member Node2D.rotation_degrees], [member Node2D.rotation] and [member Node2D.global_rotation_degrees], [member Node2D.global_rotation].

@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 15, 2022

There is no rotation_degrees and global_rotation_degrees in Godot 4. Even if there were, one could argue that description to be needlessly verbose.

@aaronfranke
Copy link
Member

How about can_rotate? This would make sense to me. If it's false, it can't rotate, so any rotation is discarded.

@Ansraer
Copy link
Contributor

Ansraer commented Aug 25, 2022

During the meeting it was agreed that this definitely needs a rename, but we couldn't agree on anything. Suggestions and discussion are welcome.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 18, 2022

What if instead we reverse the property and call it ignore_rotation?

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 18, 2022

That sounds great to me! It could also be called rotation_ignored so that the getter suits more. And perhaps it could all tidly fit into a "Rotation" group when #65776 is merged.

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 20, 2022

I was going to push the rename, but I realised that all other Camera2D have "enabled" booleans, instead. But at the same time, rotation_enabled feels wrong? Where to go from here?

@Mickeon Mickeon force-pushed the rename-camera-rotating branch from f1a2bb8 to 7260807 Compare September 20, 2022 15:06
@Mickeon Mickeon changed the title Rename Camera2D.rotating to rotating_view Reverse Camera2D.rotating to ignore_rotation Sep 20, 2022
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 20, 2022

The PR has been refactored. This PR reverses the boolean outright, and makes it clearer the Camera2D's view ignores its own rotation when it is now enabled.

@Mickeon Mickeon force-pushed the rename-camera-rotating branch from 7260807 to 09f24da Compare September 20, 2022 15:39
@Mickeon

This comment was marked as duplicate.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Looks ok, but I didn't test the converter code.

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 20, 2022

I am unable to test the converter code, but if given instructions on how to I could give it a try. I know right off the bat that it cannot be perfect on every situation, but fortunately, rotating is something that is very unlikely to be changed in code...

scene/2d/camera_2d.cpp Outdated Show resolved Hide resolved
`rotating` is misleading, as Camera2D is affected by `rotation` and `global_rotation` like any other Node2D

Updates description in the docs, as well.
@Mickeon Mickeon force-pushed the rename-camera-rotating branch from 09f24da to ee16de5 Compare September 20, 2022 23:52
@akien-mga akien-mga merged commit 7bf61e3 into godotengine:master Sep 21, 2022
@akien-mga
Copy link
Member

Thanks!

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 21, 2022

Reminder that this is worth accentuating with big caps someday because it's not just a simple rename this time around.

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.

7 participants