vcmi: unify movement

1. Now there is only one bonus: MOVEMENT, with 2 subtypes: 0 is sea, 1 is land
   For movement value on land depends on creature speed we use a new
   ARMY_MOVEMENT updater with global bonus. If we does not like such
   dependency, we can just remove this updater from json.
2. All specialities and secondary skills for movement moved to new
   system AFAIK

AI/Nullkiller/Engine/PriorityEvaluator.cpp

@@ -209,7 +209,7 @@ uint64_t evaluateArtifactArmyValue(CArtifactInstance * art)
 		return 1500;
 
 	auto statsValue =
-		10 * art->valOfBonuses(Bonus::LAND_MOVEMENT)
+		10 * art->valOfBonuses(Bonus::MOVEMENT, 1)
 		+ 1200 * art->valOfBonuses(Bonus::STACKS_SPEED)
 		+ 700 * art->valOfBonuses(Bonus::MORALE)
 		+ 700 * art->getAttack(false)

config/artifacts.json

@@ -1074,9 +1074,10 @@
 	{
 		"bonuses" : [
 			{
-				"type" : "LAND_MOVEMENT",
+				"type" : "MOVEMENT",
+				"subtype" : 1,
 				"val" : 300,
-				"valueType" : "BASE_NUMBER"
+				"valueType" : "ADDITIVE_VALUE"
 			}
 		],
 		"index" : 70,
@@ -1086,9 +1087,10 @@
 	{
 		"bonuses" : [
 			{
-				"type" : "SEA_MOVEMENT",
+				"type" : "MOVEMENT",
+				"subtype" : 0,
 				"val" : 1000,
-				"valueType" : "BASE_NUMBER"
+				"valueType" : "ADDITIVE_VALUE"
 			}
 		],
 		"index" : 71,
@@ -1430,9 +1432,10 @@
 	{
 		"bonuses" : [
 			{
-				"type" : "LAND_MOVEMENT",
+				"type" : "MOVEMENT",
+				"subtype" : 1,
 				"val" : 600,
-				"valueType" : "BASE_NUMBER"
+				"valueType" : "ADDITIVE_VALUE"
 			}
 		],
 		"index" : 98,
@@ -1778,9 +1781,10 @@
 				"valueType" : "BASE_NUMBER"
 			},
 			{
-				"type" : "SEA_MOVEMENT",
+				"type" : "MOVEMENT",
+				"subtype" : 0,
 				"val" : 500,
-				"valueType" : "BASE_NUMBER"
+				"valueType" : "ADDITIVE_VALUE"
 			},
 			{
 				"subtype" : "spell.summonBoat",

config/defaultMods.json

@@ -78,6 +78,25 @@
 			"type" : "MANA_PER_KNOWLEDGE", //10 knowledge to 100 mana is default
 			"val" : 10,
 			"valueType" : "BASE_NUMBER"
+		},
+		{
+			"type" : "MOVEMENT", //Basic land movement
+			"subtype" : 1,
+			"val" : 1300,
+			"valueType" : "BASE_NUMBER"
+		},
+		{
+			"type" : "MOVEMENT", //Enable army movement bonus
+			"subtype" : 1,
+			"val" : 0,
+			"valueType" : "BASE_NUMBER",
+			"updater" : "ARMY_MOVEMENT"
+		},
+		{
+			"type" : "MOVEMENT", //Basic sea movement
+			"subtype" : 0,
+			"val" : 1500,
+			"valueType" : "BASE_NUMBER"
 		}
 	]
 }

config/heroes/castle.json

@@ -63,11 +63,12 @@
 		"specialty" : {
 			"bonuses" : {
 				"navigation" : {
-					"subtype" : "skill.navigation",
-					"type" : "SECONDARY_SKILL_PREMY",
+					"targetSourceType" : "SECONDARY_SKILL",
+					"subtype" : 0,
+					"type" : "MOVEMENT",
 					"updater" : "TIMES_HERO_LEVEL",
 					"val" : 5,
-					"valueType" : "PERCENT_TO_BASE"
+					"valueType" : "PERCENT_TO_TARGET_TYPE"
 				}
 			}
 		}

