From d7d91c0394609587c6b53db4c93938069cc6d991 Mon Sep 17 00:00:00 2001 From: Kapu1178 <75460809+Kapu1178@users.noreply.github.com> Date: Fri, 22 Mar 2024 20:31:15 -0400 Subject: [PATCH] Canonical Movement (#82085) ## About The Pull Request - https://github.com/DaedalusDock/daedalusdock/pull/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. --- code/__DEFINES/movement_info.dm | 21 ++++++++ code/game/atoms_movable.dm | 16 +++++- code/modules/unit_tests/_unit_tests.dm | 1 + .../unit_tests/movement_order_sanity.dm | 49 +++++++++++++++++++ tgstation.dme | 1 + 5 files changed, 86 insertions(+), 2 deletions(-) create mode 100644 code/__DEFINES/movement_info.dm create mode 100644 code/modules/unit_tests/movement_order_sanity.dm diff --git a/code/__DEFINES/movement_info.dm b/code/__DEFINES/movement_info.dm new file mode 100644 index 000000000000..91d370f6c75f --- /dev/null +++ b/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)); \ + } diff --git a/code/game/atoms_movable.dm b/code/game/atoms_movable.dm index 77140b4be624..de83c8ae9741 100644 --- a/code/game/atoms_movable.dm +++ b/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. diff --git a/code/modules/unit_tests/_unit_tests.dm b/code/modules/unit_tests/_unit_tests.dm index 0d373b931c25..c662ec1f4b74 100644 --- a/code/modules/unit_tests/_unit_tests.dm +++ b/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" diff --git a/code/modules/unit_tests/movement_order_sanity.dm b/code/modules/unit_tests/movement_order_sanity.dm new file mode 100644 index 000000000000..a9f677b24980 --- /dev/null +++ b/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) diff --git a/tgstation.dme b/tgstation.dme index d074c0ae2dff..997e1fdc0a74 100644 --- a/tgstation.dme +++ b/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"