Refactors Get_Angle() and related procs

[Doubleumc]

About the pull request

To begin, fixes #5206. Get_Compass_Dir() needed atoms with valid coordinates, but didn't test for them. Adding some checks and get_turfs resolved that issue.

But then if Get_Compass_Dir() needed those checks, Get_Angle() did too. And get_angle() (lowercase!). And get_angle_raw(). And half of Get_Compass_Dir() was just duplicated code from Get_Angle(). And--

So I took inspiration from how TG and TGMC did it and refactored the lot.
https://github.com/tgstation/tgstation/blob/32fb42d19e0c763f7e5e3dafb9844d0f05e86630/code/__HELPERS/maths.dm#L1-L17
https://github.com/tgstation/TerraGov-Marine-Corps/blob/3963b2d5b89dd60b88a7a1534ee77bd654abe953/code/__HELPERS/unsorted.dm#L99-L159

REMOVED:

• get_angle_raw() - never used
• get_angle() - used 3 places, identical use case to Get_Angle()
• get_pixel_angle() - used once, replaced by delta_to_angle()

ADDED:

• angle_to_dir() - pulls out the only unique thing Get_Compass_Dir() actually did
  • only difference from TGMC is it retains our angle breakpoints instead of theirs
• delta_to_angle() - the only thing that actually calculates an angle, uses the 2-argument version of arctan()

CHANGED:

• Get_Angle() - removed the tile_bound branch (never used), added checks and get_turfs, position deltas passed to delta_to_angle()
  • only difference from TGMC is it retains return 0s instead of CRASHes, retains get_pixel_position_x/y, and passing to delta_to_angle()
• Get_Compass_Dir() - gutted, now just a convenience function of Get_Angle() piped into angle_to_dir()

Explain why it's good for the game

Having multiple unused or barely used functions trying to do the same thing makes code maintenance harder.

Testing Photographs and Procedure

• doesn't crash
• security tuner APC detection works
• SL/queen tracker works
• bullets face the right way

Changelog

🆑
fix: Fixed security tuner not dispalying direction to room's APC
refactor: Refactored code handling angles
/:cl:

[fira]

Does that change anything functionally speaking or should have no practical effects?

[Doubleumc]

The only potential difference is where get_angle() was replaced by Get_Angle().

get_angle() used (32 * start.x + start.pixel_x) while Get_Angle() uses get_pixel_position_x(start).

Since they're both calculating the pixel offset and get_pixel_position is just a more thorough solution to that, I can't think of a situation where a difference - if any - is less accurate instead of more.

Everything else is functionally or literally identical.

[Doubleumc]

Additional potential difference: projectile trajectory code used 280 as west. Now that it uses delta_to_angle() it will return 270 instead.

[fira]

WOOPS funny typo
Either way this looks good to me

[Red-byte3D]

pretty sure this is making slugs and buckshot stuns pull you towards marines

[Birdtalon]

This behaviour has been observed being caused by shotgun slugs while this PR has been on testmerge.

[Red-byte3D]

This behaviour has been observed being caused by shotgun slugs while this PR has been on testmerge.

buckshot also moves you weirdly, if its not a pb

[Doubleumc]

Thanks for the bug report. Looks like the cause is a combination of a few factors.

This is the relevant code being called on slug impact:

cmss13/code/datums/ammo/ammo.dm

Lines 151 to 153 in 72282fb

	/datum/ammo/proc/slam_back(mob/living/living_mob, obj/projectile/fired_projectile)
	/// Either knockback or slam them into an obstacle.
	var/direction = Get_Compass_Dir(fired_projectile.z ? fired_projectile : fired_projectile.firer, living_mob) //More precise than get_dir.

It gets the direction of impact based on Get_Compass_Dir() between the projectile and the target.

The original Get_Compass_Dir() implementation did not take pixel offsets into account. The new one calls Get_Angle() which does, as mentioned previously: #5212 (comment)

The pixel offsets for a projectile can be very offset since they're used to animate pixel movement across multiple tiles at a time. Further, the pixel offsets are only adjusted once per flight processed, while it is entirely possible for a projectile to move multiple tiles in a flight and any hit will occur in the middle of a flight.

When Get_Compass_Dir() is called on such a projectile its visual offset relative to its actual position is very unpredictable, sometimes by several tiles. And so slam_back() throws the target in an unpredictable direction.

My solution is to get_turf() both inputs to Get_Compass_Dir(), effectively discarding any such offsets. I believe that is in line with the proc's intent of "like get_dir(), but with better angles."

If there are additional issues of this nature I intend to restore the tile_bound argument to Get_Angle() and have Get_Compass_Dir() and other impacted procs use it.

[Birdtalon]

EDIT: I see you posted some fixes while I was writing this. 👍

Have done some investigation on this which would be interesting to get your input @Doubleumc .
See pic

I used this test scenario to test on master vs with this PR. I used this because we know with the position of these two mobs. The resulting knockback would be expected to be NORTHEAST.

These are the calculations I used for master

Master

Human x = 2, y = 2
Xeno x = 5, y = 5
Projectile x = 4, y = 4

(Projectile)
dy = 5 – 4 = 1
dx = 5 – 4 = 1
arctan(1/1) = 45
45 returns NORTHEAST
In game: NORTHEAST

Since we just use co-ordinates this is pretty simple and returns the expected result.

On this branch this is how my calculations result, this is kinda psudocode as I tried to do the math along with the execution of the code.

