From c36a40645afe4327facc376daeb441065bdca058 Mon Sep 17 00:00:00 2001
From: Graham Pentheny <grahamboree@users.noreply.github.com>
Date: Mon, 1 Jan 2024 14:56:47 -0500
Subject: [PATCH 24/36] Cleanup filter code and improved documentation (#683)

This mostly just changes variable names and adds some comments to make the code more clear.

It also has a few small fixup changes to the unit tests.
---
 Recast/Include/Recast.h             |   3 +-
 Recast/Source/RecastFilter.cpp      | 126 +++++++++++++++-------------
 Tests/Recast/Tests_RecastFilter.cpp |  56 +------------
 3 files changed, 75 insertions(+), 110 deletions(-)

diff --git a/Recast/Include/Recast.h b/Recast/Include/Recast.h
index 2104aa5..5b34ea5 100644
--- a/Recast/Include/Recast.h
+++ b/Recast/Include/Recast.h
@@ -1002,7 +1002,8 @@ bool rcRasterizeTriangles(rcContext* context,
 
 /// Marks non-walkable spans as walkable if their maximum is within @p walkableClimb of the span below them.
 ///
-/// This removes small obstacles that the agent would be able to walk over such as curbs, and also allows agents to move up structures such as stairs.
+/// This removes small obstacles and rasterization artifacts that the agent would be able to walk over
+/// such as curbs.  It also allows agents to move up terraced structures like stairs.
 /// 
 /// Obstacle spans are marked walkable if: <tt>obstacleSpan.smax - walkableSpan.smax < walkableClimb</tt>
 /// 
diff --git a/Recast/Source/RecastFilter.cpp b/Recast/Source/RecastFilter.cpp
index 1ecf858..7875040 100644
--- a/Recast/Source/RecastFilter.cpp
+++ b/Recast/Source/RecastFilter.cpp
@@ -41,24 +41,24 @@ void rcFilterLowHangingWalkableObstacles(rcContext* context, const int walkableC
 		{
 			rcSpan* previousSpan = NULL;
 			bool previousWasWalkable = false;
-			unsigned char previousArea = RC_NULL_AREA;
+			unsigned char previousAreaID = RC_NULL_AREA;
 
+			// For each span in the column...
 			for (rcSpan* span = heightfield.spans[x + z * xSize]; span != NULL; previousSpan = span, span = span->next)
 			{
 				const bool walkable = span->area != RC_NULL_AREA;
-				// If current span is not walkable, but there is walkable
-				// span just below it, mark the span above it walkable too.
-				if (!walkable && previousWasWalkable)
+
+				// If current span is not walkable, but there is walkable span just below it and the height difference
+				// is small enough for the agent to walk over, mark the current span as walkable too.
+				if (!walkable && previousWasWalkable && (int)span->smax - (int)previousSpan->smax <= walkableClimb)
 				{
-					if (rcAbs((int)span->smax - (int)previousSpan->smax) <= walkableClimb)
-					{
-						span->area = previousArea;
-					}
+					span->area = previousAreaID;
 				}
-				// Copy walkable flag so that it cannot propagate
-				// past multiple non-walkable objects.
+
+				// Copy the original walkable value regardless of whether we changed it.
+				// This prevents multiple consecutive non-walkable spans from being erroneously marked as walkable.
 				previousWasWalkable = walkable;
-				previousArea = span->area;
+				previousAreaID = span->area;
 			}
 		}
 	}
@@ -73,84 +73,98 @@ void rcFilterLedgeSpans(rcContext* context, const int walkableHeight, const int
 	const int xSize = heightfield.width;
 	const int zSize = heightfield.height;
 	
-	// Mark border spans.
+	// Mark spans that are adjacent to a ledge as unwalkable..
 	for (int z = 0; z < zSize; ++z)
 	{
 		for (int x = 0; x < xSize; ++x)
 		{
 			for (rcSpan* span = heightfield.spans[x + z * xSize]; span; span = span->next)
 			{
-				// Skip non walkable spans.
+				// Skip non-walkable spans.
 				if (span->area == RC_NULL_AREA)
 				{
 					continue;
 				}
 
-				const int bot = (int)(span->smax);
-				const int top = span->next ? (int)(span->next->smin) : MAX_HEIGHTFIELD_HEIGHT;
+				const int floor = (int)(span->smax);
+				const int ceiling = span->next ? (int)(span->next->smin) : MAX_HEIGHTFIELD_HEIGHT;
 
-				// Find neighbours minimum height.
-				int minNeighborHeight = MAX_HEIGHTFIELD_HEIGHT;
+				// The difference between this walkable area and the lowest neighbor walkable area.
+				// This is the difference between the current span and all neighbor spans that have
+				// enough space for an agent to move between, but not accounting at all for surface slope.
+				int lowestNeighborFloorDifference = MAX_HEIGHTFIELD_HEIGHT;
 
 				// Min and max height of accessible neighbours.
-				int accessibleNeighborMinHeight = span->smax;
-				int accessibleNeighborMaxHeight = span->smax;
+				int lowestTraversableNeighborFloor = span->smax;
+				int highestTraversableNeighborFloor = span->smax;
 
 				for (int direction = 0; direction < 4; ++direction)
 				{
-					int dx = x + rcGetDirOffsetX(direction);
-					int dz = z + rcGetDirOffsetY(direction);
+					const int neighborX = x + rcGetDirOffsetX(direction);
+					const int neighborZ = z + rcGetDirOffsetY(direction);
+
 					// Skip neighbours which are out of bounds.
-					if (dx < 0 || dz < 0 || dx >= xSize || dz >= zSize)
+					if (neighborX < 0 || neighborZ < 0 || neighborX >= xSize || neighborZ >= zSize)
 					{
-						minNeighborHeight = (-walkableClimb - 1) ;
+						lowestNeighborFloorDifference = -walkableClimb - 1;
 						break;
 					}
 
-					// From minus infinity to the first span.
-					const rcSpan* neighborSpan = heightfield.spans[dx + dz * xSize];
-					int neighborTop = neighborSpan ? (int)neighborSpan->smin : MAX_HEIGHTFIELD_HEIGHT;
-					
+					const rcSpan* neighborSpan = heightfield.spans[neighborX + neighborZ * xSize];
+
+					// The most we can step down to the neighbor is the walkableClimb distance.
+					// Start with the area under the neighbor span
+					int neighborCeiling = neighborSpan ? (int)neighborSpan->smin : MAX_HEIGHTFIELD_HEIGHT;
+
 					// Skip neighbour if the gap between the spans is too small.
-					if (rcMin(top, neighborTop) - bot >= walkableHeight)
+					if (rcMin(ceiling, neighborCeiling) - floor >= walkableHeight)
 					{
-						minNeighborHeight = (-walkableClimb - 1);
+						lowestNeighborFloorDifference = (-walkableClimb - 1);
 						break;
 					}
 
-					// Rest of the spans.
-					for (neighborSpan = heightfield.spans[dx + dz * xSize]; neighborSpan; neighborSpan = neighborSpan->next)
+					// For each span in the neighboring column...
+					for (; neighborSpan != NULL; neighborSpan = neighborSpan->next)
 					{
-						int neighborBot = (int)neighborSpan->smax;
-						neighborTop = neighborSpan->next ? (int)neighborSpan->next->smin : MAX_HEIGHTFIELD_HEIGHT;
-						
-						// Skip neighbour if the gap between the spans is too small.
-						if (rcMin(top, neighborTop) - rcMax(bot, neighborBot) >= walkableHeight)
+						const int neighborFloor = (int)neighborSpan->smax;
+						neighborCeiling = neighborSpan->next ? (int)neighborSpan->next->smin : MAX_HEIGHTFIELD_HEIGHT;
+
+						// Only consider neighboring areas that have enough overlap to be potentially traversable.
+						if (rcMin(ceiling, neighborCeiling) - rcMax(floor, neighborFloor) < walkableHeight)
+						{
+							// No space to traverse between them.
+							continue;
+						}
+
+						const int neighborFloorDifference = neighborFloor - floor;
+						lowestNeighborFloorDifference = rcMin(lowestNeighborFloorDifference, neighborFloorDifference);
+
+						// Find min/max accessible neighbor height.
+						// Only consider neighbors that are at most walkableClimb away.
+						if (rcAbs(neighborFloorDifference) <= walkableClimb)
+						{
+							// There is space to move to the neighbor cell and the slope isn't too much.
+							lowestTraversableNeighborFloor = rcMin(lowestTraversableNeighborFloor, neighborFloor);
+							highestTraversableNeighborFloor = rcMax(highestTraversableNeighborFloor, neighborFloor);
+						}
+						else if (neighborFloorDifference < -walkableClimb)
 						{
-							int accessibleNeighbourHeight = neighborBot - bot;
-							minNeighborHeight = rcMin(minNeighborHeight, accessibleNeighbourHeight);
-
-							// Find min/max accessible neighbour height. 
-							if (rcAbs(accessibleNeighbourHeight) <= walkableClimb)
-							{
-								if (neighborBot < accessibleNeighborMinHeight) accessibleNeighborMinHeight = neighborBot;
-								if (neighborBot > accessibleNeighborMaxHeight) accessibleNeighborMaxHeight = neighborBot;
-							}
-							else if (accessibleNeighbourHeight < -walkableClimb)
-							{
-								break;
-							}
+							// We already know this will be considered a ledge span so we can early-out
+							break;
 						}
 					}
 				}
 
-				// The current span is close to a ledge if the drop to any neighbour span is less than the walkableClimb.
-				if (minNeighborHeight < -walkableClimb)
+				// The current span is close to a ledge if the magnitude of the drop to any neighbour span is greater than the walkableClimb distance.
+				// That is, there is a gap that is large enough to let an agent move between them, but the drop (surface slope) is too large to allow it.
+				// (If this is the case, then biggestNeighborStepDown will be negative, so compare against the negative walkableClimb as a means of checking
+				// the magnitude of the delta)
+				if (lowestNeighborFloorDifference < -walkableClimb)
 				{
 					span->area = RC_NULL_AREA;
 				}
-				// If the difference between all neighbours is too large, we are at steep slope, mark the span as ledge.
-				else if ((accessibleNeighborMaxHeight - accessibleNeighborMinHeight) > walkableClimb)
+				// If the difference between all neighbor floors is too large, this is a steep slope, so mark the span as an unwalkable ledge.
+				else if (highestTraversableNeighborFloor - lowestTraversableNeighborFloor > walkableClimb)
 				{
 					span->area = RC_NULL_AREA;
 				}
@@ -175,9 +189,9 @@ void rcFilterWalkableLowHeightSpans(rcContext* context, const int walkableHeight
 		{
 			for (rcSpan* span = heightfield.spans[x + z*xSize]; span; span = span->next)
 			{
-				const int bot = (int)(span->smax);
-				const int top = span->next ? (int)(span->next->smin) : MAX_HEIGHTFIELD_HEIGHT;
-				if ((top - bot) < walkableHeight)
+				const int floor = (int)(span->smax);
+				const int ceiling = span->next ? (int)(span->next->smin) : MAX_HEIGHTFIELD_HEIGHT;
+				if (ceiling - floor < walkableHeight)
 				{
 					span->area = RC_NULL_AREA;
 				}
diff --git a/Tests/Recast/Tests_RecastFilter.cpp b/Tests/Recast/Tests_RecastFilter.cpp
index 1882adb..d600835 100644
--- a/Tests/Recast/Tests_RecastFilter.cpp
+++ b/Tests/Recast/Tests_RecastFilter.cpp
@@ -47,7 +47,7 @@ TEST_CASE("rcFilterLowHangingWalkableObstacles", "[recast, filtering]")
 	{
 		// Put the second span just above the first one.
 		rcSpan* secondSpan = (rcSpan*)rcAlloc(sizeof(rcSpan), RC_ALLOC_PERM);
-		secondSpan->area = 1;
+		secondSpan->area = RC_NULL_AREA;
 		secondSpan->next = NULL;
 		secondSpan->smin = 1 + walkableHeight;
 		secondSpan->smax = secondSpan->smin + 1;
@@ -64,7 +64,7 @@ TEST_CASE("rcFilterLowHangingWalkableObstacles", "[recast, filtering]")
 
 		// Check that nothing has changed.
 		REQUIRE(heightfield.spans[0]->area == 1);
-		REQUIRE(heightfield.spans[0]->next->area == 1);
+		REQUIRE(heightfield.spans[0]->next->area == RC_NULL_AREA);
 
 		// Check again but with a more clearance
 		secondSpan->smin += 10;
@@ -74,7 +74,7 @@ TEST_CASE("rcFilterLowHangingWalkableObstacles", "[recast, filtering]")
 
 		// Check that nothing has changed.
 		REQUIRE(heightfield.spans[0]->area == 1);
-		REQUIRE(heightfield.spans[0]->next->area == 1);
+		REQUIRE(heightfield.spans[0]->next->area == RC_NULL_AREA);
 
 		rcFree(span);
 		rcFree(secondSpan);
@@ -251,56 +251,6 @@ TEST_CASE("rcFilterLedgeSpans", "[recast, filtering]")
 			}
 		}
 	}
-
-	SECTION("Edge spans are marked unwalkable")
-	{
-		// Create a flat plane.
-		for (int x = 0; x < heightfield.width; ++x)
-		{
-			for (int z = 0; z < heightfield.height; ++z)
-			{
-				rcSpan* span = (rcSpan*)rcAlloc(sizeof(rcSpan), RC_ALLOC_PERM);
-				span->area = 1;
-				span->next = NULL;
-				span->smin = 0;
-				span->smax = 1;
-				heightfield.spans[x + z * heightfield.width] = span;
-			}
-		}
-
-		rcFilterLedgeSpans(&context, walkableHeight, walkableClimb, heightfield);
-
-		for (int x = 0; x < heightfield.width; ++x)
-		{
-			for (int z = 0; z < heightfield.height; ++z)
-			{
-				rcSpan* span = heightfield.spans[x + z * heightfield.width];
-				REQUIRE(span != NULL);
-
-				if (x == 0 || z == 0 || x == 9 || z == 9)
-				{
-					REQUIRE(span->area == RC_NULL_AREA);
-				}
-				else
-				{
-					REQUIRE(span->area == 1);
-				}
-
-				REQUIRE(span->next == NULL);
-				REQUIRE(span->smin == 0);
-				REQUIRE(span->smax == 1);
-			}
-		}
-
-		// Free all the heightfield spans
-		for (int x = 0; x < heightfield.width; ++x)
-		{
-			for (int z = 0; z < heightfield.height; ++z)
-			{
-				rcFree(heightfield.spans[x + z * heightfield.width]);
-			}
-		}
-	}
 }
 
 TEST_CASE("rcFilterWalkableLowHeightSpans", "[recast, filtering]")
-- 
2.43.0