config/heroes/dungeon.json

@@ -87,11 +87,12 @@
 		"specialty" : {
 			"bonuses" : {
 				"logistics" : {
-					"subtype" : "skill.logistics",
-					"type" : "SECONDARY_SKILL_PREMY",
+					"targetSourceType" : "SECONDARY_SKILL",
+					"subtype" : 1,
+					"type" : "MOVEMENT",
 					"updater" : "TIMES_HERO_LEVEL",
 					"val" : 5,
-					"valueType" : "PERCENT_TO_BASE"
+					"valueType" : "PERCENT_TO_TARGET_TYPE"
 				}
 			}
 		}

config/heroes/fortress.json

@@ -191,11 +191,12 @@
 		"specialty" : {
 			"bonuses" : {
 				"navigation" : {
-					"subtype" : "skill.navigation",
-					"type" : "SECONDARY_SKILL_PREMY",
+					"targetSourceType" : "SECONDARY_SKILL",
+					"subtype" : 0,
+					"type" : "MOVEMENT",
 					"updater" : "TIMES_HERO_LEVEL",
 					"val" : 5,
-					"valueType" : "PERCENT_TO_BASE"
+					"valueType" : "PERCENT_TO_TARGET_TYPE"
 				}
 			}
 		}

config/heroes/rampart.json

@@ -131,11 +131,12 @@
 		"specialty" : {
 			"bonuses" : {
 				"logistics" : {
-					"subtype" : "skill.logistics",
-					"type" : "SECONDARY_SKILL_PREMY",
+					"targetSourceType" : "SECONDARY_SKILL",
+					"subtype" : 1,
+					"type" : "MOVEMENT",
 					"updater" : "TIMES_HERO_LEVEL",
 					"val" : 5,
-					"valueType" : "PERCENT_TO_BASE"
+					"valueType" : "PERCENT_TO_TARGET_TYPE"
 				}
 			}
 		}

config/heroes/stronghold.json

@@ -171,11 +171,12 @@
 		"specialty" : {
 			"bonuses" : {
 				"logistics" : {
-					"subtype" : "skill.logistics",
-					"type" : "SECONDARY_SKILL_PREMY",
+					"targetSourceType" : "SECONDARY_SKILL",
+					"subtype" : 1,
+					"type" : "MOVEMENT",
 					"updater" : "TIMES_HERO_LEVEL",
 					"val" : 5,
-					"valueType" : "PERCENT_TO_BASE"
+					"valueType" : "PERCENT_TO_TARGET_TYPE"
 				}
 			}
 		}

config/objects/rewardableBonusing.json

@@ -338,7 +338,7 @@
 						},
 						"message" : 138,
 						"movePoints" : 400,
-						"bonuses" : [ { "type" : "LAND_MOVEMENT", "val" : 400, "duration" : "ONE_WEEK"} ],
+						"bonuses" : [ { "type" : "MOVEMENT", "subtype" : 1,  "val" : 400, "valueType" : "ADDITIVE_VALUE", "duration" : "ONE_WEEK"} ],
 						"changeCreatures" : {
 							"cavalier" : "champion"
 						}
@@ -346,7 +346,7 @@
 					{
 						"message" : 137,
 						"movePoints" : 400,
-						"bonuses" : [ { "type" : "LAND_MOVEMENT", "val" : 400, "duration" : "ONE_WEEK"} ]
+						"bonuses" : [ { "type" : "MOVEMENT", "subtype" : 1,  "val" : 400, "valueType" : "ADDITIVE_VALUE", "duration" : "ONE_WEEK"} ]
 					}
 				]
 			}

config/skills.json

@@ -57,9 +57,9 @@
 		"base" : {
 			"effects" : {
 				"main" : {
-					"subtype" : "skill.logistics",
-					"type" : "SECONDARY_SKILL_PREMY",
-					"valueType" : "BASE_NUMBER"
+					"subtype" : 1,
+					"type" : "MOVEMENT",
+					"valueType" : "PERCENT_TO_BASE"
 				}
 			}
 		},
@@ -144,9 +144,9 @@
 		"base" : {
 			"effects" : {
 				"main" : {
-					"subtype" : "skill.navigation",
-					"type" : "SECONDARY_SKILL_PREMY",
-					"valueType" : "BASE_NUMBER"
+					"subtype" : 0,
+					"type" : "MOVEMENT",
+					"valueType" : "PERCENT_TO_BASE"
 				}
 			}
 		},

lib/CPathfinder.cpp

@@ -1085,6 +1085,29 @@ int TurnInfo::getMaxMovePoints(const EPathfindingLayer layer) const
 	return layer == EPathfindingLayer::SAIL ? maxMovePointsWater : maxMovePointsLand;
 }
 
+void TurnInfo::updateHeroBonuses(Bonus::BonusType type, const CSelector& sel) const
+{
+	switch(type)
+	{
+	case Bonus::FREE_SHIP_BOARDING:
+		bonusCache->freeShipBoarding = static_cast<bool>(bonuses->getFirst(Selector::type()(Bonus::FREE_SHIP_BOARDING)));
+		break;
+	case Bonus::FLYING_MOVEMENT:
+		bonusCache->flyingMovement = static_cast<bool>(bonuses->getFirst(Selector::type()(Bonus::FLYING_MOVEMENT)));
+		bonusCache->flyingMovementVal = bonuses->valOfBonuses(Selector::type()(Bonus::FLYING_MOVEMENT));
+		break;
+	case Bonus::WATER_WALKING:
+		bonusCache->waterWalking = static_cast<bool>(bonuses->getFirst(Selector::type()(Bonus::WATER_WALKING)));
+		bonusCache->waterWalkingVal = bonuses->valOfBonuses(Selector::type()(Bonus::WATER_WALKING));
+		break;
+	case Bonus::ROUGH_TERRAIN_DISCOUNT:
+		bonusCache->pathfindingVal = bonuses->valOfBonuses(Selector::type()(Bonus::ROUGH_TERRAIN_DISCOUNT));
+		break;
+	default:
+		bonuses = hero->getUpdatedBonusList(*bonuses, Selector::type()(type).And(sel));
+	}
+}
+
 CPathfinderHelper::CPathfinderHelper(CGameState * gs, const CGHeroInstance * Hero, const PathfinderOptions & Options)
 	: CGameInfoCallback(gs, boost::optional<PlayerColor>()), turn(-1), hero(Hero), options(Options), owner(Hero->tempOwner)
 {

lib/CPathfinder.h

@@ -525,7 +525,7 @@ struct DLL_LINKAGE TurnInfo
 	std::unique_ptr<BonusCache> bonusCache;
 
 	const CGHeroInstance * hero;
-	TConstBonusListPtr bonuses;
+	mutable TConstBonusListPtr bonuses;
 	mutable int maxMovePointsLand;
 	mutable int maxMovePointsWater;
 	TerrainId nativeTerrain;
@@ -534,6 +534,7 @@ struct DLL_LINKAGE TurnInfo
 	bool isLayerAvailable(const EPathfindingLayer layer) const;
 	bool hasBonusOfType(const Bonus::BonusType type, const int subtype = -1) const;
 	int valOfBonuses(const Bonus::BonusType type, const int subtype = -1) const;
+	void updateHeroBonuses(Bonus::BonusType type, const CSelector& sel) const;
 	int getMaxMovePoints(const EPathfindingLayer layer) const;
 };
 

lib/CTownHandler.cpp

@@ -521,7 +521,7 @@ void CTownHandler::addBonusesForVanilaBuilding(CBuilding * building)
 			b = createBonus(building, Bonus::PRIMARY_SKILL, +2, PrimarySkill::DEFENSE);
 			break;
 		case BuildingSubID::LIGHTHOUSE:
-			b = createBonus(building, Bonus::SEA_MOVEMENT, +500, playerPropagator);
+			b = createBonus(building, Bonus::MOVEMENT, +500, playerPropagator, 0);
 			break;
 		}
 	}

lib/HeroBonus.cpp

@@ -95,7 +95,8 @@ const std::map<std::string, TPropagatorPtr> bonusPropagatorMap =
 const std::map<std::string, TUpdaterPtr> bonusUpdaterMap =
 {
 	{"TIMES_HERO_LEVEL", std::make_shared<TimesHeroLevelUpdater>()},
-	{"TIMES_STACK_LEVEL", std::make_shared<TimesStackLevelUpdater>()}
+	{"TIMES_STACK_LEVEL", std::make_shared<TimesStackLevelUpdater>()},
+	{"ARMY_MOVEMENT", std::make_shared<ArmyMovementUpdater>()}
 };
 
 ///CBonusProxy
@@ -565,11 +566,6 @@ std::shared_ptr<const Bonus> BonusList::getFirst(const CSelector &selector) cons
 	return nullptr;
 }
 
-void BonusList::getBonuses(BonusList & out, const CSelector &selector) const
-{
-	getBonuses(out, selector, nullptr);
-}
-
 void BonusList::getBonuses(BonusList & out, const CSelector &selector, const CSelector &limit) const
 {
 	out.reserve(bonuses.size());
@@ -1102,6 +1098,19 @@ std::shared_ptr<Bonus> CBonusSystemNode::getUpdatedBonus(const std::shared_ptr<B
 	return updater->createUpdatedBonus(b, * this);
 }
 
+TConstBonusListPtr CBonusSystemNode::getUpdatedBonusList(const BonusList & out, const CSelector & sel) const
+{
+	auto ret = std::make_shared<BonusList>();
+	for(const auto & b : out)
+	{
+		if(sel(b.get()) && b->updater)
+			ret->push_back(getUpdatedBonus(b, b->updater));
+		else
+			ret->push_back(b);
+	}
+	return ret;
+}
+
 CBonusSystemNode::CBonusSystemNode()
 	:CBonusSystemNode(false)
 {
@@ -2553,6 +2562,30 @@ JsonNode TimesHeroLevelUpdater::toJsonNode() const
 	return JsonUtils::stringNode("TIMES_HERO_LEVEL");
 }
 
+std::shared_ptr<Bonus> ArmyMovementUpdater::createUpdatedBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const
+{
+	if(b->type == Bonus::MOVEMENT && context.getNodeType() == CBonusSystemNode::HERO)
+	{
+		auto newBonus = std::make_shared<Bonus>(*b);
+		newBonus->source = Bonus::ARMY;
+		newBonus->val = static_cast<const CGHeroInstance &>(context).getArmyMovementBonus();
+		return newBonus;
+	}
+	if(b->type != Bonus::MOVEMENT)
+		logGlobal->error("ArmyMovementUpdater should only be used for MOVEMENT bonus!");
+	return b;
+}
+
+std::string ArmyMovementUpdater::toString() const
+{
+	return "ArmyMovementUpdater";
+}
+
+JsonNode ArmyMovementUpdater::toJsonNode() const
+{
+	return JsonUtils::stringNode("ARMY_MOVEMENT");
+}
+
 TimesStackLevelUpdater::TimesStackLevelUpdater()
 {
 }

lib/HeroBonus.h

@@ -171,9 +171,7 @@ public:
 #define BONUS_LIST										\
 	BONUS_NAME(NONE) 									\
 	BONUS_NAME(LEVEL_COUNTER) /* for commander artifacts*/ \
-	BONUS_NAME(MOVEMENT) /*both water/land*/			\
-	BONUS_NAME(LAND_MOVEMENT) \
-	BONUS_NAME(SEA_MOVEMENT) \
+	BONUS_NAME(MOVEMENT) /*Subtype is 1 - land, 0 - sea*/ \
 	BONUS_NAME(MORALE) \
 	BONUS_NAME(LUCK) \
 	BONUS_NAME(PRIMARY_SKILL) /*uses subtype to pick skill; additional info if set: 1 - only melee, 2 - only distance*/  \
@@ -590,11 +588,9 @@ public:
 	// BonusList functions
 	void stackBonuses();
 	int totalValue() const;
-	void getBonuses(BonusList &out, const CSelector &selector, const CSelector &limit) const;
+	void getBonuses(BonusList &out, const CSelector &selector, const CSelector &limit = nullptr) const;
 	void getAllBonuses(BonusList &out) const;
 
-	void getBonuses(BonusList & out, const CSelector &selector) const;
-
 	//special find functions
 	std::shared_ptr<Bonus> getFirst(const CSelector &select);
 	std::shared_ptr<const Bonus> getFirst(const CSelector &select) const;
@@ -801,6 +797,7 @@ public:
 	TConstBonusListPtr getAllBonuses(const CSelector &selector, const CSelector &limit, const CBonusSystemNode *root = nullptr, const std::string &cachingStr = "") const override;
 	void getParents(TCNodes &out) const;  //retrieves list of parent nodes (nodes to inherit bonuses from),
 	std::shared_ptr<const Bonus> getBonusLocalFirst(const CSelector & selector) const;
+	TConstBonusListPtr getUpdatedBonusList(const BonusList& out, const CSelector &sel) const; //update bonuses in list with builtin updaters, passes this as context
 
 	//non-const interface
 	void getParents(TNodes &out);  //retrieves list of parent nodes (nodes to inherit bonuses from)
@@ -1311,6 +1308,19 @@ public:
 	virtual JsonNode toJsonNode() const override;
 };
 
