Improve sheet packing in Dune 2000.

In a3d0a50f4d, SpriteCache is updated to sort sprites by height before adding them onto the sheet. This improves packing by reducing wasted space as the sprites are packed onto the sheet. D2kSpriteSequence does not fully benefit from this change, as it creates additional sprites afterwards in the ResolveSprites method. These are not sorted, so they often waste space due to height changes between adjacent sprites and cause an inefficient packing. Sorting them in place is insufficient, as each sequence performs the operation independently. So sets of sprites across different sequences end up with poor packing overall. We need all the sprites to be collected together and sorted in one place for best effect.

We restructure SpriteCache to allow a frame mutation function to be provided when reserving sprites. This removes the need for the ReserveFrames and ResolveFrames methods in SpriteCache. D2kSpriteSequence can use this new function to pass in the required modification, and no longer has to add frames to the sheet builder itself. Now the SpriteCache can apply the desired frame mutations, it can batch together these mutated frames with the other frames and sort them all as a single batch. With all frames sorted together the maximum benefit of this packing approach is realised.

This reduces the number of BGRA sheets required for the d2k mod from 3 to 2.

OpenRA.Game/Graphics/SpriteCache.cs

@@ -24,11 +24,9 @@ namespace OpenRA.Graphics
 		readonly ISpriteLoader[] loaders;
 		readonly IReadOnlyFileSystem fileSystem;
 
-		readonly Dictionary<int, (int[] Frames, MiniYamlNode.SourceLocation Location, bool Premultiplied)> spriteReservations = new();
+		readonly Dictionary<int, (int[] Frames, MiniYamlNode.SourceLocation Location, Func<ISpriteFrame, ISpriteFrame> AdjustFrame, bool Premultiplied)> spriteReservations = new();
-		readonly Dictionary<int, (int[] Frames, MiniYamlNode.SourceLocation Location)> frameReservations = new();
 		readonly Dictionary<string, List<int>> reservationsByFilename = new();
 
-		readonly Dictionary<int, ISpriteFrame[]> resolvedFrames = new();
 		readonly Dictionary<int, Sprite[]> resolvedSprites = new();
 
 		readonly Dictionary<int, (string Filename, MiniYamlNode.SourceLocation Location)> missingFiles = new();
@@ -47,18 +45,10 @@ namespace OpenRA.Graphics
 			this.loaders = loaders;
 		}
 
-		public int ReserveSprites(string filename, IEnumerable<int> frames, MiniYamlNode.SourceLocation location, bool premultiplied = false)
+		public int ReserveSprites(string filename, IEnumerable<int> frames, MiniYamlNode.SourceLocation location, Func<ISpriteFrame, ISpriteFrame> adjustFrame = null, bool premultiplied = false)
 		{
 			var token = nextReservationToken++;
-			spriteReservations[token] = (frames?.ToArray(), location, premultiplied);
+			spriteReservations[token] = (frames?.ToArray(), location, adjustFrame, premultiplied);
-			reservationsByFilename.GetOrAdd(filename, _ => new List<int>()).Add(token);
-			return token;
-		}
-
-		public int ReserveFrames(string filename, IEnumerable<int> frames, MiniYamlNode.SourceLocation location)
-		{
-			var token = nextReservationToken++;
-			frameReservations[token] = (frames?.ToArray(), location);
 			reservationsByFilename.GetOrAdd(filename, _ => new List<int>()).Add(token);
 			return token;
 		}
@@ -84,34 +74,19 @@ namespace OpenRA.Graphics
 			foreach (var sb in SheetBuilders.Values)
 				sb.Current.CreateBuffer();
 
