Fix RemoteTransform2D could fail to update AnimatableBody2D's position or rotation

[Rindbee]

Configure the transform per condition, and then only set it once to prevent multiple NOTIFICATION_LOCAL_TRANSFORM_CHANGED notifications from being sent.

Simple fix #75486.

Before	After

[kleonc]

I'd say the question is why the scale was being set separately from the transform (after it) in the first place? AFAICT the reason is somehow historical: #9355 (comment). I don't see why incorporating the scale into the transform and setting just the transform (like in this PR) would be problematic.

LGTM.

[kleonc]

I suspect RemoteTransform3D needs a similar change (so just the transform is set at the end)? 🤔

godot/scene/3d/remote_transform_3d.cpp

Lines 64 to 107 in c29866d

	if (use_global_coordinates) {
	if (update_remote_position && update_remote_rotation && update_remote_scale) {
	n->set_global_transform(get_global_transform());
	} else {
	Transform3D our_trans = get_global_transform();
	if (update_remote_rotation) {
	n->set_rotation(our_trans.basis.get_euler_normalized(EulerOrder(n->get_rotation_order())));
	}
	if (update_remote_scale) {
	n->set_scale(our_trans.basis.get_scale());
	}
	if (update_remote_position) {
	Transform3D n_trans = n->get_global_transform();
	n_trans.set_origin(our_trans.get_origin());
	n->set_global_transform(n_trans);
	}
	}
	} else {
	if (update_remote_position && update_remote_rotation && update_remote_scale) {
	n->set_transform(get_transform());
	} else {
	Transform3D our_trans = get_transform();
	if (update_remote_rotation) {
	n->set_rotation(our_trans.basis.get_euler_normalized(EulerOrder(n->get_rotation_order())));
	}
	if (update_remote_scale) {
	n->set_scale(our_trans.basis.get_scale());
	}
	if (update_remote_position) {
	Transform3D n_trans = n->get_transform();
	n_trans.set_origin(our_trans.get_origin());
	n->set_transform(n_trans);
	}
	}
	}

[Rindbee]

I'd say the question is why the scale was being set separately from the transform (after it) in the first place? AFAICT the reason is somehow historical: #9355 (comment). I don't see why incorporating the scale into the transform and setting just the transform (like in this PR) would be problematic.

Maybe Transform2D at that time did not expose the setters of rotation/scale, just like the current Transform3D does not expose them.

[kleonc]

Maybe Transform2D at that time did not expose the setters of rotation/scale

Indeed there was no Transform2D::set_scale back then:

godot/core/math/math_2d.h

Lines 589 to 683 in a2e4b80

	struct Transform2D {
	// Warning #1: basis of Transform2D is stored differently from Basis. In terms of elements array, the basis matrix looks like "on paper":
	// M = (elements[0][0] elements[1][0])
	// (elements[0][1] elements[1][1])
	// This is such that the columns, which can be interpreted as basis vectors of the coordinate system "painted" on the object, can be accessed as elements[i].
	// Note that this is the opposite of the indices in mathematical texts, meaning: $M_{12}$ in a math book corresponds to elements[1][0] here.
	// This requires additional care when working with explicit indices.
	// See https://en.wikipedia.org/wiki/Row-_and_column-major_order for further reading.
	// Warning #2: 2D be aware that unlike 3D code, 2D code uses a left-handed coordinate system: Y-axis points down,
	// and angle is measure from +X to +Y in a clockwise-fashion.
	Vector2 elements[3];
	_FORCE_INLINE_ real_t tdotx(const Vector2 &v) const { return elements[0][0] * v.x + elements[1][0] * v.y; }
	_FORCE_INLINE_ real_t tdoty(const Vector2 &v) const { return elements[0][1] * v.x + elements[1][1] * v.y; }
	const Vector2 &operator[](int p_idx) const { return elements[p_idx]; }
	Vector2 &operator[](int p_idx) { return elements[p_idx]; }
	_FORCE_INLINE_ Vector2 get_axis(int p_axis) const {
	ERR_FAIL_INDEX_V(p_axis, 3, Vector2());
	return elements[p_axis];
	}
	_FORCE_INLINE_ void set_axis(int p_axis, const Vector2 &p_vec) {
	ERR_FAIL_INDEX(p_axis, 3);
	elements[p_axis] = p_vec;
	}
	void invert();
	Transform2D inverse() const;
	void affine_invert();
	Transform2D affine_inverse() const;
	void set_rotation(real_t p_phi);
	real_t get_rotation() const;
	_FORCE_INLINE_ void set_rotation_and_scale(real_t p_phi, const Size2 &p_scale);
	void rotate(real_t p_phi);
	void scale(const Size2 &p_scale);
	void scale_basis(const Size2 &p_scale);
	void translate(real_t p_tx, real_t p_ty);
	void translate(const Vector2 &p_translation);
	real_t basis_determinant() const;
	Size2 get_scale() const;
	_FORCE_INLINE_ const Vector2 &get_origin() const { return elements[2]; }
	_FORCE_INLINE_ void set_origin(const Vector2 &p_origin) { elements[2] = p_origin; }
	Transform2D scaled(const Size2 &p_scale) const;
	Transform2D basis_scaled(const Size2 &p_scale) const;
	Transform2D translated(const Vector2 &p_offset) const;
	Transform2D rotated(real_t p_phi) const;
	Transform2D untranslated() const;
	void orthonormalize();
	Transform2D orthonormalized() const;
	bool operator==(const Transform2D &p_transform) const;
	bool operator!=(const Transform2D &p_transform) const;
	void operator*=(const Transform2D &p_transform);
	Transform2D operator*(const Transform2D &p_transform) const;
	Transform2D interpolate_with(const Transform2D &p_transform, real_t p_c) const;
	_FORCE_INLINE_ Vector2 basis_xform(const Vector2 &p_vec) const;
	_FORCE_INLINE_ Vector2 basis_xform_inv(const Vector2 &p_vec) const;
	_FORCE_INLINE_ Vector2 xform(const Vector2 &p_vec) const;
	_FORCE_INLINE_ Vector2 xform_inv(const Vector2 &p_vec) const;
	_FORCE_INLINE_ Rect2 xform(const Rect2 &p_vec) const;
	_FORCE_INLINE_ Rect2 xform_inv(const Rect2 &p_vec) const;
	operator String() const;
	Transform2D(real_t xx, real_t xy, real_t yx, real_t yy, real_t ox, real_t oy) {
	elements[0][0] = xx;
	elements[0][1] = xy;
	elements[1][0] = yx;
	elements[1][1] = yy;
	elements[2][0] = ox;
	elements[2][1] = oy;
	}
	Transform2D(real_t p_rot, const Vector2 &p_pos);
	Transform2D() {
	elements[0][0] = 1.0;
	elements[1][1] = 1.0;
	}
	};

[Rindbee]

I suspect RemoteTransform3D needs a similar change (so just the transform is set at the end)? thinking

Yes, for AnimatableBody3D, it is also best to set them at once. But need Transform3D to expose these getter/setter first.