Remove unnecessary operations for set_rotation and rectangle collision

[FireBrandMint]

This pr is just 2 simple performance optimizations.

• set_rotation now doesn't re-normalized the normals produced by sin and cos.

• Rectangle collision doesn't normalize the 2nd pillar of transform anymore. Instead it picks the 1st already normalized pillar and simply does {x2 = -y1; y2 = x1} which gives the same result and is less costly.

[Spartan322]

This PR still needs to run clang-format.

[FireBrandMint]

This PR still needs to run clang-format.

Just did.

[Spartan322]

This will need a squash and I would suggest renaming the commit message to Remove unnecessary operations for set_rotation and rectangle collision

[FireBrandMint]

This will need a squash and I would suggest renaming the commit message to Remove unnecessary operations for set_rotation and rectangle collision

It's done. I'll do this from now on.

[SkogiB]

This will need a squash and I would suggest renaming the commit message to Remove unnecessary operations for set_rotation and rectangle collision

It's done. I'll do this from now on.

Awesome, we appreciate it a lot

[Spartan322]

Could you write tests in tests/core/math/test_transform_2d.h to always ensure set_rotation's behavior? Unfortunately godot physics doesn't have tests, and I'm not sure if we want to spend time writing tests just for this case when it be the only tests for it and its got so many other problems anyway, especially when we are considering replacing it with box2d. (which already has testing)

[FireBrandMint]

Could you write tests in tests/core/math/test_transform_2d.h to always ensure set_rotation's behavior? Unfortunately godot physics doesn't have tests, and I'm not sure if we want to spend time writing tests just for this case when it be the only tests for it and its got so many other problems anyway, especially when we are considering replacing it with box2d. (which already has testing)

I don't know if you guys are going to decide to completely replace transform2d but in case you don't: I don't think this needs a programmed test. The only thing that can make this go wrong is if Math::sin and Math::cos becomes inaccurate which is a completely different problem.

When it comes to a Vector2, setting an axis to +-sin(angle) and then making the other +-cos(same_angle) is already supposed to make it a normalized vector.

The reason for this part of the pr is that the old set_rotation was normalizing a normal, wasting resources... here:

(inlined set_size and added comments for an easier read)

void Transform2D::set_rotation(real_t p_rot) {
	Size2 scale = get_scale();
	real_t cr = Math::cos(p_rot);
	real_t sr = Math::sin(p_rot);
	//sets culumns[0] to the normal that represents p_rot
	columns[0][0] = cr;
	columns[0][1] = sr;
	//sets columns[1] to the normal that represents p_rot but it's the complete opposite and inverts x (still a normal)
	columns[1][0] = -sr;
	columns[1][1] = cr;
	//removed because this tries to change the normals in both columns into normals
	//it is the same result whether you remove this or not.
	columns[0].normalize();
	columns[1].normalize();
	//the rest just sets the scale for both pillars
	columns[0] *= p_scale.x;
	columns[1] *= p_scale.y;
}

[Spartan322]

Yeah the issue with Transform2D is this specific function isn't being tested anywhere even though it probably should be, since you're messing with it already it might as well be tested reinforcing it'll still spit out the right values, it doesn't just matter if this theoretically performs the same behavior, the tests reinforce expected behavior to reduce the chances of regressions, and which is trivial for this case anyway. Since you're already messing with a Transform2D method that isn't being tested we might as well be comprehensive and add it as well, it needs the test regardless.

[FireBrandMint]

Yeah the issue with Transform2D is this specific function isn't being tested anywhere even though it probably should be, since you're messing with it already it might as well be tested reinforcing it'll still spit out the right values, it doesn't just matter if this theoretically performs the same behavior, the tests reinforce expected behavior to reduce the chances of regressions, and which is trivial for this case anyway. Since you're already messing with a Transform2D method that isn't being tested we might as well be comprehensive and add it as well, it needs the test regardless.

Added the test, is it accurate enough?