-			var pendingResolve = new List<(string Filename, int FrameIndex, bool Premultiplied, ISpriteFrame Frame, Sprite[] SpritesForToken)>();
+			var pendingResolve = new List<(
+				string Filename,
+				int FrameIndex,
+				bool Premultiplied,
+				Func<ISpriteFrame, ISpriteFrame> AdjustFrame,
+				ISpriteFrame Frame,
+				Sprite[] SpritesForToken)>();
 			foreach (var (filename, tokens) in reservationsByFilename)
 			{
 				modData.LoadScreen?.Display();
 				var loadedFrames = GetFrames(fileSystem, filename, loaders, out _);
 				foreach (var token in tokens)
 				{
-					if (frameReservations.TryGetValue(token, out var rf))
-					{
-						if (loadedFrames != null)
-						{
-							if (rf.Frames != null)
-							{
-								var resolved = new ISpriteFrame[loadedFrames.Length];
-								foreach (var i in rf.Frames)
-									resolved[i] = loadedFrames[i];
-								resolvedFrames[token] = resolved;
-							}
-							else
-								resolvedFrames[token] = loadedFrames;
-						}
-						else
-						{
-							resolvedFrames[token] = null;
-							missingFiles[token] = (filename, rf.Location);
-						}
-					}
-
 					if (spriteReservations.TryGetValue(token, out var rs))
 					{
 						if (loadedFrames != null)
@@ -121,7 +96,12 @@ namespace OpenRA.Graphics
 							var frames = rs.Frames ?? Enumerable.Range(0, loadedFrames.Length);
 
 							foreach (var i in frames)
-								pendingResolve.Add((filename, i, rs.Premultiplied, loadedFrames[i], resolved));
+							{
+								var frame = loadedFrames[i];
+								if (rs.AdjustFrame != null)
+									frame = rs.AdjustFrame(frame);
+								pendingResolve.Add((filename, i, rs.Premultiplied, rs.AdjustFrame, frame, resolved));
+							}
 						}
 						else
 						{
@@ -136,13 +116,18 @@ namespace OpenRA.Graphics
 			// We can achieve better sheet packing by keeping sprites with similar heights together.
 			var orderedPendingResolve = pendingResolve.OrderBy(x => x.Frame.Size.Height);
 
-			var spriteCache = new Dictionary<(string Filename, int FrameIndex, bool Premultiplied), Sprite>(pendingResolve.Count);
+			var spriteCache = new Dictionary<(
-			foreach (var (filename, frameIndex, premultiplied, frame, spritesForToken) in orderedPendingResolve)
+				string Filename,
+				int FrameIndex,
+				bool Premultiplied,
+				Func<ISpriteFrame, ISpriteFrame> AdjustFrame),
+				Sprite>(pendingResolve.Count);
+			foreach (var (filename, frameIndex, premultiplied, adjustFrame, frame, spritesForToken) in orderedPendingResolve)
 			{
 				// Premultiplied and non-premultiplied sprites must be cached separately
 				// to cover the case where the same image is requested in both versions.
 				spritesForToken[frameIndex] = spriteCache.GetOrAdd(
-					(filename, frameIndex, premultiplied),
+					(filename, frameIndex, premultiplied, adjustFrame),
 					_ =>
 					{
 						var sheetBuilder = SheetBuilders[SheetBuilder.FrameTypeToSheetType(frame.Type)];
@@ -153,7 +138,6 @@ namespace OpenRA.Graphics
 			}
 
 			spriteReservations.Clear();
-			frameReservations.Clear();
 			reservationsByFilename.Clear();
 
 			foreach (var sb in SheetBuilders.Values)
@@ -170,16 +154,6 @@ namespace OpenRA.Graphics
 			return resolved;
 		}
 
-		public ISpriteFrame[] ResolveFrames(int token)
-		{
-			var resolved = resolvedFrames[token];
-			resolvedFrames.Remove(token);
-			if (missingFiles.TryGetValue(token, out var r))
-				throw new FileNotFoundException($"{r.Location}: {r.Filename} not found", r.Filename);
-
-			return resolved;
-		}
-
 		public IEnumerable<(string Filename, MiniYamlNode.SourceLocation Location)> MissingFiles => missingFiles.Values.ToHashSet();
 
 		public void Dispose()

OpenRA.Mods.D2k/Graphics/D2kSpriteSequence.cs

@@ -62,6 +62,13 @@ namespace OpenRA.Mods.Cnc.Graphics
 			var offset = LoadField(Offset, data, defaults);
 			var blendMode = LoadField(BlendMode, data, defaults);
 
+			Func<ISpriteFrame, ISpriteFrame> adjustFrame = null;
+			if (remapColor != default || convertShroudToFog)
+				adjustFrame = RemapFrame;
+
+			ISpriteFrame RemapFrame(ISpriteFrame f) =>
+				(f is R8Loader.RemappableFrame rf) ? rf.WithSequenceFlags(useShadow, convertShroudToFog, remapColor) : f;
+
 			var combineNode = data.NodeWithKeyOrDefault(Combine.Key);
 			if (combineNode != null)
 			{
@@ -75,11 +82,7 @@ namespace OpenRA.Mods.Cnc.Graphics
 
 					foreach (var f in ParseCombineFilenames(modData, tileset, subFrames, subData))
 					{
-						int token;
+						var token = cache.ReserveSprites(f.Filename, f.LoadFrames, f.Location, adjustFrame);
-						if (remapColor != default || convertShroudToFog)
-							token = cache.ReserveFrames(f.Filename, f.LoadFrames, f.Location);
-						else
-							token = cache.ReserveSprites(f.Filename, f.LoadFrames, f.Location);
 
 						spritesToLoad.Add(new SpriteReservation
 						{
@@ -98,11 +101,7 @@ namespace OpenRA.Mods.Cnc.Graphics
 			{
 				foreach (var f in ParseFilenames(modData, tileset, frames, data, defaults))
 				{
-					int token;
+					var token = cache.ReserveSprites(f.Filename, f.LoadFrames, f.Location, adjustFrame);
-					if (remapColor != default || convertShroudToFog)
-						token = cache.ReserveFrames(f.Filename, f.LoadFrames, f.Location);
-					else
-						token = cache.ReserveSprites(f.Filename, f.LoadFrames, f.Location);
 
 					spritesToLoad.Add(new SpriteReservation
 					{
@@ -129,20 +128,7 @@ namespace OpenRA.Mods.Cnc.Graphics
 
 			var allSprites = spritesToLoad.SelectMany(r =>
 			{
-				Sprite[] resolved;
+				var resolved = cache.ResolveSprites(r.Token);
-				if (remapColor != default || convertShroudToFog)
-					resolved = cache.ResolveFrames(r.Token)
-						.Select(f => (f is R8Loader.RemappableFrame rf) ? rf.WithSequenceFlags(useShadow, convertShroudToFog, remapColor) : f)
-						.Select(f =>
-						{
-							if (f == null)
-								return null;
-
-							return cache.SheetBuilders[SheetBuilder.FrameTypeToSheetType(f.Type)]
-								.Add(f.Data, f.Type, f.Size, 0, f.Offset);
-						}).ToArray();
-				else
-					resolved = cache.ResolveSprites(r.Token);
 
 				if (r.Frames != null)
 					resolved = r.Frames.Select(f => resolved[f]).ToArray();