Better translatable string registration logic for Lua spell effects

config/schemas/spellEffect.json

@@ -21,6 +21,13 @@
 			"type" : "string",
 			"description" : "path to the script that represents this effect"
 		},
+		"stringRegistrations" : {
+			"type" : "array",
+			"items" : {
+				"type" : "string",
+				"description" : "path to the script that represents this effect"
+			}
+		},
 		"schema" : {
 			"type" : "object",
 			"description" : "Json schema for validating spell effect",

config/spellEffects.json

@@ -212,6 +212,7 @@
 	"timed" : {
 		"type" : "lua",
 		"script" : "timed",
+		"stringRegistrations" : ["battleLogSingular", "battleLogPlural"],
 		"schema" : {
 			"properties" : {
 				"type" : {},
@@ -220,14 +221,8 @@
 				"ignoreImmunity" : { "type" : "boolean" },
 				"chainLength" : { "type" : "number" },
 				"cumulative" : { "type" : "boolean" },
-				"battleLogMessage" : {
-					"type" : "object",
-					"additionalProperties" : false,
-					"properties" : {
-						"singular" : { "type" : "string" },
-						"plural" : { "type" : "string" }
-					}
-				},
+				"battleLogSingular" : { "type" : "string" },
+				"battleLogPlural" : { "type" : "string" },
 				"bonus" : { "additionalProperties" : { "$ref" : "bonusInstance.json" } }
 			},
 			"additionalProperties" : false

config/spells/ability.json

@@ -17,7 +17,8 @@
 				"battleEffects" : {
 					"petrification" : {
 						"type" : "core:timed",
-						"battleLogMessage" : { "singular" : "@core.genrltxt.558", "plural" : "@core.genrltxt.559" },
+						"battleLogSingular" : "@core.genrltxt.558",
+						"battleLogPlural" : "@core.genrltxt.559",
 						"bonus" : {
 							"notActive" : {
 								"val" : 0,
@@ -75,7 +76,8 @@
 				"battleEffects" : {
 					"poisoning" : {
 						"type" : "core:timed",
-						"battleLogMessage" : { "singular" : "@core.genrltxt.561", "plural" : "@core.genrltxt.562" },
+						"battleLogSingular" : "@core.genrltxt.561",
+						"battleLogPlural" : "@core.genrltxt.562",
 						"bonus" : {
 							"poison" : {
 								"val" : 30,
@@ -123,7 +125,8 @@
 				"battleEffects" : {
 					"binding" : {
 						"type" : "core:timed",
-						"battleLogMessage" : { "singular" : "@core.genrltxt.560", "plural" : "@core.genrltxt.560" },
+						"battleLogSingular" : "@core.genrltxt.560",
+						"battleLogPlural" : "@core.genrltxt.560",
 						"bonus" : {
 							"bindEffect" : {
 								"val" : 0,
@@ -158,7 +161,8 @@
 				"battleEffects" : {
 					"illness" : {
 						"type" : "core:timed",
-						"battleLogMessage" : { "singular" : "@core.genrltxt.553", "plural" : "@core.genrltxt.554" },
+						"battleLogSingular" : "@core.genrltxt.553",
+						"battleLogPlural" : "@core.genrltxt.554",
 						"bonus" : {
 							"attack" : {
 								"val" : -2,
@@ -206,7 +210,8 @@
 				"battleEffects" : {
 					"paralysis" : {
 						"type" : "core:timed",
-						"battleLogMessage" : { "singular" : "@core.genrltxt.563", "plural" : "@core.genrltxt.564" },
+						"battleLogSingular" : "@core.genrltxt.563",
+						"battleLogPlural" : "@core.genrltxt.564",
 						"bonus" : {
 							"notActive" : {
 								"val" : 0,

docs/developers/Lua_Scripting_System.md

@@ -30,10 +30,8 @@ This page describes the internal working of the Lua scripting module. For usage
 - Review spell effect-related API and ensure that it follows rules described here:
   - Remove usage of numeric identifiers from script. In cases where entity does not exists such as `PlayerColor`, replace them with copyable API class
   - Review UnitState class and check its mutable methods - do we need all of those? Should we name them differently?
-  - try to remove remaining hardcoded bits of SpellID's CLONE, TELEPORT, SACRIFICE, STONE_GAZE, SLAYER, AIR_SHIELD, POISON, RESURRECTION, FIRE_SHIELD, DEATH_STARE, as well as some entries in .lua
+  - try to remove remaining hardcoded bits of SpellID's CLONE, STONE_GAZE, SLAYER, AIR_SHIELD, POISON, RESURRECTION, FIRE_SHIELD, DEATH_STARE, as well as some entries in .lua
   - decide on how to handle RNG support for Lua scripts
-  - `battleLogMessage` entries in timed spell effects without a leading `@` are currently ignored in Lua scripts; C++ resolved them as hierarchical text IDs (`spell.{scope}.{id}.{effectName}.battleLogMessage.{field}`). Support for this needs to be added to the Lua Timed effect and the scripting infrastructure.
-    - add suport for list of strings that effect wants to register?
 
 ## Future improvements
 

lib/modding/CModHandler.cpp

@@ -313,15 +313,15 @@ void CModHandler::load()
 
 	content->loadCustom();
 
-	for(const TModID & modName : activeMods)
-		loadTranslation(modName);
-
 	logMod->info("\tLoading mod data");
 	LIBRARY->creh->loadCrExpMod();
 	LIBRARY->identifiersHandler->finalize();
 	logMod->info("\tResolving identifiers");
 
 	content->afterLoadFinalization();
+	for(const TModID & modName : activeMods)
+		loadTranslation(modName);
+
 	logMod->info("\tHandlers post-load finalization");
 	logMod->info("\tAll game content loaded");
 }

lib/spells/CSpellHandler.cpp

@@ -473,25 +473,6 @@ std::shared_ptr<CSpell> CSpellHandler::loadFromJson(const std::string & scope, c
 		{
 			levelObject.battleEffects = levelNode["battleEffects"];
 
-			for(const auto & effectEntry : levelNode["battleEffects"].Struct())
-			{
-				const JsonNode & msgNode = effectEntry.second["battleLogMessage"];
-				if(msgNode.isStruct())
-				{
-					auto registerField = [&](const std::string & field)
-					{
-						const std::string & value = msgNode[field].String();
-						if(!value.empty() && value.at(0) != '@')
-						{
-							TextIdentifier textID("spell", scope, identifier, effectEntry.first, "battleLogMessage", field);
-							LIBRARY->generaltexth->registerString(scope, textID, msgNode[field]);
-						}
-					};
-					registerField("singular");
-					registerField("plural");
-				}
-			}
-
 			if(!levelObject.cumulativeEffects.Struct().empty() || !levelObject.effects.Struct().empty() || spell->isOffensive())
 				logGlobal->error("Mixing %s special effects with old format effects gives unpredictable result", spell->getNameTranslated());
 		}

lib/spells/ISpellMechanics.cpp

@@ -65,8 +65,7 @@ protected:
 
 	void loadEffects(const JsonNode & config, const int level)
 	{
-		JsonDeserializer deser(nullptr, config);
-		effects->serializeJson(deser, level, spell->modScope, spell->identifier);
+		effects->data.at(level) = effects::Effects::loadJson(config, spell->modScope, spell->identifier);
 	}
 private:
 	std::shared_ptr<IReceptiveCheck> targetCondition;

lib/spells/effects/Effect.cpp

@@ -11,7 +11,7 @@
 
 #include "Effect.h"
 
-#include "../../serializer/JsonSerializeFormat.h"
+#include "../../json/JsonNode.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
 
@@ -30,11 +30,11 @@ bool Effect::applicableTarget(Problem & problem, const Mechanics * m, const Targ
 	return true;
 }
 
-void Effect::serializeJson(JsonSerializeFormat & handler)
+void Effect::init(JsonNode data)
 {
-	handler.serializeBool("indirect", indirect, false);
-	handler.serializeBool("optional", optional, false);
-	serializeJsonEffect(handler);
+	indirect = data["indirect"].Bool();
+	optional = data["optional"].Bool();
+	initImpl(std::move(data));
 }
 
 }

lib/spells/effects/Effect.h

@@ -14,13 +14,13 @@
 #include <vcmi/spells/Magic.h>
 
 #include "../../constants/EntityIdentifiers.h"
+#include "../../json/JsonNode.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
 
 class BattleHex;
 class BattleHexArray;
 class CBattleInfoCallback;
-class JsonSerializeFormat;
 class ServerCallback;
 
 namespace vstd
@@ -100,11 +100,12 @@ public:
 	// Returns total damage or heal amount that this spell will result in when cast on unit
 	virtual SpellEffectValue getHealthChange(const Mechanics * m, const Target & spellTarget) const { return {};}
 
-	/// Serializes (or deserializes) parameters of Effect
-	void serializeJson(JsonSerializeFormat & handler);
+	/// Initializes Effect from its (already preprocessed) JSON definition.
+	/// Reads common flags from the node and delegates the rest to initImpl.
+	void init(JsonNode data);
 
 protected:
-	virtual void serializeJsonEffect(JsonSerializeFormat & handler) = 0;
+	virtual void initImpl(JsonNode data) = 0;
 };
 
 }

lib/spells/effects/Effects.cpp

@@ -16,7 +16,9 @@
 
 #include "../ISpellMechanics.h"
 
-#include "../../serializer/JsonSerializeFormat.h"
+#include "../../GameLibrary.h"
+#include "../../json/JsonNode.h"
+#include "../../modding/IdentifierStorage.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
 
@@ -26,12 +28,6 @@ namespace spells
 namespace effects
 {
 
-void Effects::add(const std::string & name, const std::shared_ptr<Effect>& effect, const int level)
-{
-	effect->name = name;
-	data.at(level)[name] = effect;
-}
-
 bool Effects::applicable(Problem & problem, const Mechanics * m) const
 {
 	//stop on first problem
@@ -129,31 +125,30 @@ Effects::EffectsToApply Effects::prepare(const Mechanics * m, const Target & aim
 	return effectsToApply;
 }
 
-void Effects::serializeJson(JsonSerializeFormat & handler, const int level, const std::string & spellScope, const std::string & spellIdentifier)
+Effects::EffectsMap Effects::loadJson(const JsonNode & effectMap, const std::string & spellScope, const std::string & spellIdentifier)
 {
-	assert(!handler.saving);
+	EffectsMap result;
 
-	const JsonNode & effectMap = handler.getCurrent();
-
-	for(const auto & p : effectMap.Struct())
+	for(const auto & [name, raw] : effectMap.Struct())
 	{
-		const std::string & name = p.first;
+		SpellEffectID effectID(*LIBRARY->identifiers()->getIdentifier("spellEffect", raw["type"]));
 
-		auto guard = handler.enterStruct(name);
-		SpellEffectID effectID(*LIBRARY->identifiers()->getIdentifier("spellEffect", p.second["type"]));
-
-		LIBRARY->spellEffects()->validateEffect(effectID, p.second, spellScope + ":" + spellIdentifier + " effect " + name);
+		JsonNode data = raw;
+		LIBRARY->spellEffects()->prepareEffect(effectID, data, spellScope, spellIdentifier, name);
 
 		auto effect = LIBRARY->spellEffects()->create(effectID);
-		if(effect)
-		{
-			effect->name = name;
-			effect->spellScope = spellScope;
-			effect->spellIdentifier = spellIdentifier;
-			effect->serializeJson(handler);
-			add(name, effect, level);
-		}
+		if(!effect)
+			continue;
+
+		effect->name = name;
+		effect->spellScope = spellScope;
+		effect->spellIdentifier = spellIdentifier;
+		effect->init(std::move(data));
+
+		result.emplace(name, std::move(effect));
 	}
+
+	return result;
 }
 
 }

lib/spells/effects/Effects.h

@@ -32,8 +32,6 @@ public:
 
 	virtual ~Effects() = default;
 
-	void add(const std::string & name, const std::shared_ptr<Effect>& effect, const int level);
-
 	bool applicable(Problem & problem, const Mechanics * m) const;
 	bool applicable(Problem & problem, const Mechanics * m, const Target & aimPoint, const Target & spellTarget) const;
 
@@ -41,7 +39,8 @@ public:
 
 	EffectsToApply prepare(const Mechanics * m, const Target & aimPoint, const Target & spellTarget) const;
 
-	void serializeJson(JsonSerializeFormat & handler, const int level, const std::string & spellScope, const std::string & spellIdentifier);
+	/// Builds the effects map for a single spell level from its JSON config.
+	static EffectsMap loadJson(const JsonNode & effectMap, const std::string & spellScope, const std::string & spellIdentifier);
 };
 
 

lib/spells/effects/SpellEffectHandler.cpp

@@ -11,7 +11,10 @@
 
 #include "SpellEffectHandler.h"
 
+#include "../../GameLibrary.h"
 #include "../../json/JsonUtils.h"
+#include "../../texts/CGeneralTextHandler.h"
+#include "../../texts/TextIdentifier.h"
 
 #include "Effect.h"
 
@@ -53,6 +56,9 @@ void SpellEffectHandler::loadObject(std::string scope, std::string name, const J
 	newEffect.scriptName = data["script"].String();
 	newEffect.validationSchema = data["schema"];
 
+	for(const auto & item : data["stringRegistrations"].Vector())
+		newEffect.stringRegistrations.push_back(item.String());
+
 	registerObject(scope, "spellEffect", name, data, effectTypes.size());
 	effectTypes.push_back(newEffect);
 
@@ -70,11 +76,34 @@ void SpellEffectHandler::afterLoadFinalization()
 
 }
 
-void SpellEffectHandler::validateEffect(SpellEffectID effectID, const JsonNode & data, const std::string & name) const
+void SpellEffectHandler::prepareEffect(SpellEffectID effectID, JsonNode & data, const std::string & spellScope, const std::string & spellIdentifier, const std::string & effectName) const
 {
-	const auto & schema = effectTypes.at(effectID.getNum()).validationSchema;
-	if (!schema.isNull())
-		JsonUtils::validate(data, schema, name);
+	const auto & effectType = effectTypes.at(effectID.getNum());
+	const std::string validationName = spellScope + ":" + spellIdentifier + " effect " + effectName;
+
+	if(!effectType.validationSchema.isNull())
+		JsonUtils::validate(data, effectType.validationSchema, validationName);
+
+	for(const auto & field : effectType.stringRegistrations)
+	{
+		const JsonNode & fieldNode = static_cast<const JsonNode &>(data)[field];
+		if(fieldNode.isNull())
+			continue;
+		const std::string & value = fieldNode.String();
+		if(value.empty())
+			continue;
+
+		if(value.at(0) == '@')
+		{
+			data[field].String() = value.substr(1);
+		}
+		else
+		{
+			TextIdentifier textID("spell", spellScope, spellIdentifier, effectName, field);
+			LIBRARY->generaltexth->registerString(spellScope, textID, fieldNode);
+			data[field].String() = textID.get();
+		}
+	}
 }
 
 }

lib/spells/effects/SpellEffectHandler.h

@@ -27,6 +27,7 @@ struct SpellEffectType
 	std::string type;
 	std::string scriptName;
 	JsonNode validationSchema;
+	std::vector<std::string> stringRegistrations;
 };
 
 class SpellEffectHandler final : public IHandlerBase, public SpellEffectService
@@ -38,7 +39,7 @@ public:
 
 	void registerFactory(const std::string & typeName, std::shared_ptr<ISpellEffectFactory> factory) override;
 
-	void validateEffect(SpellEffectID effectID, const JsonNode & data, const std::string & name) const override;
+	void prepareEffect(SpellEffectID effectID, JsonNode & data, const std::string & spellScope, const std::string & spellIdentifier, const std::string & effectName) const override;
 
 	std::vector<JsonNode> loadLegacyData() override;
 

lib/spells/effects/SpellEffectService.h

@@ -36,7 +36,7 @@ public:
 	virtual ~SpellEffectService() = default;
 	virtual std::shared_ptr<Effect> create(SpellEffectID effectID) const = 0;
 	virtual void registerFactory(const std::string & typeName, std::shared_ptr<ISpellEffectFactory> factory) = 0;
-	virtual void validateEffect(SpellEffectID effectID, const JsonNode & data, const std::string & name) const = 0;
+	virtual void prepareEffect(SpellEffectID effectID, JsonNode & data, const std::string & spellScope, const std::string & spellIdentifier, const std::string & effectName) const = 0;
 
 };
 

luascript/LuaSpellEffect.cpp

@@ -20,14 +20,12 @@
 #include "../lib/spells/ISpellMechanics.h"
 #include "../lib/battle/Unit.h"
 #include "../lib/battle/CBattleInfoCallback.h"
-#include "../lib/serializer/JsonSerializeFormat.h"
 
 static const std::string APPLICABLE_GENERAL = "applicableGeneral";
 static const std::string APPLICABLE_TARGET = "applicableTarget";
 static const std::string FILTER_TARGET = "filterTarget";
 static const std::string TRANSFORM_TARGET = "transformTarget";
 static const std::string APPLY = "apply";
-//static const std::string INITIALIZE = "initialize";
 static const std::string GET_HEALTH_CHANGE = "getHealthChange";
 static const std::string ADJUST_AFFECTED_HEXES = "adjustAffectedHexes";
 static const std::string ADJUST_TARGET_TYPES = "adjustTargetTypes";
@@ -124,9 +122,9 @@ Target LuaSpellEffect::transformTarget(const Mechanics * m, const Target & aimPo
 	return response;
 }
 
-void LuaSpellEffect::serializeJsonEffect(JsonSerializeFormat & handler)
+void LuaSpellEffect::initImpl(JsonNode data)
 {
-	parameters = handler.getCurrent();
+	parameters = std::move(data);
 }
 
 std::shared_ptr<scripting::LuaContext> LuaSpellEffect::resolveScript(const Mechanics * m) const

luascript/LuaSpellEffect.h

@@ -72,7 +72,7 @@ public:
 	SpellEffectValue getHealthChange(const Mechanics * m, const Target & spellTarget) const override;
 
 protected:
-	void serializeJsonEffect(JsonSerializeFormat & handler) override;
+	void initImpl(JsonNode data) override;
 
 private:
 	const LuaScriptInstance * script;

scripts/summon.lua

@@ -37,7 +37,6 @@ function Script:applicableGeneral(mechanics, problem)
 	local creature = LIBRARY:getCreatureByName(self.id)
 
 	if self:summonedCreatureAmount(mechanics) == 0 then
-		print("SpellEffectSummon: zero summoned creatures!")
 		problem:addGeneric(mechanics)
 		return false
 	end
@@ -52,7 +51,6 @@ function Script:applicableGeneral(mechanics, problem)
 		end)
 		local elemental = elementals[1]
 
-		print("SpellEffectSummon - summoning:", creature:getJsonKey(), " elemental is ", elemental)
 		if elemental ~= nil then
 			local hero = mechanics:getHeroCaster()
 			local himHer = "core.genrltxt.539"
@@ -74,7 +72,6 @@ function Script:applicableGeneral(mechanics, problem)
 					append = { "core.genrltxt.538" }
 				})
 			end
-			print("SpellEffectSummon - summoning:", creature:getJsonKey(), " already summoned: ", elemental:getCreature():getJsonKey())
 			return false
 		end
 	end

scripts/timed.lua

@@ -92,7 +92,7 @@ function Script:applyHeroSpecialty(mechanics, buffer, unit)
 	end
 end
 
-function Script:describeEffect(server, battle, unit, bonuses, singular, plural)
+function Script:describeEffect(server, battle, unit, bonuses)
 	-- Age spell: STACK_HEALTH bonus with negative val gets a custom message
 	for _, nb in pairs(bonuses) do
 		if nb.type == "STACK_HEALTH" and (nb.val or 0) < 0 then
@@ -116,9 +116,9 @@ function Script:describeEffect(server, battle, unit, bonuses, singular, plural)
 		end
 	end
 
-	if not plural or plural == "" then return end
+	if not self.battleLogPlural or self.battleLogPlural == "" then return end
 
-	local textID = (singular and singular ~= "" and unit:getCount() == 1) and singular or plural
+	local textID = (self.battleLogSingular and self.battleLogSingular ~= "" and unit:getCount() == 1) and self.battleLogSingular or self.battleLogPlural
 	local nameTextID = unit:getCreature():getNameTextID(unit:getCount())
 	server:appendLog(battle, {
 		append         = { textID },
@@ -131,14 +131,6 @@ function Script:apply(mechanics, server, target)
 	local describe = server:describeChanges()
 	local converted = self:convertBonuses(mechanics)
 
-	local singular, plural
-	if self.battleLogMessage then
-		local s = self.battleLogMessage.singular
-		local p = self.battleLogMessage.plural
-		singular = (s and #s > 1 and s:sub(1, 1) == "@") and s:sub(2) or nil
-		plural   = (p and #p > 1 and p:sub(1, 1) == "@") and p:sub(2) or nil
-	end
-
 	for _, dest in ipairs(target) do
 		local unit = dest.unit
 		if not unit or not unit:isAlive() then goto continue end
@@ -151,7 +143,7 @@ function Script:apply(mechanics, server, target)
 		self:applyHeroSpecialty(mechanics, buffer, unit)
 
 		if describe then
-			self:describeEffect(server, battle, unit, buffer, singular, plural)
+			self:describeEffect(server, battle, unit, buffer)
 		end
 
 		for _, nb in pairs(buffer) do

test/spells/effects/EffectFixture.cpp

@@ -16,9 +16,12 @@
 #include "../../../lib/networkPacks/PacksForClientBattle.h"
 #include "../../../lib/networkPacks/SetStackEffect.h"
 
-#include "../../../lib/serializer/JsonDeserializer.h"
 #include "../../../lib/spells/effects/SpellEffectService.h"
 
+#include "../../../lib/GameLibrary.h"
+#include "../../../lib/modding/IdentifierStorage.h"
+#include "../../../lib/modding/ModScope.h"
+
 bool battle::operator==(const Destination& left, const Destination& right)
 {
 	return left.unitValue == right.unitValue && left.hexValue == right.hexValue;
@@ -61,8 +64,7 @@ void EffectFixture::setupEffect(const JsonNode & effectConfig)
 
 	JsonNode effectConfigActual = effectConfig;
 	effectConfigActual.setModScope("game");
-	JsonDeserializer deser(nullptr, effectConfigActual);
-	subject->serializeJson(deser);
+	subject->init(std::move(effectConfigActual));
 }