Fix #14044: Negative string parameters from GS were rendered as zero. (#14049)

String parameters are always stored as uint64_t. Negative values are sign-extended to int64_t and then casted to uint64_t.
The same applies to encoded strings. But ScriptText encoded them as int64_t.

Co-authored-by: rubidium42 <rubidium42@users.noreply.github.com>

src/saveload/saveload.cpp

@@ -981,6 +981,43 @@ void FixSCCEncoded(std::string &str, bool fix_code)
 	str = std::move(result);
 }
 
+/**
+ * Scan the string for SCC_ENCODED_NUMERIC with negative values, and reencode them as uint64_t.
+ * @param str the string to fix.
+ */
+void FixSCCEncodedNegative(std::string &str)
+{
+	if (str.empty()) return;
+
+	StringConsumer consumer(str);
+
+	/* Check whether this is an encoded string */
+	if (!consumer.ReadUtf8If(SCC_ENCODED)) return;
+
+	std::string result;
+	StringBuilder builder(result);
+	builder.PutUtf8(SCC_ENCODED);
+	while (consumer.AnyBytesLeft()) {
+		/* Copy until next record */
+		builder.Put(consumer.ReadUntilUtf8(SCC_RECORD_SEPARATOR, StringConsumer::READ_ONE_SEPARATOR));
+
+		/* Check whether this is a numeric parameter */
+		if (!consumer.ReadUtf8If(SCC_ENCODED_NUMERIC)) continue;
+		builder.PutUtf8(SCC_ENCODED_NUMERIC);
+
+		/* First try unsigned */
+		if (auto u = consumer.TryReadIntegerBase<uint64_t>(16); u.has_value()) {
+			builder.PutIntegerBase<uint64_t>(*u, 16);
+		} else {
+			/* Read as signed, store as unsigned */
+			auto s = consumer.ReadIntegerBase<int64_t>(16);
+			builder.PutIntegerBase<uint64_t>(static_cast<uint64_t>(s), 16);
+		}
+	}
+
+	str = std::move(result);
+}
+
 /**
  * Read the given amount of bytes from the buffer into the string.
  * @param str The string to write to.
@@ -1024,6 +1061,7 @@ static void SlStdString(void *ptr, VarType conv)
 			if ((conv & SLF_ALLOW_CONTROL) != 0) {
 				settings.Set(StringValidationSetting::AllowControlCode);
 				if (IsSavegameVersionBefore(SLV_ENCODED_STRING_FORMAT)) FixSCCEncoded(*str, IsSavegameVersionBefore(SLV_169));
+				if (IsSavegameVersionBefore(SLV_FIX_SCC_ENCODED_NEGATIVE)) FixSCCEncodedNegative(*str);
 			}
 			if ((conv & SLF_ALLOW_NEWLINE) != 0) {
 				settings.Set(StringValidationSetting::AllowNewline);

src/saveload/saveload.h

@@ -400,6 +400,7 @@ enum SaveLoadVersion : uint16_t {
 	SLV_ENCODED_STRING_FORMAT,              ///< 350  PR#13499 Encoded String format changed.
 	SLV_PROTECT_PLACED_HOUSES,              ///< 351  PR#13270 Houses individually placed by players can be protected from town/AI removal.
 	SLV_SCRIPT_SAVE_INSTANCES,              ///< 352  PR#13556 Scripts are allowed to save instances.
+	SLV_FIX_SCC_ENCODED_NEGATIVE,           ///< 353  PR#14049 Fix encoding of negative parameters.
 
 	SL_MAX_VERSION,                         ///< Highest possible saveload version
 };

src/script/api/script_text.cpp

@@ -211,7 +211,8 @@ void ScriptText::ParamCheck::Encode(StringBuilder &builder, std::string_view cmd
 		void operator()(const SQInteger &value)
 		{
 			this->builder.PutUtf8(SCC_ENCODED_NUMERIC);
-			this->builder.PutIntegerBase(value, 16);
+			/* Sign-extend the value, then store as unsigned */
+			this->builder.PutIntegerBase<uint64_t>(static_cast<uint64_t>(static_cast<int64_t>(value)), 16);
 		}
 
 		void operator()(const ScriptTextRef &value)

