Fixes misc Move() bugs, adds integration test for Move chain

Content.IntegrationTests/DMProject/Tests/move.dm

@@ -0,0 +1,253 @@
+// Ensures movable.Move() has the correct order and iteration
+
+var/global/testing_move = FALSE
+var/global/list/move_trace = list()
+#define LOG_LOC(some_loc) (isturf(some_loc) ? "[(some_loc).type] ([(some_loc).x],[(some_loc).y])" : "[(some_loc).type] [(some_loc).name]")
+#define BEGIN_TRACE(separator) move_trace.Cut()
+#define MARK(ref, mark) "[LOG_LOC(ref)] [mark]"
+#define TRACE_STEP(ref, mark) if(testing_move){move_trace += MARK(ref, mark)}
+
+#define ENTER "enter"
+#define ENTERED "entered"
+#define EXIT "exit"
+#define EXITED "exited"
+#define CROSS "cross"
+#define CROSSED "crossed"
+#define UNCROSS "uncross"
+#define UNCROSSED "uncrossed"
+#define BUMP "bump"
+#define MOVE "move"
+
+/atom/Enter(O, oldloc)
+	TRACE_STEP(src, ENTER)
+	. = ..()
+
+/atom/Entered(Obj, OldLoc)
+	TRACE_STEP(src, ENTERED)
+	. = ..()
+
+/atom/Exit(O, newloc)
+	TRACE_STEP(src, EXIT)
+	. = ..()
+
+/atom/Exited(Obj, newloc)
+	TRACE_STEP(src, EXITED)
+	. = ..()
+
+/atom/Cross(O)
+	TRACE_STEP(src, CROSS)
+	. = ..()
+
+/atom/Crossed(O)
+	TRACE_STEP(src, CROSSED)
+	. = ..()
+
+/atom/Uncross(O)
+	TRACE_STEP(src, UNCROSS)
+	. = ..()
+
+/atom/Uncrossed(O)
+	TRACE_STEP(src, UNCROSSED)	
+	. = ..()
+
+/atom/movable/Bump(Obstacle)
+	TRACE_STEP(src, BUMP)
+	. = ..()
+
+/atom/movable/Move(NewLoc, Dir, step_x, step_y)
+	TRACE_STEP(src, MOVE)
+	. = ..()
+
+#define RUN_TEST(expected, expression)\
+	BEGIN_TRACE("running [nameof(expected)]"); \
+	##expression; \
+	_run_test(expected, nameof(expected));
+/datum/unit_test/move_parity/proc/_run_test(list/expected, identifier)
+	var/error_index = 1
+	try
+		if(expected.len != move_trace.len)
+			error_index = expected.len
+			CRASH("result was [expected.len > move_trace.len ? "shorter" : "longer"] than expected")
+
+		for(var/i in 1 to expected.len)
+			if(expected[i] != move_trace[i])
+				error_index = i
+				CRASH("expected did not match result")
+	catch(var/exception/exc)
+		var/list/error_message = list("[identifier]: [exc]")
+
+		error_message += "expected:"
+		for(var/i in 1 to expected.len)
+			error_message += "\t[expected[i]][i == error_index ? "<--- here" : null]"
+
+		error_message += "got:"
+		for(var/i in 1 to move_trace.len)
+			error_message += "\t[move_trace[i]][i == error_index ? "<--- here" : null]"
+
+		CRASH(error_message.Join("\n"))
+
+// this needs to be a proc cause BYOND gets confused 
+/datum/unit_test/move_parity/proc/LOC(x, y) as /turf
+	return locate(x, y, 3)
+
+/datum/unit_test/move_parity/RunTest()
+	testing_move = TRUE
+	world.maxx = world.maxy = 2
+	var/area/local_area = LOC(1, 1).loc
+
+	// There is a plot here if you can read it
+
+	var/mob/protag = new(LOC(1, 1))
+	protag.name = "protag"
+	var/list/protag_init = list()
+	RUN_TEST(protag_init, step(protag, 0))
+
+	var/list/protag_step = list(
+		MARK(protag, MOVE),
+		MARK(LOC(1, 1), EXIT),
+		MARK(LOC(1, 1), UNCROSS),
+		MARK(LOC(2, 1), ENTER),
+		MARK(LOC(2, 1), CROSS),
+		MARK(LOC(1, 1), EXITED),
+		MARK(LOC(1, 1), UNCROSSED),
+		MARK(LOC(2, 1), ENTERED),
+		MARK(LOC(2, 1), CROSSED),
+	)
+	RUN_TEST(protag_step, step(protag, EAST))
+
+	var/list/protag_wall = list()
+	RUN_TEST(protag_wall, step(protag, EAST))
+
+	var/mob/antag = new()
+	antag.name = "antag"
+	var/list/antag_summon = list(
+		MARK(antag, MOVE),
+		MARK(LOC(1, 1), ENTER),
+		MARK(LOC(1, 1), CROSS),
+		MARK(local_area, ENTER),
+		MARK(local_area, CROSS),
+		MARK(LOC(1, 1), ENTERED),
+		MARK(LOC(1, 1), CROSSED),
+		MARK(local_area, ENTERED),
+		MARK(local_area, CROSSED),
+	)
+	RUN_TEST(antag_summon, antag.Move(LOC(1, 1)))
+
+	var/list/protag_bump = list(
+		MARK(protag, MOVE),
+		MARK(LOC(2, 1), EXIT),
+		MARK(LOC(2, 1), UNCROSS),
+		MARK(LOC(1, 1), ENTER),
+		MARK(LOC(1, 1), CROSS),
+		MARK(antag, CROSS),
+		MARK(protag, BUMP),
+	)
+	RUN_TEST(protag_bump, step(protag, WEST))
+
+	var/mob/deuterag = new(LOC(2, 2))
+	deuterag.name = "deuterag"
+	deuterag.density = FALSE
+	var/list/protag_cross = list(
+		MARK(protag, MOVE),
+		MARK(LOC(2, 1), EXIT),
+		MARK(LOC(2, 1), UNCROSS),
+		MARK(LOC(2, 2), ENTER),
+		MARK(LOC(2, 2), CROSS),
+		MARK(deuterag, CROSS),
+		MARK(LOC(2, 1), EXITED),
+		MARK(LOC(2, 1), UNCROSSED),
+		MARK(LOC(2, 2), ENTERED),
+		MARK(LOC(2, 2), CROSSED),
+		MARK(deuterag, CROSSED),
+	)
+	RUN_TEST(protag_cross, step(protag, NORTH))
+
+	var/list/protag_uncross = list(
+		MARK(protag, MOVE),
+		MARK(LOC(2, 2), EXIT),
+		MARK(LOC(2, 2), UNCROSS),
+		MARK(deuterag, UNCROSS),
+		MARK(LOC(2, 1), ENTER),
+		MARK(LOC(2, 1), CROSS),
+		MARK(LOC(2, 2), EXITED),
+		MARK(LOC(2, 2), UNCROSSED),
+		MARK(deuterag, UNCROSSED),
+		MARK(LOC(2, 1), ENTERED),
+		MARK(LOC(2, 1), CROSSED),
+	)
+	RUN_TEST(protag_uncross, step(protag, SOUTH))
+
+	var/mob/minion = new(antag)
+	minion.name = "minion"
+	minion.group += antag
+	var/list/minion_bump = list(
+		MARK(minion, MOVE),
+		MARK(antag, EXIT),
+		MARK(LOC(2, 1), ENTER),
+		MARK(LOC(2, 1), CROSS),
+		MARK(protag, CROSS),
+		MARK(local_area, ENTER),
+		MARK(local_area, CROSS),
+		MARK(minion, BUMP)
+	)
+	RUN_TEST(minion_bump, step(minion, EAST))
+
+	var/list/minion_deploy = list(
+		MARK(minion, MOVE),
+		MARK(antag, EXIT),
+		MARK(LOC(1, 2), ENTER),
+		MARK(LOC(1, 2), CROSS),
+		MARK(local_area, ENTER),
+		MARK(local_area, CROSS),
+		MARK(antag, EXITED),
+		MARK(LOC(1, 2), ENTERED),
+		MARK(LOC(1, 2), CROSSED),
+		MARK(local_area, ENTERED),
+		MARK(local_area, CROSSED),
+	)
+	RUN_TEST(minion_deploy, step(minion, NORTH))
+
+	// TODO: implement mob.group shuffling and test here
+
+	var/list/minion_retreat = list(
+		MARK(minion, MOVE),
+		MARK(LOC(1, 2), EXIT),
+		MARK(LOC(1, 2), UNCROSS),
+		MARK(local_area, EXIT),
+		MARK(antag, ENTER),
+		MARK(LOC(1, 2), EXITED),
+		MARK(LOC(1, 2), UNCROSSED),
+		MARK(local_area, EXITED),
+		MARK(local_area, UNCROSSED),
+		MARK(antag, ENTERED),
+	)
+	RUN_TEST(minion_retreat, minion.Move(antag, SOUTH))
+
+	var/list/antag_banish = list(
+		MARK(antag, MOVE),
+	)
+	RUN_TEST(antag_banish, antag.Move(null))
+
+	del(minion)
+	del(deuterag)
+	del(antag)
+	del(protag)
+
+/datum/unit_test/move_parity/Del()
+	testing_move = FALSE	
+
+#undef RUN_TEST
+#undef LOG_LOC
+#undef BEGIN_TRACE
+#undef MARK
+#undef TRACE_STEP
+#undef ENTER
+#undef ENTERED
+#undef EXIT
+#undef EXITED
+#undef CROSS
+#undef CROSSED
+#undef UNCROSS
+#undef UNCROSSED
+#undef BUMP
+#undef MOVE

