Canonical Movement (#82085) (#3974)

## About The Pull Request

- DaedalusDock/daedalusdock#892
Currently, TG's movement chain is not canonical, meaning movement can
resolve in an order that doesn't actually represent what it going on.
For example, if something inside of an Entered() call would move the
currently moving object, it resolves in this order:

1. Original Move
2. Intercepted Move
3. Intercepted Moved
4. Original Moved

This makes Moved() unreliable at tracking things like spatial grid
locations. This is a massive problem.

This PR introduces `active_movement`, a list containing arguments for
`Moved()`. At the start of `Move()` and `doMove()`, if the list is
present, it will call Moved(), concluding the original movement, before
proceeding. The original `Move()` call will not end in `Moved()`, due to
`active_movement` being null.

This is touching some of the most important code in the game and needs
extensive testing.

Co-authored-by: Kapu1178 <[email protected]>

code/__DEFINES/movement_info.dm

@@ -0,0 +1,21 @@
+#define ACTIVE_MOVEMENT_OLDLOC 1
+#define ACTIVE_MOVEMENT_DIRECTION 2
+#define ACTIVE_MOVEMENT_FORCED 3
+#define ACTIVE_MOVEMENT_OLDLOCS 4
+
+/// The arguments of this macro correspond directly to the argument order of /atom/movable/proc/Moved
+#define SET_ACTIVE_MOVEMENT(_old_loc, _direction, _forced, _oldlocs) \
+	active_movement = list( \
+		_old_loc, \
+		_direction, \
+		_forced, \
+		_oldlocs, \
+	)
+
+/// Finish any active movements
+#define RESOLVE_ACTIVE_MOVEMENT \
+	if(active_movement) { \
+		var/__move_args = active_movement; \
+		active_movement = null; \
+		Moved(arglist(__move_args)); \
+	}

code/game/atoms_movable.dm

@@ -4,6 +4,8 @@
 	appearance_flags = TILE_BOUND|PIXEL_SCALE|LONG_GLIDE
 
 	var/last_move = null
+	/// A list containing arguments for Moved().
+	VAR_PRIVATE/tmp/list/active_movement
 	var/anchored = FALSE
 	var/move_resist = MOVE_RESIST_DEFAULT
 	var/move_force = MOVE_FORCE_DEFAULT
@@ -594,6 +596,7 @@
  * most of the time you want forceMove()
  */
 /atom/movable/proc/abstract_move(atom/new_loc)
+	RESOLVE_ACTIVE_MOVEMENT // This should NEVER happen, but, just in case...
 	var/atom/old_loc = loc
 	var/direction = get_dir(old_loc, new_loc)
 	loc = new_loc
@@ -608,6 +611,9 @@
 	if(!newloc || newloc == loc)
 		return
 
+	// A mid-movement... movement... occured, resolve that first.
+	RESOLVE_ACTIVE_MOVEMENT
+
 	if(!direction)
 		direction = get_dir(src, newloc)
 
@@ -657,6 +663,7 @@
 	var/area/oldarea = get_area(oldloc)
 	var/area/newarea = get_area(newloc)
 
+	SET_ACTIVE_MOVEMENT(oldloc, direction, FALSE, old_locs)
 	loc = newloc
 
 	. = TRUE
@@ -677,7 +684,7 @@
 	if(oldarea != newarea)
 		newarea.Entered(src, oldarea)
 
-	Moved(oldloc, direction, FALSE, old_locs)
+	RESOLVE_ACTIVE_MOVEMENT
 
 ////////////////////////////////////////
 
@@ -1096,8 +1103,13 @@
 
 /atom/movable/proc/doMove(atom/destination)
 	. = FALSE
+	RESOLVE_ACTIVE_MOVEMENT
+
 	var/atom/oldloc = loc
 	var/is_multi_tile = bound_width > world.icon_size || bound_height > world.icon_size
+
+	SET_ACTIVE_MOVEMENT(oldloc, NONE, TRUE, null)
+
 	if(destination)
 		///zMove already handles whether a pull from another movable should be broken.
 		if(pulledby && !currently_z_moving)
@@ -1159,7 +1171,7 @@
 			if(old_area)
 				old_area.Exited(src, NONE)
 
-	Moved(oldloc, NONE, TRUE)
+	RESOLVE_ACTIVE_MOVEMENT
 
 /**
  * Called when a movable changes z-levels.

code/modules/unit_tests/_unit_tests.dm

@@ -163,6 +163,7 @@
 #include "modsuit.dm"
 #include "modular_map_loader.dm"
 #include "mouse_bite_cable.dm"
+#include "movement_order_sanity.dm"
 #include "mutant_hands_consistency.dm"
 #include "mutant_organs.dm"
 #include "novaflower_burn.dm"

code/modules/unit_tests/movement_order_sanity.dm

@@ -0,0 +1,49 @@
+/datum/unit_test/movement_order_sanity/Run()
+	var/obj/movement_tester/test_obj = allocate(__IMPLIED_TYPE__, run_loc_floor_bottom_left)
+	var/list/movement_cache = test_obj.movement_order
+
+	var/obj/movement_interceptor/interceptor = allocate(__IMPLIED_TYPE__)
+	interceptor.forceMove(locate(run_loc_floor_bottom_left.x + 1, run_loc_floor_bottom_left.y, run_loc_floor_bottom_left.z))
+
+	var/did_move = step(test_obj, EAST)
+
+	TEST_ASSERT(did_move, "Object did not move at all.")
+	TEST_ASSERT(QDELETED(test_obj), "Object was not qdeleted.")
+	TEST_ASSERT(length(movement_cache) == 4, "Movement order length was not the expected value of 4, got: [length(movement_cache)].\nMovement Log\n[jointext(movement_cache, "\n")]")
+
+	// Due to when the logging takes place, it will always be Move Move > Moved Moved instead of the reality of
+	// Move > Moved > Move > Moved
+	TEST_ASSERT(findtext(movement_cache[1], "Moving from"),"Movement step 1 was not a Move attempt.\nMovement Log\n[jointext(movement_cache, "\n")]")
+	TEST_ASSERT(findtext(movement_cache[2], "Moving from"),"Movement step 2 was not a Move attempt.\nMovement Log\n[jointext(movement_cache, "\n")]")
+	TEST_ASSERT(findtext(movement_cache[3], "Moved from"),"Movement step 3 was not a Moved() call.\nMovement Log\n[jointext(movement_cache, "\n")]")
+	TEST_ASSERT(findtext(movement_cache[4], "Moved from"),"Movement step 4 was not a Moved() call.\nMovement Log\n[jointext(movement_cache, "\n")]")
+
+/obj/movement_tester
+	name = "movement debugger"
+	var/list/movement_order = list()
+
+/obj/movement_tester/Move(atom/newloc, direct, glide_size_override, z_movement_flags)
+	movement_order += "Moving from ([loc.x], [loc.y]) to [newloc ? "([newloc.x], [newloc.y])" : "NULL"]"
+	return ..()
+
+/obj/movement_tester/doMove(atom/destination)
+	movement_order += "Abstractly Moving from ([loc.x], [loc.y]) to [destination ? "([destination.x], [destination.y])" : "NULL"]"
+	return ..()
+
+/obj/movement_tester/Moved(atom/old_loc, movement_dir, forced, list/old_locs, momentum_change)
+	movement_order += "Moved from ([old_loc.x], [old_loc.y]) to [loc ? "([loc.x], [loc.y])" : "NULL"]"
+	return ..()
+
+/obj/movement_interceptor
+	name = "movement interceptor"
+
+/obj/movement_interceptor/Initialize(mapload)
+	. = ..()
+	AddElement(/datum/element/connect_loc, list(COMSIG_ATOM_ENTERED = PROC_REF(on_crossed)))
+
+/obj/movement_interceptor/proc/on_crossed(datum/source, atom/movable/arrived)
+	SIGNAL_HANDLER
+	if(src == arrived)
+		return
+
+	qdel(arrived)

tgstation.dme

@@ -169,6 +169,7 @@
 #include "code\__DEFINES\mood.dm"
 #include "code\__DEFINES\move_force.dm"
 #include "code\__DEFINES\movement.dm"
+#include "code\__DEFINES\movement_info.dm"
 #include "code\__DEFINES\movespeed_modification.dm"
 #include "code\__DEFINES\multiz.dm"
 #include "code\__DEFINES\nanites.dm"