src/tests/string_func.cpp

@@ -464,6 +464,9 @@ TEST_CASE("FixSCCEncoded")
 	/* Test conversion with one numeric parameter. */
 	CHECK(FixSCCEncodedWrapper("\uE00022:1", false) == Compose(SCC_ENCODED, "22", SCC_RECORD_SEPARATOR, SCC_ENCODED_NUMERIC, "1"));
 
+	/* Test conversion with signed numeric parameter. */
+	CHECK(FixSCCEncodedWrapper("\uE00022:-1", false) == Compose(SCC_ENCODED, "22", SCC_RECORD_SEPARATOR, SCC_ENCODED_NUMERIC, "-1"));
+
 	/* Test conversion with two numeric parameters. */
 	CHECK(FixSCCEncodedWrapper("\uE0003:12:2", false) == Compose(SCC_ENCODED, "3", SCC_RECORD_SEPARATOR, SCC_ENCODED_NUMERIC, "12", SCC_RECORD_SEPARATOR, SCC_ENCODED_NUMERIC, "2"));
 
@@ -480,7 +483,27 @@ TEST_CASE("FixSCCEncoded")
 	CHECK(FixSCCEncodedWrapper("\uE000777:\uE0008888:\"Foo\":\"BarBaz\"", false) == Compose(SCC_ENCODED, "777", SCC_RECORD_SEPARATOR, SCC_ENCODED, "8888", SCC_RECORD_SEPARATOR, SCC_ENCODED_STRING, "Foo", SCC_RECORD_SEPARATOR, SCC_ENCODED_STRING, "BarBaz"));
 }
 
-TEST_CASE("EncodedString::ReplaceParam")
+extern void FixSCCEncodedNegative(std::string &str);
+
+/* Helper to call FixSCCEncodedNegative and return the result in a new string. */
+static std::string FixSCCEncodedNegativeWrapper(const std::string &str)
+{
+	std::string result = str;
+	FixSCCEncodedNegative(result);
+	return result;
+}
+
+TEST_CASE("FixSCCEncodedNegative")
+{
+	auto positive = Compose(SCC_ENCODED, "777", SCC_RECORD_SEPARATOR, SCC_ENCODED_NUMERIC, "ffffffffffffffff");
+	auto negative = Compose(SCC_ENCODED, "777", SCC_RECORD_SEPARATOR, SCC_ENCODED_NUMERIC, "-1");
+
+	CHECK(FixSCCEncodedNegativeWrapper("") == "");
+	CHECK(FixSCCEncodedNegativeWrapper(positive) == positive);
+	CHECK(FixSCCEncodedNegativeWrapper(negative) == positive);
+}
+
+TEST_CASE("EncodedString::ReplaceParam - positive")
 {
 	/* Test that two encoded strings with different parameters are not the same. */
 	EncodedString string1 = GetEncodedString(STR_NULL, "Foo", 10, "Bar");
@@ -491,3 +514,18 @@ TEST_CASE("EncodedString::ReplaceParam")
 	EncodedString string3 = string1.ReplaceParam(1, 15);
 	CHECK(string2 == string3);
 }
+
+TEST_CASE("EncodedString::ReplaceParam - negative")
+{
+	EncodedString string1 = GetEncodedString(STR_NULL, "Foo", -1, "Bar");
+	EncodedString string2 = GetEncodedString(STR_NULL, "Foo", -2, "Bar");
+	EncodedString string3 = GetEncodedString(STR_NULL, "Foo", 0xFFFF'FFFF'FFFF'FFFF, "Bar");
+	/* Test that two encoded strings with different parameters are not the same. */
+	CHECK(string1 != string2);
+	/* Test that signed values are stored as unsigned. */
+	CHECK(string1 == string3);
+
+	/* Test that replacing parameter results in the same string. */
+	EncodedString string4 = string1.ReplaceParam(1, -2);
+	CHECK(string2 == string4);
+}