DMCompiler/DMStandard/Types/Atoms/Area.dm

@@ -2,4 +2,22 @@
 	parent_type = /atom
 
 	layer = 1.0
-	luminosity = 1
+	luminosity = 1
+
+	Enter(atom/movable/O, atom/oldloc)
+		if(!..())
+			return FALSE
+		return src.Cross(O)
+
+	Entered(atom/movable/Obj, atom/OldLoc)
+		..()
+		Crossed(Obj)
+
+	Exit(atom/movable/O, atom/newloc)
+		if (!..())
+			return FALSE
+		return TRUE // areas don't call Uncross for some reason?
+
+	Exited(atom/movable/Obj, atom/newloc)
+		..()
+		Uncrossed(Obj)

DMCompiler/DMStandard/Types/Atoms/Movable.dm

@@ -23,39 +23,55 @@
 
 		if (Dir != 0)
 			dir = Dir
+
+		var/area/oldarea = isarea(loc?.loc) ? loc.loc : null
+		var/area/newarea = isarea(NewLoc.loc) ? NewLoc.loc : null
+		var/consider_area = (oldarea != newarea)
 
-		if(!isnull(loc))
+		if (!isnull(loc))
+			// Loc first
 			if (!loc.Exit(src, NewLoc))
 				return FALSE
+			// Area second
+			if (consider_area && !isnull(oldarea) && !oldarea.Exit(src, NewLoc))
+				return FALSE
 			// Ensure the atoms on the turf also permit this exit
 			for (var/atom/movable/exiting in loc)
-				if (!exiting.Uncross(src))
-					return FALSE
+				if (exiting != src)
+					if (!exiting.Uncross(src))
+						return FALSE
 
-		if (NewLoc.Enter(src, loc))
+		var/loc_entered = NewLoc.Enter(src, loc)
+		var/area_entered = !consider_area || isnull(newarea) || newarea.Enter(src, loc)
+		if (!loc_entered || !area_entered)
+			if (!area_entered)
+				src.Bump(newarea)
+			if (!loc_entered)
+				for (var/atom/content in NewLoc.contents)
+					src.Bump(content)
+
+			return FALSE
+		else
 			var/atom/oldloc = loc
-			var/area/oldarea = oldloc?.loc
-			var/area/newarea = NewLoc.loc
 			loc = NewLoc
 
-			// First, call Exited() on the old area
-			if (newarea != oldarea)
-				oldarea?.Exited(src, loc)
-
-			// Second, call Exited() on the old turf and Uncrossed() on its contents
+			// First, call Exited() on the old turf and Uncrossed() on its contents
 			oldloc?.Exited(src, loc)
 			for (var/atom/movable/uncrossed in oldloc)
 				uncrossed.Uncrossed(src)
+
+			// Second, call Exited() on the old area
+			if (consider_area)
+				oldarea?.Exited(src, loc)
 
 			// Third, call Entered() on the new turf and Crossed() on its contents
 			loc.Entered(src, oldloc)
 			for (var/atom/movable/crossed in loc)
-				crossed.Crossed(src)
+				if(crossed != src)
+					crossed.Crossed(src)
 
 			// Fourth, call Entered() on the new area
-			if (newarea != oldarea)
-				newarea.Entered(src, oldloc)
+			if (consider_area)
+				newarea?.Entered(src, oldloc)
 
 			return TRUE
-		else
-			return FALSE

DMCompiler/DMStandard/Types/Atoms/Turf.dm

@@ -8,10 +8,9 @@
 			return FALSE
 		if (!src.Cross(O))
 			return FALSE
-
+		
 		for (var/atom/content in src.contents)
 			if (!content.Cross(O))
-				O.Bump(content)
 				return FALSE
 
 		return TRUE
@@ -23,9 +22,11 @@
 		// /atom/movable/Move() is responsible for calling Uncross() on contents
 
 	Entered(atom/movable/Obj, atom/OldLoc)
+		..()
 		Crossed(Obj)
 		// /atom/movable/Move() is responsible for calling Crossed() on contents
 
 	Exited(atom/movable/Obj, atom/newloc)
+		..()
 		Uncrossed(Obj)
 		// /atom/movable/Move() is responsible for calling Uncrossed() on contents

DMCompiler/DMStandard/_Standard.dm

@@ -167,7 +167,11 @@ proc/winset(player, control_id, params)
 
 /proc/step(atom/movable/Ref as /atom/movable, var/Dir, var/Speed=0) as num
 	//TODO: Speed = step_size if Speed is 0
-	return Ref.Move(get_step(Ref, Dir), Dir)
+	var/NewLoc = get_step(Ref, Dir)
+	if (!NewLoc || NewLoc == Ref.loc)
+		return 0
+
+	return Ref.Move(NewLoc, Dir)
 
 /proc/step_away(atom/movable/Ref as /atom/movable, /atom/Trg, Max=5, Speed=0) as num
     return Ref.Move(get_step_away(Ref, Trg, Max), turn(get_dir(Ref, Trg), 180))