From 885931ae7405d1f01e675de2bc2706db4cce6ced Mon Sep 17 00:00:00 2001
From: Robert <rpvn@robpvn.net>
Date: Sat, 22 Feb 2020 16:13:30 +0000
Subject: [PATCH] Run every color validation together

This ensures that color picks that have multiple issues will
have them all checked at the same time, including ensuring that
the fix for one issue doesn't cause another issue.

Handling of the onError action has been changed from being called
at once to collecting the potential errors in a HashSet to deduplicate
them and then calling onError after a valid color has been found.
(Otherwise you would in the worst case get 256 error messages logged!)
---
 OpenRA.Mods.Common/ColorValidator.cs | 29 ++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/OpenRA.Mods.Common/ColorValidator.cs b/OpenRA.Mods.Common/ColorValidator.cs
index 869019537c..31baa29812 100644
--- a/OpenRA.Mods.Common/ColorValidator.cs
+++ b/OpenRA.Mods.Common/ColorValidator.cs
@@ -55,7 +55,7 @@ namespace OpenRA.Mods.Common
 			return true;
 		}
 
-		public bool IsValid(Color askedColor, out Color forbiddenColor, IEnumerable<Color> terrainColors, IEnumerable<Color> playerColors, Action<string> onError)
+		public bool IsValid(Color askedColor, out Color forbiddenColor, IEnumerable<Color> terrainColors, IEnumerable<Color> playerColors, HashSet<string> errorMessages = null)
 		{
 			// Validate color against HSV
 			float h, s, v;
@@ -64,7 +64,8 @@ namespace OpenRA.Mods.Common
 			askedColor.ToAhsv(out a, out h, out s, out v);
 			if (s < HsvSaturationRange[0] || s > HsvSaturationRange[1] || v < HsvValueRange[0] || v > HsvValueRange[1])
 			{
-				onError("Color was adjusted to be inside the allowed range.");
+				if (errorMessages != null)
+					errorMessages.Add("Color was adjusted to be inside the allowed range.");
 				forbiddenColor = askedColor;
 				return false;
 			}
@@ -72,14 +73,16 @@ namespace OpenRA.Mods.Common
 			// Validate color against the current map tileset
 			if (!IsValid(askedColor, terrainColors, out forbiddenColor))
 			{
-				onError("Color was adjusted to be less similar to the terrain.");
+				if (errorMessages != null)
+					errorMessages.Add("Color was adjusted to be less similar to the terrain.");
 				return false;
 			}
 
 			// Validate color against other clients
 			if (!IsValid(askedColor, playerColors, out forbiddenColor))
 			{
-				onError("Color was adjusted to be less similar to another player.");
+				if (errorMessages != null)
+					errorMessages.Add("Color was adjusted to be less similar to another player.");
 				return false;
 			}
 
@@ -94,9 +97,8 @@ namespace OpenRA.Mods.Common
 			if (TeamColorPresets.Any())
 			{
 				Color forbidden;
-				Action<string> ignoreError = _ => { };
 				foreach (var c in TeamColorPresets.Shuffle(random))
-					if (IsValid(c, out forbidden, terrainColors, playerColors, ignoreError))
+					if (IsValid(c, out forbidden, terrainColors, playerColors))
 						return c;
 			}
 
@@ -107,7 +109,6 @@ namespace OpenRA.Mods.Common
 		{
 			Color color;
 			Color forbidden;
-			Action<string> ignoreError = _ => { };
 			do
 			{
 				var h = random.Next(255) / 255f;
@@ -115,15 +116,17 @@ namespace OpenRA.Mods.Common
 				var v = float2.Lerp(HsvValueRange[0], HsvValueRange[1], random.NextFloat());
 				color = Color.FromAhsv(h, s, v);
 			}
-			while (!IsValid(color, out forbidden, terrainColors, playerColors, ignoreError));
+			while (!IsValid(color, out forbidden, terrainColors, playerColors));
 
 			return color;
 		}
 
 		public Color MakeValid(Color askedColor, MersenneTwister random, IEnumerable<Color> terrainColors, IEnumerable<Color> playerColors, Action<string> onError)
 		{
+			var errorMessages = new HashSet<string>();
+
 			Color forbiddenColor;
-			if (IsValid(askedColor, out forbiddenColor, terrainColors, playerColors, onError))
+			if (IsValid(askedColor, out forbiddenColor, terrainColors, playerColors, errorMessages))
 				return askedColor;
 
 			// Vector between the 2 colors
@@ -160,7 +163,6 @@ namespace OpenRA.Mods.Common
 			};
 
 			var attempt = 1;
-			var allForbidden = terrainColors.Concat(playerColors);
 			Color color;
 			do
 			{
@@ -168,7 +170,7 @@ namespace OpenRA.Mods.Common
 				if (attempt >= 255)
 				{
 					color = RandomPresetColor(random, terrainColors, playerColors);
-					onError("Color could not be adjusted enough, a new color has been picked.");
+					errorMessages.Add("Color could not be adjusted enough, a new color has been picked.");
 					break;
 				}
 
@@ -182,7 +184,10 @@ namespace OpenRA.Mods.Common
 
 				attempt++;
 			}
-			while (!IsValid(color, allForbidden, out forbiddenColor));
+			while (!IsValid(color, out forbiddenColor, terrainColors, playerColors, errorMessages));
+
+			// Coalesce the error messages to only print one of each type despite up to 255 iterations
+			errorMessages.Do(onError);
 
 			return color;
 		}