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

Inconsistent behavior when rotating control nodes #95798

Open
ababen1 opened this issue Aug 19, 2024 · 5 comments
Open

Inconsistent behavior when rotating control nodes #95798

ababen1 opened this issue Aug 19, 2024 · 5 comments

Comments

@ababen1
Copy link

ababen1 commented Aug 19, 2024

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

  1. 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
  2. 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
Copy link
Member

AThousandShips commented Aug 19, 2024

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
Copy link
Contributor

Rindbee commented Aug 19, 2024

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

godot/scene/gui/control.cpp

Lines 1552 to 1566 in da5f398

void Control::set_rotation(real_t p_radians) {
ERR_MAIN_THREAD_GUARD;
if (data.rotation == p_radians) {
return;
}
data.rotation = p_radians;
queue_redraw();
_notify_transform();
}
void Control::set_rotation_degrees(real_t p_degrees) {
ERR_MAIN_THREAD_GUARD;
set_rotation(Math::deg_to_rad(p_degrees));
}

It might be better to provide a dedicated rotate method.

@kleonc
Copy link
Member

kleonc commented Aug 19, 2024

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
t5h0WYY164 eIkJCRVN1q

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[15073af]
eIkJCRVN1q Jc2utG6sB3
  • 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 [15073af] v4.3.stable.official [77dcf97]
(1) buYmLd3svS inyXsKJtEk
(2) xfK9XWCpF2 XsqOoZiMqk

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 [15073af]
(1) beVUoYNqI1
(3) 12ZRbOd7Kc

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 [77dcf97]), 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 [77dcf97], 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? 🤔). So suggestions/proposals about what could be improved are of course welcomed (implementation shouldn't be a problem). 🙂

@Rindbee
Copy link
Contributor

Rindbee commented Aug 19, 2024

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.

@akien-mga akien-mga changed the title Incosistent behavior when rotating control nodes Inconsistent behavior when rotating control nodes Aug 20, 2024
@tvister091
Copy link

tvister091 commented Oct 2, 2024

relevant in version v4.4.dev2.official [97ef3c8].
When moving a node, its pivot_offset position starts to be read as default, although I have it set to the center of the node. The problem is not detected if you change the local position, not the global one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants