games/vcmi
1
0
Fork 0
mirror of https://github.com/vcmi/vcmi.git synced 2026-06-19 22:57:37 +02:00
Code Issues Releases Activity

Implemented several TODO items

Browse Source 
- Dropped incomplete attempt to add initialize() function
- Removed effectively duplicated getSpellByKey() method from API
- Fixed several Sonar warnings
This commit is contained in:
Ivan Savenko
2026-06-03 23:13:17 +03:00
parent f6c5bbcf3b
commit 94a586b172
14 changed files with 25 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/developers/Lua_Scripting_System.md
+6 -3
View File
@@ -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<LuaReference> in LuaContext, and weak_ptr<LuaReference> in LuaSpellEffect.
## General rules
Scripts must be constant and should not generate any side effects.
luascript/api/callback/ServerCallback.cpp
+2 -2
View File
@@ -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;
luascript/api/callback/ServerCallback.h
+1 -1
View File
@@ -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<const CObstacleInstance> 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);
luascript/api/spells/Mechanics.cpp
-7
View File
@@ -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},
};
luascript/api/spells/Mechanics.h
-1
View File
@@ -25,7 +25,6 @@ namespace scripting::api
static const std::vector<typename Wrapper::CustomRegType> 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);
};
}
scripts/dispel.lua
+1 -1
View File
@@ -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
scripts/spellEffect.lua
-7
View File
@@ -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
scripts/summon.lua
-8
View File
@@ -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
test/spells/effects/CatapultTest.cpp
+5 -5
View File
@@ -142,7 +142,7 @@ TEST_F(CatapultApplyTest, DamageToIntactPart)
CatapultAttack capturedPack;
EXPECT_CALL(serverMock, apply(Matcher<CatapultAttack &>(_)))
.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<CatapultAttack &>(_)))
.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<CatapultAttack &>(_)))
.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<CatapultAttack &>(_)))
.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<CatapultAttack> capturedPacks;
EXPECT_CALL(serverMock, apply(Matcher<CatapultAttack &>(_)))
.WillRepeatedly(Invoke([&](CatapultAttack & pack)
.WillRepeatedly(Invoke([this, &capturedPacks](CatapultAttack & pack)
{
capturedPacks.push_back(pack);
BattleStatePackVisitor visitor(*battleFake);
test/spells/effects/DispelTest.cpp
+6 -8
View File
@@ -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));
}
test/spells/effects/EffectFixture.cpp
+1
View File
@@ -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));
test/spells/effects/MoatTest.cpp
+1 -1
View File
@@ -60,7 +60,7 @@ public:
{
EXPECT_CALL(serverMock, apply(Matcher<BattleObstaclesChanged &>(_)))
.Times(AnyNumber())
.WillRepeatedly(Invoke([this](BattleObstaclesChanged & pack)
.WillRepeatedly(Invoke([this](const BattleObstaclesChanged & pack)
{
for(const auto & change : pack.changes)
capturedPack.changes.push_back(change);
test/spells/effects/ObstacleTest.cpp
+1 -1
View File
@@ -152,7 +152,7 @@ public:
{
EXPECT_CALL(serverMock, apply(Matcher<BattleObstaclesChanged &>(_)))
.Times(AnyNumber())
.WillRepeatedly(Invoke([this](BattleObstaclesChanged & pack)
.WillRepeatedly(Invoke([this](const BattleObstaclesChanged & pack)
{
for(const auto & change : pack.changes)
capturedPack.changes.push_back(change);
test/spells/effects/RemoveObstacleTest.cpp
+1 -1
View File
@@ -187,7 +187,7 @@ public:
void captureObstaclePack()
{
EXPECT_CALL(serverMock, apply(Matcher<BattleObstaclesChanged &>(_)))
.WillRepeatedly(Invoke([this](BattleObstaclesChanged & pack)
.WillRepeatedly(Invoke([this](const BattleObstaclesChanged & pack)
{
for(const auto & change : pack.changes)
capturedPack.changes.push_back(change);