Pass new test: ensure we don't create gaps larger than the ancient state interval.

src/BizHawk.Client.Common/movie/tasproj/IStateManager.cs

@@ -23,9 +23,9 @@ namespace BizHawk.Client.Common
 		void Capture(int frame, IStatable source, bool force = false);
 
 		/// <summary>
-		/// Commands the state manager to remove a reserved state for the given frame, if it is exists
+		/// Tell the state manager we no longer wish to reserve the state for the given frame.
 		/// </summary>
-		void EvictReserved(int frame);
+		void Unreserve(int frame);
 
 		bool HasState(int frame);
 

src/BizHawk.Client.Common/movie/tasproj/TasBranch.cs

@@ -88,7 +88,7 @@ namespace BizHawk.Client.Common
 			if (newBranch.UserText.Length is 0) newBranch.UserText = old.UserText;
 			this[index] = newBranch;
 			if (!_movie.IsReserved(old.Frame))
-				_movie.TasStateManager.EvictReserved(old.Frame);
+				_movie.TasStateManager.Unreserve(old.Frame);
 
 			_movie.FlagChanges();
 		}
@@ -120,7 +120,7 @@ namespace BizHawk.Client.Common
 			if (result)
 			{
 				if (!_movie.IsReserved(item!.Frame))
-					_movie.TasStateManager.EvictReserved(item.Frame);
+					_movie.TasStateManager.Unreserve(item.Frame);
 
 				_movie.FlagChanges();
 			}

src/BizHawk.Client.Common/movie/tasproj/TasMovieMarker.cs

@@ -206,7 +206,7 @@ namespace BizHawk.Client.Common
 				return;
 			}
 
-			_movie.TasStateManager.EvictReserved(item.Frame - 1);
+			_movie.TasStateManager.Unreserve(item.Frame - 1);
 			_movie.ChangeLog.AddMarkerChange(null, item.Frame, item.Message);
 
 			base.Remove(item);
@@ -221,7 +221,7 @@ namespace BizHawk.Client.Common
 				if (match.Invoke(m))
 				{
 					_movie.ChangeLog.AddMarkerChange(null, m.Frame, m.Message);
-					_movie.TasStateManager.EvictReserved(m.Frame - 1);
+					_movie.TasStateManager.Unreserve(m.Frame - 1);
 				}
 			}
 

src/BizHawk.Client.Common/movie/tasproj/ZwinderStateManager.cs

@@ -93,22 +93,22 @@ namespace BizHawk.Client.Common
 
 			if (keepOldStates)
 			{
-				// For ancients ... lets just make sure we aren't keeping states with a gap below the new interval
+				// For ancients, let's throw out states if doing so still satisfies the ancient state interval.
 				if (settings.AncientStateInterval > _ancientInterval)
 				{
-					int lastReserved = 0;
+					List<int> reservedFrames = _reserved.Keys.ToList();
-					List<int> framesToRemove = new List<int>();
+					reservedFrames.Sort();
-					foreach (int f in _reserved.Keys)
+					for (int i = 1; i < reservedFrames.Count - 1; i++)
 					{
-						if (!_reserveCallback(f) && f - lastReserved < settings.AncientStateInterval)
+						if (_reserveCallback(reservedFrames[i]))
-							framesToRemove.Add(f);
+							continue;
-						else
+
-							lastReserved = f;
+						if (reservedFrames[i + 1] - reservedFrames[i - 1] <= settings.AncientStateInterval)
+						{
+							EvictReserved(reservedFrames[i]);
+							reservedFrames.RemoveAt(i);
+							i--;
 						}
-					foreach (int f in framesToRemove)
-					{
-						if (f != 0)
-							EvictReserved(f);
 					}
 				}
 			}
@@ -297,7 +297,7 @@ namespace BizHawk.Client.Common
 			}
 		}
 
-		public void EvictReserved(int frame)
+		private void EvictReserved(int frame)
 		{
 			if (frame == 0)
 			{
@@ -311,6 +311,16 @@ namespace BizHawk.Client.Common
 			}
 		}
 
+		public void Unreserve(int frame)
+		{
+			// Before removing the state, check if it should be still be reserved.
+			// For now, this just means checking if we need to keep this state to satisfy the ancient interval.
+			if (ShouldKeepForAncient(frame))
+				return;
+
+			EvictReserved(frame);
+		}
+
 		public void Capture(int frame, IStatable source, bool force = false)
 		{
 			// We already have this state, no need to capture
@@ -366,42 +376,40 @@ namespace BizHawk.Client.Common
 						index2 =>
 						{
 							var state2 = _recent.GetState(index2);
-							StateCache.Remove(state2.Frame);
-
 							var isReserved = _reserveCallback(state2.Frame);
 
 							// Add to reserved if reserved, or if it matches an "ancient" state consideration
-							if (isReserved || !HasNearByReserved(state2.Frame))
+							if (isReserved || ShouldKeepForAncient(state2.Frame))
 							{
 								AddToReserved(state2);
 							}
+							else
+								StateCache.Remove(state2.Frame);
 						});
 				});
 		}
 
-		// Returns whether or not a frame has a reserved state within the frame interval on either side of it
+		/// <summary>
-		private bool HasNearByReserved(int frame)
+		/// Will removing the state on this leave us with a gap larger than the ancient interval?
+		/// </summary>
+		private bool ShouldKeepForAncient(int frame)
 		{
-			// An easy optimization, we know frame 0 always exists
+			int index = StateCache.BinarySearch(frame + 1);
-			if (frame < _ancientInterval)
+			if (index < 0)
+				index = ~index;
+			if (index == StateCache.Count)
 			{
-				return true;
+				// There is no future state... so why are we wanting to evict this state?
-			}
+				// We aren't decaying from _recent, that's for sure. So,
-
-			// Has nearby before
-			if (_reserved.Any(kvp => kvp.Key < frame && kvp.Key > frame - _ancientInterval))
-			{
-				return true;
-			}
-
-			// Has nearby after
-			if (_reserved.Any(kvp => kvp.Key > frame && kvp.Key < frame + _ancientInterval))
-			{
-				return true;
-			}
-
 				return false;
 			}
+			if (index <= 1)
+				return true; // This should never happen.
+
+			int nextState = StateCache[index];
+			int previousState = StateCache[index - 2]; // assume StateCache[index - 1] == frame
+			return nextState - previousState > _ancientInterval;
+		}
 
 		private bool NeedsGap(int frame)
 		{

src/BizHawk.Tests/Client.Common/Movie/ZwinderStateManagerTests.cs

@@ -413,7 +413,7 @@ namespace BizHawk.Tests.Client.Common.Movie
 
 			for (int i = 400; i < 1000; i += 400)
 			{
-				zw.EvictReserved(i);
+				zw.Unreserve(i);
 			}
 
 			for (int i = 0; i < 10000; i++)