+class DLL_LINKAGE ArmyMovementUpdater : public IUpdater
+{
+public:
+	template <typename Handler> void serialize(Handler & h, const int version)
+	{
+		h & static_cast<IUpdater &>(*this);
+	}
+
+	std::shared_ptr<Bonus> createUpdatedBonus(const std::shared_ptr<Bonus> & b, const CBonusSystemNode & context) const override;
+	virtual std::string toString() const override;
+	virtual JsonNode toJsonNode() const override;
+};
+
 class DLL_LINKAGE OwnerUpdater : public IUpdater
 {
 public:

lib/mapObjects/CGHeroInstance.cpp

@@ -188,32 +188,27 @@ int CGHeroInstance::maxMovePoints(bool onLand) const
 	return maxMovePointsCached(onLand, &ti);
 }
 
-int CGHeroInstance::maxMovePointsCached(bool onLand, const TurnInfo * ti) const
+int CGHeroInstance::getArmyMovementBonus() const
 {
-	int base = 0;
-
-	if(onLand)
-	{
-		// used function is f(x) = 66.6x + 1300, rounded to second digit, where x is lowest speed in army
-		static constexpr int baseSpeed = 1300; // base speed from creature with 0 speed
+	return armyMovementVal;
+}
 
-		int armySpeed = lowestSpeed(this) * 20 / 3;
+void CGHeroInstance::updateArmyMovementBonus(bool onLand, const TurnInfo * ti) const
+{
+	int armySpeed = lowestSpeed(this) * 20 / 3;
 
-		base = armySpeed * 10 + baseSpeed; // separate *10 is intentional to receive same rounding as in h3
-		vstd::abetween(base, 1500, 2000); // base speed is limited by these values
-	}
-	else
+	auto base = armySpeed * 10; // separate *10 is intentional to receive same rounding as in h3
+	if(armyMovementVal != vstd::abetween(base, 200, 700)) // army modifier speed is limited by these values
 	{
-		base = 1500; //on water base movement is always 1500 (speed of army doesn't matter)
+		armyMovementVal = base;
+		ti->updateHeroBonuses(Bonus::MOVEMENT, Selector::subtype()(!!onLand).And(Selector::sourceTypeSel(Bonus::ARMY)));
 	}
+}
 
