X Tutup
Skip to content

Make rotation gizmo white outline a 4th handle that rotates around the camera's view-axis#108608

Merged
Repiteo merged 1 commit intogodotengine:masterfrom
Open-Industry-Project:camera-view-axis-rotation
Nov 18, 2025
Merged

Make rotation gizmo white outline a 4th handle that rotates around the camera's view-axis#108608
Repiteo merged 1 commit intogodotengine:masterfrom
Open-Industry-Project:camera-view-axis-rotation

Conversation

@ryevdokimov
Copy link
Contributor

@ryevdokimov ryevdokimov commented Jul 14, 2025

Requires and includes: #108576

Common functionality in other 3D software that increases the usability of the rotation gizmo.

Made the white outline slightly larger so it's more distinct and easier to select.

Everything else should work as expected or at least how #108576 works.

2025-07-15.15-36-41.mp4

@ryevdokimov ryevdokimov requested a review from a team July 14, 2025 16:57
@ryevdokimov ryevdokimov force-pushed the camera-view-axis-rotation branch from d76bc2e to 2bf683f Compare July 14, 2025 17:03
@BlueCube3310 BlueCube3310 added this to the 4.x milestone Jul 14, 2025
@ryevdokimov ryevdokimov force-pushed the camera-view-axis-rotation branch 2 times, most recently from db6f08a to 9e6438c Compare July 15, 2025 11:35
@fire fire requested a review from a team July 15, 2025 20:41
@ryevdokimov ryevdokimov force-pushed the camera-view-axis-rotation branch from 9e6438c to c6bae67 Compare July 24, 2025 16:06
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

Code looks good to me.

@ryevdokimov ryevdokimov force-pushed the camera-view-axis-rotation branch from c6bae67 to cc1389a Compare July 29, 2025 14:00
@Repiteo Repiteo modified the milestones: 4.x, 4.6 Jul 31, 2025
@ryevdokimov ryevdokimov force-pushed the camera-view-axis-rotation branch from cc1389a to ab24ce1 Compare October 1, 2025 03:25
@ryevdokimov
Copy link
Contributor Author

Rebased since #108576 was merged.

@ryevdokimov ryevdokimov force-pushed the camera-view-axis-rotation branch 2 times, most recently from 70c6ad3 to 366170b Compare October 1, 2025 05:21
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

I tested shearing and multiple selections.

The shearing case is handled to be fine:

sh.mp4

The combination of multiple selections and rotation in local mode lacks consistency with axis-specified rotation and should be fixed; Local mode rotation uses each origin of the node.

Axis = Y (Correct, use each node's origin):

locrot_y.mp4

Axis = ViewPlaneNormal (Incorrect, use only primary selected node's origin):

locrot_view.mp4

I assume this can likely be fixed by passing the origin individually.

sh_3d.zip

@ryevdokimov ryevdokimov force-pushed the camera-view-axis-rotation branch from 366170b to 47e57fe Compare October 10, 2025 03:31
@ryevdokimov
Copy link
Contributor Author

Should be fixed now.

2025-10-09.20-12-45.mp4

@ryevdokimov
Copy link
Contributor Author

Looks like there is some shearing when multiple nodes are selected now, so I will need to look into that.

@TokageItLab
Copy link
Member

When multiple selections are made, it should be correct behavior for each selection to be sheared individually.

@ryevdokimov
Copy link
Contributor Author

ryevdokimov commented Oct 11, 2025

Oops I didn't pay attention to the scales across parents close enough in my testing.

I think things are fine, let me know if I missed anything.

@TokageItLab
Copy link
Member

TokageItLab commented Nov 9, 2025

As I mentioned above, in local mode, the XYZ rotation gizmo references each node's local axes.

When multiple nodes are selected, the gizmo displayed is based on the last selected node.

image image

Considering this, the 4th gizmo should extract the local axes relative to the last selected node from that plane and apply them to the other nodes during multiple selection. In other words, in the case shown in the following video, the Y rotation and the rotation applied by the 4th gizmo should be equal.

Godot.Engine.2025.11.09.-.17.24.03.05.mp4

sh_3d.zip

In other words, the current local mode with 4th gizmo is inconsistent as the origin is local while rotation is global.

@ryevdokimov
Copy link
Contributor Author

ryevdokimov commented Nov 9, 2025

Wouldn't that require #104233 as a prerequisite and its function enabled to make sense? Right now, local mode just means transform each node relative to its own local coordinates, and the 4th gizmo is always facing the camera, so it effectively behaves as a global transform.

@TokageItLab
Copy link
Member

TokageItLab commented Nov 9, 2025

@ryevdokimov If #104233 is implemented and that option is enabled, the current 4th gizmo behavior is correct. Conversely, after implementing #104233 and that option is enabled, the y rotation in the above video should follow the current 4th gizmo's behavior.

However, since #104233 is currently disabled in this PR, so the y rotation is correct and the 4th gizmo's movement is incorrect now. This PR must be fixed assuming that option #104233 is disabled.

So if we implement #104233, you should reintroduce the current 4th gizmo behavior as the optional behavior when that option is enabled.

Right now, local mode just means transform each node relative to its own local coordinates, and the 4th gizmo is always facing the camera, so it effectively behaves as a global transform.

In other words, we must select the ring's normal as the rotation axis in the Local coordinate.

image

In the above case, the rotation axis for the 4th gizmo and the y rotation gizmo are differed.

image

However, in the above case, the rotation axis for the 4th gizmo and the y rotation gizmo must be the same.

As long as option #104233 is disabled, that axis must be treated as a local rotation axis for each node. The current behavior always treats the 4th gizmo as the global rotation axis, so it is wrong for the behavior when #104233 is disabled.

@ryevdokimov
Copy link
Contributor Author

If #104233 is implemented and that option is enabled, the current 4th gizmo behavior is correct.

Ah yes, that makes sense actually, so the converse of that is what clarifies it.

It's just a little confusing because I interpreted the correct behavior as how each node should behave as if it were selected individually in local mode, but when put in that context and if the other PR gets merged what you're saying is the correct way to differentiate the behavior between the two modes, and without that PR the default should be as if it were off.

Thanks for the explanation.

@ryevdokimov ryevdokimov force-pushed the camera-view-axis-rotation branch from 47e57fe to 8778080 Compare November 10, 2025 01:42
@ryevdokimov
Copy link
Contributor Author

Alright, try it now.

2025-11-09.19-05-09.mp4

@ryevdokimov ryevdokimov force-pushed the camera-view-axis-rotation branch from 8778080 to 30311ab Compare November 10, 2025 02:02
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

I feel the behavior is now working properly.

I would point out the comment format (regarding caps and periods), redundant code, and unnecessary changes.

@ryevdokimov ryevdokimov force-pushed the camera-view-axis-rotation branch from 30311ab to f26e0d8 Compare November 10, 2025 05:37
@ryevdokimov ryevdokimov force-pushed the camera-view-axis-rotation branch from f26e0d8 to 3a34350 Compare November 10, 2025 05:47
@ryevdokimov
Copy link
Contributor Author

Feedback addressed (comments, unnecessary changes, organization and naming/clarity).

Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

LGTM :)

@TokageItLab TokageItLab requested a review from Repiteo November 13, 2025 00:19
@Repiteo Repiteo merged commit d933313 into godotengine:master Nov 18, 2025
39 of 40 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 18, 2025

Thanks!

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.

6 participants

X Tutup