Inconsistent behavior when rotating control nodes

ababen1 commented 3 weeks ago

Tested versions

Reproduciable in 4.3 stable, not reproduciable in 4.2.2 stable

System information

Windows 10

Issue description

Rotating a control node doesn't rotate on the pivot offset as expected if you change the node's position while rotating

Steps to reproduce

Open the attached project in godot 4.3 and run. click anywhere to pick up the icon and right click to rotate it. you will see it does not rotate on the pivot if you try to rotate while the icon is picked up. However, it does rotate correctly if you right click without dragging
Open the attached project in godot 4.2.2 and do the same, you will see it does rotate on the pivot as expected, in both cases.

Minimal reproduction project (MRP)

RotationTest.zip

AThousandShips commented 3 weeks ago

Can't fully confirm but might be related to:

Could be a consequence of either fix, unsure if 4.2.2 or 4.3 behavior is intended (i.e. if this is a bug, or if you were relying on bugged behavior in 4.2.2 that was fixed)

CC @kleonc @Rindbee @Sauermann

Rindbee commented 3 weeks ago

Probably because Control::set_rotation_degrees() doesn't take pivot_offset into account.

https://github.com/godotengine/godot/blob/da5f39889f155658cef7f7ec3cc1abb94e17d815/scene/gui/control.cpp#L1552-L1566

It might be better to provide a dedicated rotate method.

kleonc commented 3 weeks ago

In 4.2.2 (and earlier) there is a discrepancy between what global position "means" in context of Control.set_global_position and Control.get_global_position.

unscaled, unrotated	pivot at center, scaled, rotated

For a TextureRect like above:

Control.get_global_position would return the yellow point (equivalent to global_transform.origin).
Control.set_global_position would expect you to pass the red point, aka the global position of an unrotated, unscaled rect (according to the pivot).

Because of the above, doing global_position = global_position in GDScript was resulting in moving the given Control scaled/rotated around non-default pivot, this was reported in #87354. I think we all agree that's a buggy unexpected behavior.

original	after `global_position = global_position` v4.2.2.stable.official[15073afe3]

global_position = global_position is equivalent to set_global_position(get_global_position())
get_global_position() returns original_yellow
set_global_position(original_yellow) moves control so red is at original_yellow

The above discrepancy / buggy behavior was ultimately fixed in 4.3, by #89502 (#87432 was incorrect and caused #89497). It made both Control.set_global_position and Control.get_global_position return/expect the "yellow points" (aka global_transform.origin).

Hence this is expected that MRP which is conceptually doing global_position = "red point" is working in 4.2.2, but not in 4.3. In 4.3 a "yellow point" would need to be passed instead.

After changing main.gd in the MRP to:

extends Control

@onready var rotating: TextureButton = $Rotating

var is_dragging = false

func _process(delta: float) -> void:
    if Input.is_action_just_pressed("click"):
        is_dragging = !is_dragging
    if Input.is_action_just_pressed("right_click"):
        rotating.rotate()
    if is_dragging:
        # (1)
        #var half_size: Vector2 = rotating.size / 2
        #rotating.global_position = get_global_mouse_position() - half_size

        # (2)
        #var half_size_global: Vector2 = rotating.get_global_transform().basis_xform(rotating.size / 2)
        #rotating.global_position = get_global_mouse_position() - half_size_global

        # (3)
        #var half_size_parent_global: Vector2 = (rotating.get_global_transform() * rotating.get_transform().affine_inverse()).basis_xform(rotating.size / 2)
        #rotating.global_position = get_global_mouse_position() - half_size_parent_global

    %RectLabel.text = "Rect: %s" % str(rotating.get_global_rect())

Here's a visualization of the behavior after uncommenting section (1) or (2): (black shows what's assigned to `rotating.global_position`) uncommented section	v4.2.2.stable.official [15073afe3]	v4.3.stable.official [77dcf97d8]
(1)
(2)

But note that even though the code from (1) seemingly works in 4.2.2, it's not guaranteed to always work. For simple case sure it does. But in general to properly obtain the unrotated/unscaled position something like (3) would need to be done instead.

Here's for example what happens after rotating and scaling the parent `Main` Control in the MRP, notice that for (1) mouse is no longer at the center (which is the pivot): uncommented section	v4.2.2.stable.official [15073afe3]
(1)
(3)

Thus it's not like doing things properly was simpler in 4.2.2.

To sum up, in 4.3 the behavior of Control.set_global_position/Control.get_global_position is more consistent, global_position = global_position doesn't move the Control.

However, note that it could have been potentially fixed the other way around. Instead of changing Control.set_global_position to expect a "yellow position" (done in #89502), an alternative would be to change Control.get_global_position to return the "red position". Whether one was preferable over the other I'm not sure.

If Control.set_global_position/Control.get_global_position used "yellow points" (current behavior in v4.3.stable.official [77dcf97d8]), then:

global_position is equivalent to global_transform.origin. This is consistent with Node2D and Node3D.
global_position is not equivalent to parent_global_transform * position. This is inconsistent with Node2D and Node3D.

If Control.set_global_position/Control.get_global_position used "red points" (alternative to current behavior), then:

global_position is not equivalent to global_transform.origin. This is not consistent with Node2D and Node3D.
global_position is equivalent to parent_global_transform * position. This is consistent with Node2D and Node3D.

In the end we can't make everything consistent with Node2D/Node3D because of how for Control there's an additional pivot "between" its parent and local coordinate spaces. So, again, not sure if one is preferable over the other.

Regardless of whether we will leave the behavior as in v4.3.stable.official [77dcf97d8], or change it to the alternative (after concluding it's better for some reason), we could of course potentially consider adding some helper methods/properties to Control (or just documenting better? :thinking:). So suggestions/proposals about what could be improved are of course welcomed (implementation shouldn't be a problem). :slightly_smiling_face:

Rindbee commented 3 weeks ago

When the pivot is not at the default position (top left corner, i.e. position), rotating around the pivot should change the entire transform (correcting position/global_position).

This may be more mathematical, but the values may not be very intuitive.

godotengine / godot