Get Angle
Human x = 2, y = 2
Xeno x = 5, y = 5 
Projectile x = 4, y = 4
dy = get_pixel_position_y(xeno) – get_pixel_position_y(projectile)
	get_pixel_position_y(xeno)
		. = (pixel_y = 0) + (base_pixel_y = 0) = 0
		. += 32 * 5 = 160
		. = 160
		. += (48 – 32) / 2 = 8
		. = 168
	get_pixel_position_y(projectile)
		. = 19 + 0 = 19
		. += 32 * 4 = 128
		. = 147
		. += 32 – 32 / 2 = 0
		. = 147
dy = 168 – 147 = 21

dx = get_pixel_position_x(xeno) – get_pixel_position_x(projectile)
	get_pixel_position_x(xeno)
		. = -12 + 0 = -12
		. += 32 * 5 = 160
		. = 148
		. += (48 – 32) / 2 = 8
		. = 156
	get_pixel_position_x(projectile)
		. = 22 + 0 = 22
		. += 32 * 4 = 128
		. = 150
		. += 32 – 32 / 2 = 0
		. = 150
dx = 156 – 150 = 6

delta_to_angle()
arctan(21,6) = 15.945
RETURNS NORTH

The drone moves north as per the pic above. But the expected behaviour is for the drone to move northeast.

Interestingly I also did the same math for the relative position of the Human and the Xeno.

Get Angle
Human x = 2, y = 2 | 
Xeno x = 5, y = 5 |
dy = get_pixel_position_y(xeno) – get_pixel_position_y(Human)
	get_pixel_position_y(xeno)
		. = (pixel_y = 0) + (base_pixel_y = 0) = 0
		. += 32 * 5 = 160
		. = 160
		. += (48 – 32) / 2 = 8
		. = 168
	get_pixel_position_y(human)
		. = 0 + 0 = 0
		. += 32 * 2 = 64
		. = 64
		. += (32 – 32) / 2 = 0
		. = 64
dy = 168 – 64 = 104



dx = get_pixel_position_x(xeno) – get_pixel_position_x(human)
	get_pixel_position_x(xeno)
		. = -12 + 0 = -12
		. += 32 * 5 = 160
		. = 148
		. += (48 – 32) / 2 = 8
		. = 156
	get_pixel_position_x(human)
		. = 0 + 0 = 0
		. += 32 * 2 = 64
		. = 64
		. += (32 – 32) / 2 = 0
		. = 64
dx = 156 – 64 = 92

delta_to_angle()
arctan(104,92) = 41.4694…
returns NORTHEAST

Bear in mind with this scenario I was shooting at the middle of the xeno sprite which resulted in the projectile being below the xeno, so maybe NORTH relative to the projectile in that case is expected? I haven't got time to do more testing but would be interesting to get your thoughts on the whole thing.
One thing worth noting although you may already know is that the usage of arctan() is changed with this PR as giving 2 arguments returns polar angle as the below quote from the Byond reference.

The two-argument form uses the polar angle. This angle starts at 0° for due east, and increases counter-clockwise from there. Therefore 1,0 has an arctangent of 0°, 0,1 is 90°, -1,0 is 180°, and so on. At point 0,0 the angle is undefined since it could be any angle, but arctan will return 0.

Have we accounted for this in angle_to_dir()?

My trigonometry is very rusty and you have been looking at this so would be interesting to hear your take.

--

[Doubleumc]

@Birdtalon thank you for the detailed investigation. As far as I can tell your math is entirely correct, and that NORTHEAST is indeed the expected knockback direction. Your work gives me further confidence that the issue was in the pixel offsets. For example, here is projectile-to-xeno calculation using the get_turf() fix:

Get Angle
Human x = 2, y = 2
Xeno x = 5, y = 5 
Projectile x = 4, y = 4
dy = get_pixel_position_y(xeno) – get_pixel_position_y(projectile)
	get_pixel_position_y(xeno)
		. = (pixel_y = 0) + (base_pixel_y = 0) = 0
		. += 32 * 5 = 160
		. = 160
		. += 0 + 0 = 0
		. = 160
	get_pixel_position_y(projectile)
		. = 0 + 0 = 0
		. += 32 * 4 = 128
		. = 128
		. += 0 + 0 = 0
		. = 128
dy = 160 – 128 = 32

dx = get_pixel_position_x(xeno) – get_pixel_position_x(projectile)
	get_pixel_position_x(xeno)
		. = 0 + 0 = 0
		. += 32 * 5 = 160
		. = 160
		. += 0 + 0 = 0
		. = 160
	get_pixel_position_x(projectile)
		. = 0 + 0 = 0
		. += 32 * 4 = 128
		. = 128
		. += 0 + 0 = 0
		. = 128
dx = 160 – 128 = 32

delta_to_angle()
arctan(32,32) = 45
RETURNS NORTHEAST

It now returns NORTHEAST, as is expected.

You are absolutely correct that arctan(x, y) produces different results from arctan(x / y), giving east-counterclockwise angles instead of the north-clockwise ones we are more familiar with and which angle_to_dir() expects. Instead, this uses arctan(y, x) with the x and y transposed:

This transposition causes arctan() to return north-clockwise angles again. The Wikipedia article that I pulled it from explains it far better than I can:
https://en.wikipedia.org/wiki/Atan2#East-counterclockwise,_north-clockwise_and_south-clockwise_conventions,_etc.

[Drulikar]

Tuner finding and slug usage appears to be working as intended; not been any other reports as far as I've seen from test merging either.