-	const Bonus::BonusType bt = onLand ? Bonus::LAND_MOVEMENT : Bonus::SEA_MOVEMENT;
-	const int bonus = ti->valOfBonuses(Bonus::MOVEMENT) + ti->valOfBonuses(bt);
-
-	const int subtype = onLand ? SecondarySkill::LOGISTICS : SecondarySkill::NAVIGATION;
-	const double modifier = ti->valOfBonuses(Bonus::SECONDARY_SKILL_PREMY, subtype) / 100.0;
-
-	return static_cast<int>(base * (1 + modifier)) + bonus;
+int CGHeroInstance::maxMovePointsCached(bool onLand, const TurnInfo * ti) const
+{
+	updateArmyMovementBonus(onLand, ti);
+	return ti->valOfBonuses(Bonus::MOVEMENT, !!onLand);;
 }
 
 CGHeroInstance::CGHeroInstance():
@@ -226,7 +221,8 @@ CGHeroInstance::CGHeroInstance():
 	portrait(UNINITIALIZED_PORTRAIT),
 	level(1),
 	exp(UNINITIALIZED_EXPERIENCE),
-	sex(std::numeric_limits<ui8>::max())
+	sex(std::numeric_limits<ui8>::max()),
+	armyMovementVal(0)
 {
 	setNodeType(HERO);
 	ID = Obj::HERO;

lib/mapObjects/CGHeroInstance.h

@@ -49,6 +49,7 @@ class DLL_LINKAGE CGHeroInstance : public CArmedInstance, public IBoatGenerator,
 
 private:
 	std::set<SpellID> spells; //known spells (spell IDs)
+	mutable int armyMovementVal;
 
 public:
 
@@ -210,6 +211,9 @@ public:
 	int maxMovePoints(bool onLand) const;
 	//cached version is much faster, TurnInfo construction is costly
 	int maxMovePointsCached(bool onLand, const TurnInfo * ti) const;
+	//update army movement bonus
+	void updateArmyMovementBonus(bool onLand, const TurnInfo * ti) const;
+	int getArmyMovementBonus() const;
 
 	int movementPointsAfterEmbark(int MPsBefore, int basicCost, bool disembark = false, const TurnInfo * ti = nullptr) const;
 

lib/mapObjects/CGTownInstance.cpp

@@ -1676,7 +1676,7 @@ void COPWBonus::onHeroVisit (const CGHeroInstance * h) const
 			if(!h->hasBonusFrom(Bonus::OBJECT, Obj::STABLES)) //does not stack with advMap Stables
 			{
 				GiveBonus gb;
-				gb.bonus = Bonus(Bonus::ONE_WEEK, Bonus::LAND_MOVEMENT, Bonus::OBJECT, 600, 94, VLC->generaltexth->arraytxt[100]);
+				gb.bonus = Bonus(Bonus::ONE_WEEK, Bonus::MOVEMENT, Bonus::OBJECT, 600, 94, VLC->generaltexth->arraytxt[100], 1);
 				gb.id = heroID.getNum();
 				cb->giveHeroBonus(&gb);
 

lib/mapObjects/MiscObjects.cpp

@@ -2154,12 +2154,13 @@ void CGLighthouse::initObj(CRandomGenerator & rand)
 void CGLighthouse::giveBonusTo(const PlayerColor & player, bool onInit) const
 {
 	GiveBonus gb(GiveBonus::PLAYER);
-	gb.bonus.type = Bonus::SEA_MOVEMENT;
+	gb.bonus.type = Bonus::MOVEMENT;
 	gb.bonus.val = 500;
 	gb.id = player.getNum();
 	gb.bonus.duration = Bonus::PERMANENT;
 	gb.bonus.source = Bonus::OBJECT;
 	gb.bonus.sid = id.getNum();
+	gb.bonus.subtype = 1;
 
 	// FIXME: This is really dirty hack
 	// Proper fix would be to make CGLighthouse into bonus system node

lib/registerTypes/RegisterTypes.h

@@ -137,6 +137,7 @@ void registerTypesMapObjectTypes(Serializer &s)
 	s.template registerType<IUpdater, TimesHeroLevelUpdater>();
 	s.template registerType<IUpdater, TimesStackLevelUpdater>();
 	s.template registerType<IUpdater, OwnerUpdater>();
+	s.template registerType<IUpdater, ArmyMovementUpdater>();
 
 	s.template registerType<ILimiter, AnyOfLimiter>();
 	s.template registerType<ILimiter, NoneOfLimiter>();