diff --git a/docs/developers/Lua_Scripting_System.md b/docs/developers/Lua_Scripting_System.md index 4b1a3b2d1..3b3aa04e0 100644 --- a/docs/developers/Lua_Scripting_System.md +++ b/docs/developers/Lua_Scripting_System.md @@ -24,12 +24,10 @@ This page describes the internal working of the Lua scripting module. For usage - try to remove or at least reduce usage of abbreviations - 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? -- Add "preprocess" or "initialize" function to initialize parameters (e.g. load string ID and resolve it to Creature type) - decide how to handle inheritance in Lua API. For example a lot of classes would need methods like getAllBonuses - consider changing list of exported methods to std::array in header. Or even add some registerMethods() and have this as implementation detail (and also support inheritance?) -- reconsider approach to mutable methods (like BattleHexArrayProxy). Either remove or provide better API bindings approach for such cases +- reconsider approach to mutable methods (like BattleHexArrayProxy). Either remove or provide better API bindings approach for such cases. Or convert it to pure Lua class - consider removing excessive namespace from scripting API, have all API classes directly in scripting::api namespace -- remove getSpellByKey from Mechanics - 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 - implement comparison operator of exposed API classes by auto-implementing `__eq` Lua field for all exported classes - consider wrapping Lua userdata into std::any for better type safety @@ -40,6 +38,11 @@ This page describes the internal working of the Lua scripting module. For usage - `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 + +- Spell Effect: Add "preprocess" or "initialize" function to initialize parameters (e.g. load string ID and resolve it to Creature type). Would require some way to store references to Lua table in different LuaContext's in LuaSpellEffect class, for example - shared_ptr in LuaContext, and weak_ptr in LuaSpellEffect. + + ## General rules Scripts must be constant and should not generate any side effects. diff --git a/luascript/api/callback/ServerCallback.cpp b/luascript/api/callback/ServerCallback.cpp index ab52defd1..4f3ee24d9 100644 --- a/luascript/api/callback/ServerCallback.cpp +++ b/luascript/api/callback/ServerCallback.cpp @@ -251,7 +251,7 @@ int ServerCallbackProxy::rngInt(lua_State * L) int low = 0; int high = 0; - S.get(1, object); + S.getNonNull(1, object); S.get(2, low); S.get(3, high); @@ -275,7 +275,7 @@ void ServerCallbackProxy::moveUnit(ServerCallback * object, BattleID battleID, c object->apply(pack); } -void ServerCallbackProxy::appendLog(ServerCallback * object, BattleID battleID, JsonNode config) +void ServerCallbackProxy::appendLog(ServerCallback * object, BattleID battleID, const JsonNode & config) { BattleLogMessage msg; msg.battleID = battleID; diff --git a/luascript/api/callback/ServerCallback.h b/luascript/api/callback/ServerCallback.h index a5b4f2271..bb4a77143 100644 --- a/luascript/api/callback/ServerCallback.h +++ b/luascript/api/callback/ServerCallback.h @@ -40,7 +40,7 @@ public: static void removeUnit(ServerCallback * object, BattleID battleID, const battle::Unit * unit); static void removeObstacle(ServerCallback * object, BattleID battleID, std::shared_ptr obstacle); static void moveUnit(ServerCallback * object, BattleID battleID, const battle::Unit * unit, BattleHex destination, bool isTeleport); - static void appendLog(ServerCallback * object, BattleID battleID, JsonNode config); + static void appendLog(ServerCallback * object, BattleID battleID, const JsonNode & config); static bool describeChanges(ServerCallback * object); static void removeUnitBonuses(ServerCallback * object, BattleID battleID, const battle::Unit * unit, const BonusList & bonusList); static void addUnitBonus(ServerCallback * object, BattleID battleID, uint32_t unitId, const JsonNode & data); diff --git a/luascript/api/spells/Mechanics.cpp b/luascript/api/spells/Mechanics.cpp index d70e7ea9e..bd5df02dd 100644 --- a/luascript/api/spells/Mechanics.cpp +++ b/luascript/api/spells/Mechanics.cpp @@ -20,7 +20,6 @@ #include "../../../lib/battle/Unit.h" #include "../../../lib/spells/Problem.h" #include "../../../lib/mapObjects/CGHeroInstance.h" -#include "../../../lib/constants/EntityIdentifiers.h" #include "../../../lib/GameLibrary.h" #include "../../../lib/texts/CGeneralTextHandler.h" #include "../../../lib/texts/Languages.h" @@ -38,11 +37,6 @@ namespace scripting::api return m->ownerMatches(unit); } - const spells::Spell * MechanicsProxy::getSpellByKey(const spells::Mechanics * m, const std::string & key) - { - return m->spells()->getByIndex(SpellID::decode(key)); - } - std::string MechanicsProxy::getPluralFormTextID(const spells::Mechanics * m, const std::string & baseTextID, int32_t count) { std::string lang = LIBRARY->generaltexth->getPreferredLanguage(); @@ -76,7 +70,6 @@ namespace scripting::api {"isReceptive", LuaMethodWrapper<&Mechanics::isReceptive>::invoke, false}, {"ownerMatches", LuaFunctionWrapper<&ownerMatchesUnit>::invoke, false}, {"getSpell", LuaMethodWrapper<&Mechanics::getSpell>::invoke, false}, - {"getSpellByKey", LuaFunctionWrapper<&MechanicsProxy::getSpellByKey>::invoke, false}, {"adjustEffectValue", LuaMethodWrapper<&Mechanics::adjustEffectValue>::invoke, false}, {"getPluralFormTextID", LuaFunctionWrapper<&MechanicsProxy::getPluralFormTextID>::invoke, false}, }; diff --git a/luascript/api/spells/Mechanics.h b/luascript/api/spells/Mechanics.h index 263cca820..c58c4d29a 100644 --- a/luascript/api/spells/Mechanics.h +++ b/luascript/api/spells/Mechanics.h @@ -25,7 +25,6 @@ namespace scripting::api static const std::vector REGISTER_CUSTOM; static bool ownerMatchesUnit(const ::spells::Mechanics * m, const battle::Unit * unit); - static const ::spells::Spell * getSpellByKey(const ::spells::Mechanics * m, const std::string & key); static std::string getPluralFormTextID(const ::spells::Mechanics * m, const std::string & baseTextID, int32_t count); }; } diff --git a/scripts/dispel.lua b/scripts/dispel.lua index 0b7c990c4..22b665c36 100644 --- a/scripts/dispel.lua +++ b/scripts/dispel.lua @@ -7,7 +7,7 @@ function Script:getDispelableBonuses(mechanics, unit) return unit:getBonuses(function(bonus) if bonus:getSource() ~= ENUM.BonusSource.spellEffect then return false end if bonus:getSourceID() == currentSpellKey then return false end - local sourceSpell = mechanics:getSpellByKey(bonus:getSourceID()) + local sourceSpell = LIBRARY:getSpellByName(bonus:getSourceID()) if not sourceSpell then return false end if sourceSpell:isPersistent() then return false end if sourceSpell:isAdventure() then return false end diff --git a/scripts/spellEffect.lua b/scripts/spellEffect.lua index e35b7fe03..7a47f52bd 100644 --- a/scripts/spellEffect.lua +++ b/scripts/spellEffect.lua @@ -2,13 +2,6 @@ local Script = {} Script.__index = Script Script.type = "spellEffect" --- TODO --- initializes parameters of the script using spell effect json --- returns converted parameters that contain resolved identifiers -function Script:initialize() - return self -end - --- Returns true if specified target can be affected by the spell --- if target can not be affected, script needs to call `problem:add` --- to explain the reason to the player diff --git a/scripts/summon.lua b/scripts/summon.lua index 32c1f61ab..23d401dc8 100644 --- a/scripts/summon.lua +++ b/scripts/summon.lua @@ -30,14 +30,6 @@ function Script:summonedCreatureAmount(mechanics) end end --- TODO --- initializes parameters of the script using spell effect json --- returns converted parameters that contain resolved identifiers -function Script:initialize() - self.creature = LIBRARY:getCreatureByName(self.id) - return self -end - --- Returns true if spell can be casted in general --- if no valid targets exist, script needs to call `problem:add` --- to explain the reason to the player diff --git a/test/spells/effects/CatapultTest.cpp b/test/spells/effects/CatapultTest.cpp index c6ceb39f7..f48e3ae22 100644 --- a/test/spells/effects/CatapultTest.cpp +++ b/test/spells/effects/CatapultTest.cpp @@ -142,7 +142,7 @@ TEST_F(CatapultApplyTest, DamageToIntactPart) CatapultAttack capturedPack; EXPECT_CALL(serverMock, apply(Matcher(_))) - .WillOnce(Invoke([&](CatapultAttack & pack) + .WillOnce(Invoke([this, &capturedPack](CatapultAttack & pack) { capturedPack = pack; BattleStatePackVisitor visitor(*battleFake); @@ -184,7 +184,7 @@ TEST_F(CatapultApplyTest, TargetedHitOnSpecifiedPart) CatapultAttack capturedPack; EXPECT_CALL(serverMock, apply(Matcher(_))) - .WillOnce(Invoke([&](CatapultAttack & pack) + .WillOnce(Invoke([this, &capturedPack](CatapultAttack & pack) { capturedPack = pack; BattleStatePackVisitor visitor(*battleFake); @@ -229,7 +229,7 @@ TEST_F(CatapultApplyTest, TargetedMissRedirectsToPotentialTarget) CatapultAttack capturedPack; EXPECT_CALL(serverMock, apply(Matcher(_))) - .WillOnce(Invoke([&](CatapultAttack & pack) + .WillOnce(Invoke([this, &capturedPack](CatapultAttack & pack) { capturedPack = pack; BattleStatePackVisitor visitor(*battleFake); @@ -277,7 +277,7 @@ TEST_F(CatapultApplyTest, RemovesTowerShooterOnKeepDestroyed) CatapultAttack capturedPack; EXPECT_CALL(serverMock, apply(Matcher(_))) - .WillOnce(Invoke([&](CatapultAttack & pack) + .WillOnce(Invoke([this, &capturedPack](CatapultAttack & pack) { capturedPack = pack; BattleStatePackVisitor visitor(*battleFake); @@ -316,7 +316,7 @@ TEST_F(CatapultApplyTest, MassiveAttacksMultipleParts) std::vector capturedPacks; EXPECT_CALL(serverMock, apply(Matcher(_))) - .WillRepeatedly(Invoke([&](CatapultAttack & pack) + .WillRepeatedly(Invoke([this, &capturedPacks](CatapultAttack & pack) { capturedPacks.push_back(pack); BattleStatePackVisitor visitor(*battleFake); diff --git a/test/spells/effects/DispelTest.cpp b/test/spells/effects/DispelTest.cpp index 63eb88926..c28fd7ef3 100644 --- a/test/spells/effects/DispelTest.cpp +++ b/test/spells/effects/DispelTest.cpp @@ -42,36 +42,34 @@ public: } // Called by every test that has at least one SPELL_EFFECT bonus on a unit, - // so the Lua filter's mechanics:getSpellByKey() calls are satisfied. + // so the Lua filter's LIBRARY:getSpellByName() calls are satisfied. void setDefaultExpectations() { - EXPECT_CALL(mechanicsMock, spells()).Times(AnyNumber()); - - EXPECT_CALL(spellServiceMock, getByIndex(Eq(positiveID.getNum()))).WillRepeatedly(Return(&positiveSpell)); + EXPECT_CALL(spellServiceMock, getByName(Eq(SpellID::encode(positiveID.getNum())))).WillRepeatedly(Return(&positiveSpell)); EXPECT_CALL(positiveSpell, isPersistent()).WillRepeatedly(Return(false)); EXPECT_CALL(positiveSpell, isAdventure()).WillRepeatedly(Return(false)); EXPECT_CALL(positiveSpell, isPositive()).WillRepeatedly(Return(true)); EXPECT_CALL(positiveSpell, isNegative()).WillRepeatedly(Return(false)); EXPECT_CALL(positiveSpell, isNeutral()).WillRepeatedly(Return(false)); - EXPECT_CALL(spellServiceMock, getByIndex(Eq(negativeID.getNum()))).WillRepeatedly(Return(&negativeSpell)); + EXPECT_CALL(spellServiceMock, getByName(Eq(SpellID::encode(negativeID.getNum())))).WillRepeatedly(Return(&negativeSpell)); EXPECT_CALL(negativeSpell, isPersistent()).WillRepeatedly(Return(false)); EXPECT_CALL(negativeSpell, isAdventure()).WillRepeatedly(Return(false)); EXPECT_CALL(negativeSpell, isPositive()).WillRepeatedly(Return(false)); EXPECT_CALL(negativeSpell, isNegative()).WillRepeatedly(Return(true)); EXPECT_CALL(negativeSpell, isNeutral()).WillRepeatedly(Return(false)); - EXPECT_CALL(spellServiceMock, getByIndex(Eq(neutralID.getNum()))).WillRepeatedly(Return(&neutralSpell)); + EXPECT_CALL(spellServiceMock, getByName(Eq(SpellID::encode(neutralID.getNum())))).WillRepeatedly(Return(&neutralSpell)); EXPECT_CALL(neutralSpell, isPersistent()).WillRepeatedly(Return(false)); EXPECT_CALL(neutralSpell, isAdventure()).WillRepeatedly(Return(false)); EXPECT_CALL(neutralSpell, isPositive()).WillRepeatedly(Return(false)); EXPECT_CALL(neutralSpell, isNegative()).WillRepeatedly(Return(false)); EXPECT_CALL(neutralSpell, isNeutral()).WillRepeatedly(Return(true)); - EXPECT_CALL(spellServiceMock, getByIndex(Eq(persistentID.getNum()))).WillRepeatedly(Return(&persistentSpell)); + EXPECT_CALL(spellServiceMock, getByName(Eq(SpellID::encode(persistentID.getNum())))).WillRepeatedly(Return(&persistentSpell)); EXPECT_CALL(persistentSpell, isPersistent()).WillRepeatedly(Return(true)); - EXPECT_CALL(spellServiceMock, getByIndex(Eq(adventureID.getNum()))).WillRepeatedly(Return(&adventureSpell)); + EXPECT_CALL(spellServiceMock, getByName(Eq(SpellID::encode(adventureID.getNum())))).WillRepeatedly(Return(&adventureSpell)); EXPECT_CALL(adventureSpell, isPersistent()).WillRepeatedly(Return(false)); EXPECT_CALL(adventureSpell, isAdventure()).WillRepeatedly(Return(true)); } diff --git a/test/spells/effects/EffectFixture.cpp b/test/spells/effects/EffectFixture.cpp index 8eb866a8a..ad8e55dee 100644 --- a/test/spells/effects/EffectFixture.cpp +++ b/test/spells/effects/EffectFixture.cpp @@ -91,6 +91,7 @@ void EffectFixture::setUp() ON_CALL(*battleFake, getUnitsIf(_)).WillByDefault(Invoke(&unitsFake, &battle::UnitsFake::getUnitsIf)); ON_CALL(mechanicsMock, spells()).WillByDefault(Return(&spellServiceMock)); + EXPECT_CALL(servicesMock, spells()).WillRepeatedly(Return(&spellServiceMock)); ON_CALL(spellServiceMock, getById(_)).WillByDefault(Return(&spellStub)); ON_CALL(serverMock, getRNG()).WillByDefault(Return(&rngMock)); diff --git a/test/spells/effects/MoatTest.cpp b/test/spells/effects/MoatTest.cpp index 1f0937eb5..1b4e8aedc 100644 --- a/test/spells/effects/MoatTest.cpp +++ b/test/spells/effects/MoatTest.cpp @@ -60,7 +60,7 @@ public: { EXPECT_CALL(serverMock, apply(Matcher(_))) .Times(AnyNumber()) - .WillRepeatedly(Invoke([this](BattleObstaclesChanged & pack) + .WillRepeatedly(Invoke([this](const BattleObstaclesChanged & pack) { for(const auto & change : pack.changes) capturedPack.changes.push_back(change); diff --git a/test/spells/effects/ObstacleTest.cpp b/test/spells/effects/ObstacleTest.cpp index 78c2b9c6c..990c734e1 100644 --- a/test/spells/effects/ObstacleTest.cpp +++ b/test/spells/effects/ObstacleTest.cpp @@ -152,7 +152,7 @@ public: { EXPECT_CALL(serverMock, apply(Matcher(_))) .Times(AnyNumber()) - .WillRepeatedly(Invoke([this](BattleObstaclesChanged & pack) + .WillRepeatedly(Invoke([this](const BattleObstaclesChanged & pack) { for(const auto & change : pack.changes) capturedPack.changes.push_back(change); diff --git a/test/spells/effects/RemoveObstacleTest.cpp b/test/spells/effects/RemoveObstacleTest.cpp index cc052efc9..53e5df466 100644 --- a/test/spells/effects/RemoveObstacleTest.cpp +++ b/test/spells/effects/RemoveObstacleTest.cpp @@ -187,7 +187,7 @@ public: void captureObstaclePack() { EXPECT_CALL(serverMock, apply(Matcher(_))) - .WillRepeatedly(Invoke([this](BattleObstaclesChanged & pack) + .WillRepeatedly(Invoke([this](const BattleObstaclesChanged & pack) { for(const auto & change : pack.changes) capturedPack.changes.push_back(change);