Pass new tests: 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> framesToRemove = new List<int>();
-					foreach (int f in _reserved.Keys)
+					List<int> reservedFrames = _reserved.Keys.ToList();
+					reservedFrames.Sort();
+					for (int i = 1; i < reservedFrames.Count - 1; i++)
 					{
-						if (!_reserveCallback(f) && f - lastReserved < settings.AncientStateInterval)
-							framesToRemove.Add(f);
-						else
-							lastReserved = f;
-					}
-					foreach (int f in framesToRemove)
-					{
-						if (f != 0)
-							EvictReserved(f);
+						if (_reserveCallback(reservedFrames[i]))
+							continue;
+
+						if (reservedFrames[i + 1] - reservedFrames[i - 1] <= settings.AncientStateInterval)
+						{
+							EvictReserved(reservedFrames[i]);
+							reservedFrames.RemoveAt(i);
+							i--;
+						}
 					}
 				}
 			}
@@ -297,7 +297,7 @@ namespace BizHawk.Client.Common
 			}
 		}
 
-		public void EvictReserved(int frame)
+		private void EvictReserved(int frame)
 		{
 			if (frame == 0)
 			{
@@ -311,6 +311,49 @@ 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);
+		}
+
+		private void PossiblyMoveToReserved(ZwinderBuffer buffer, int stateIndex)
+		{
+			var state = buffer.GetState(stateIndex);
+
+			// Add to reserved buffer if externally reserved, or if it matches an "ancient" state consideration
+			if (_reserveCallback(state.Frame) || ShouldKeepForAncient(state.Frame))
+				AddToReserved(state);
+			else
+				StateCache.Remove(state.Frame);
+		}
+
+		/// <summary>
+		/// Will removing the state on this leave us with a gap larger than the ancient interval?
+		/// </summary>
+		private bool ShouldKeepForAncient(int frame)
+		{
+			int index = StateCache.BinarySearch(frame + 1);
+			if (index < 0)
+				index = ~index;
+			if (index == StateCache.Count)
+			{
+				// 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,
+				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;
+		}
+
 		public void Capture(int frame, IStatable source, bool force = false)
 		{
 			// We already have this state, no need to capture
@@ -363,46 +406,10 @@ namespace BizHawk.Client.Common
 							rs.CopyTo(s);
 							AddStateCache(state.Frame);
 						},
-						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))
-							{
-								AddToReserved(state2);
-							}
-						});
+						index2 => PossiblyMoveToReserved(_recent, index2));
 				});
 		}
 
-		// Returns whether or not a frame has a reserved state within the frame interval on either side of it
-		private bool HasNearByReserved(int frame)
-		{
-			// An easy optimization, we know frame 0 always exists
-			if (frame < _ancientInterval)
-			{
-				return true;
-			}
-
-			// 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;
-		}
-
 		private bool NeedsGap(int frame)
 		{
 			// We don't want to "fill gaps" if we are past the latest state in the current/recent buffers.
@@ -445,17 +452,7 @@ namespace BizHawk.Client.Common
 					AddStateCache(frame);
 					source.SaveStateBinary(new BinaryWriter(s));
 				},
-				index =>
-				{
-					var state = _gapFiller.GetState(index);
-					StateCache.Remove(state.Frame);
-
-					if (_reserveCallback(state.Frame))
-					{
-						AddToReserved(state);
-						return;
-					}
-				});
+				index => PossiblyMoveToReserved(_gapFiller, index));
 		}
 
 		public void Clear()

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++)