mirror of
				https://github.com/vcmi/vcmi.git
				synced 2025-10-31 00:07:39 +02:00 
			
		
		
		
	Fixes for code review issues
This commit is contained in:
		| @@ -1462,14 +1462,9 @@ void CBattleInterface::displayEffect(ui32 effect, BattleHex destTile) | ||||
| 	addNewAnim(new CEffectAnimation(this, customAnim, destTile)); | ||||
| } | ||||
|  | ||||
| void CBattleInterface::displaySpellCast(SpellID spellID, BattleHex destinationTile) | ||||
| void CBattleInterface::displaySpellAnimationQueue(const CSpell::TAnimationQueue & q, BattleHex destinationTile) | ||||
| { | ||||
| 	const CSpell * spell = spellID.toSpell(); | ||||
|  | ||||
| 	if(spell == nullptr) | ||||
| 		return; | ||||
|  | ||||
| 	for(const CSpell::TAnimation & animation : spell->animationInfo.cast) | ||||
| 	for(const CSpell::TAnimation & animation : q) | ||||
| 	{ | ||||
| 		if(animation.pause > 0) | ||||
| 			addNewAnim(new CDummyAnimation(this, animation.pause)); | ||||
| @@ -1478,37 +1473,28 @@ void CBattleInterface::displaySpellCast(SpellID spellID, BattleHex destinationTi | ||||
| 	} | ||||
| } | ||||
|  | ||||
| void CBattleInterface::displaySpellCast(SpellID spellID, BattleHex destinationTile) | ||||
| { | ||||
| 	const CSpell * spell = spellID.toSpell(); | ||||
|  | ||||
| 	if(spell) | ||||
| 		displaySpellAnimationQueue(spell->animationInfo.cast, destinationTile); | ||||
| } | ||||
|  | ||||
| void CBattleInterface::displaySpellEffect(SpellID spellID, BattleHex destinationTile) | ||||
| { | ||||
| 	const CSpell *spell = spellID.toSpell(); | ||||
|  | ||||
| 	if(spell == nullptr) | ||||
| 		return; | ||||
|  | ||||
| 	for(const CSpell::TAnimation & animation : spell->animationInfo.affect) | ||||
| 	{ | ||||
| 		if(animation.pause > 0) | ||||
| 			addNewAnim(new CDummyAnimation(this, animation.pause)); | ||||
| 		else | ||||
| 			addNewAnim(new CEffectAnimation(this, animation.resourceName, destinationTile, false, animation.verticalPosition == VerticalPosition::BOTTOM)); | ||||
|  | ||||
| 	} | ||||
| 	if(spell) | ||||
| 		displaySpellAnimationQueue(spell->animationInfo.affect, destinationTile); | ||||
| } | ||||
|  | ||||
| void CBattleInterface::displaySpellHit(SpellID spellID, BattleHex destinationTile) | ||||
| { | ||||
| 	const CSpell * spell = spellID.toSpell(); | ||||
|  | ||||
| 	if(spell == nullptr) | ||||
| 		return; | ||||
|  | ||||
| 	for(const CSpell::TAnimation & animation : spell->animationInfo.hit) | ||||
| 	{ | ||||
| 		if(animation.pause > 0) | ||||
| 			addNewAnim(new CDummyAnimation(this, animation.pause)); | ||||
| 		else | ||||
| 			addNewAnim(new CEffectAnimation(this, animation.resourceName, destinationTile, false, animation.verticalPosition == VerticalPosition::BOTTOM)); | ||||
| 	} | ||||
| 	if(spell) | ||||
| 		displaySpellAnimationQueue(spell->animationInfo.hit, destinationTile); | ||||
| } | ||||
|  | ||||
| void CBattleInterface::battleTriggerEffect(const BattleTriggerEffect & bte) | ||||
| @@ -1704,13 +1690,12 @@ void CBattleInterface::enterCreatureCastingMode() | ||||
| 		spells::Target target; | ||||
| 		target.emplace_back(); | ||||
|  | ||||
|  | ||||
| 		spells::BattleCast cast(curInt->cb.get(), caster, spells::Mode::CREATURE_ACTIVE, spell); | ||||
|  | ||||
| 		auto m = spell->battleMechanics(&cast); | ||||
| 		spells::detail::ProblemImpl ignored; | ||||
|  | ||||
| 		const bool isCastingPossible = m->canBeCastAt(ignored, target); | ||||
| 		const bool isCastingPossible = m->canBeCastAt(target, ignored); | ||||
|  | ||||
| 		if (isCastingPossible) | ||||
| 		{ | ||||
| @@ -2543,7 +2528,7 @@ bool CBattleInterface::isCastingPossibleHere(const CStack *sactive, const CStack | ||||
| 			auto m = sp->battleMechanics(&cast); | ||||
| 			spells::detail::ProblemImpl problem; //todo: display problem in status bar | ||||
|  | ||||
| 			isCastingPossible = m->canBeCastAt(problem, target); | ||||
| 			isCastingPossible = m->canBeCastAt(target, problem); | ||||
| 		} | ||||
| 	} | ||||
| 	else | ||||
|   | ||||
| @@ -358,6 +358,7 @@ public: | ||||
|  | ||||
| 	void displayEffect(ui32 effect, BattleHex destTile); //displays custom effect on the battlefield | ||||
|  | ||||
| 	void displaySpellAnimationQueue(const CSpell::TAnimationQueue & q, BattleHex destinationTile); | ||||
| 	void displaySpellCast(SpellID spellID, BattleHex destinationTile); //displays spell`s cast animation | ||||
| 	void displaySpellEffect(SpellID spellID, BattleHex destinationTile); //displays spell`s affected animation | ||||
| 	void displaySpellHit(SpellID spellID, BattleHex destinationTile); //displays spell`s affected animation | ||||
|   | ||||
| @@ -112,12 +112,14 @@ void CBuildingRect::hover(bool on) | ||||
| void CBuildingRect::clickLeft(tribool down, bool previousState) | ||||
| { | ||||
| 	if(previousState && getBuilding() && area && !down && (parent->selectedBuilding==this)) | ||||
| 	{ | ||||
| 		if(!CSDL_Ext::isTransparent(area, GH.current->motion.x-pos.x, GH.current->motion.y-pos.y)) //inside building image | ||||
| 		{ | ||||
| 			auto building = getBuilding(); | ||||
| 			parent->buildingClicked(building->bid, building->subId, building->upgrade); | ||||
| 		} | ||||
| 	} | ||||
| } | ||||
|  | ||||
| void CBuildingRect::clickRight(tribool down, bool previousState) | ||||
| { | ||||
| @@ -606,8 +608,7 @@ void CCastleBuildings::recreate() | ||||
|  | ||||
| 		const CStructure * toAdd = *boost::max_element(entry.second, [=](const CStructure * a, const CStructure * b) | ||||
| 		{ | ||||
| 			return build->getDistance(a->building->bid) | ||||
| 			     < build->getDistance(b->building->bid); | ||||
| 			return build->getDistance(a->building->bid) < build->getDistance(b->building->bid); | ||||
| 		}); | ||||
|  | ||||
| 		buildings.push_back(std::make_shared<CBuildingRect>(this, town, toAdd)); | ||||
|   | ||||
| @@ -32,20 +32,20 @@ public: | ||||
| 	using PostHandler = std::function<void(const E &)>; | ||||
| 	using BusTag = const void *; | ||||
|  | ||||
| 	std::unique_ptr<EventSubscription> subscribeBefore(BusTag tag, PreHandler && cb) | ||||
| 	std::unique_ptr<EventSubscription> subscribeBefore(BusTag tag, PreHandler && handler) | ||||
| 	{ | ||||
| 		boost::unique_lock<boost::shared_mutex> lock(mutex); | ||||
|  | ||||
| 		auto storage = std::make_shared<PreHandlerStorage>(std::move(cb)); | ||||
| 		auto storage = std::make_shared<PreHandlerStorage>(std::move(handler)); | ||||
| 		preHandlers[tag].push_back(storage); | ||||
| 		return make_unique<PreSubscription>(tag, storage); | ||||
| 	} | ||||
|  | ||||
| 	std::unique_ptr<EventSubscription> subscribeAfter(BusTag tag, PostHandler && cb) | ||||
| 	std::unique_ptr<EventSubscription> subscribeAfter(BusTag tag, PostHandler && handler) | ||||
| 	{ | ||||
| 		boost::unique_lock<boost::shared_mutex> lock(mutex); | ||||
|  | ||||
| 		auto storage = std::make_shared<PostHandlerStorage>(std::move(cb)); | ||||
| 		auto storage = std::make_shared<PostHandlerStorage>(std::move(handler)); | ||||
| 		postHandlers[tag].push_back(storage); | ||||
| 		return make_unique<PostSubscription>(tag, storage); | ||||
| 	} | ||||
| @@ -84,18 +84,18 @@ private: | ||||
| 	class HandlerStorage | ||||
| 	{ | ||||
| 	public: | ||||
| 		explicit HandlerStorage(T && cb_) | ||||
| 			: cb(cb_) | ||||
| 		explicit HandlerStorage(T && handler_) | ||||
| 			: handler(handler_) | ||||
| 		{ | ||||
| 		} | ||||
|  | ||||
| 		STRONG_INLINE | ||||
| 		void operator()(E & event) | ||||
| 		{ | ||||
| 			cb(event); | ||||
| 			handler(event); | ||||
| 		} | ||||
| 	private: | ||||
| 		T cb; | ||||
| 		T handler; | ||||
| 	}; | ||||
|  | ||||
| 	using PreHandlerStorage = HandlerStorage<PreHandler>; | ||||
| @@ -104,8 +104,8 @@ private: | ||||
| 	class PreSubscription : public EventSubscription | ||||
| 	{ | ||||
| 	public: | ||||
| 		PreSubscription(BusTag tag_, std::shared_ptr<PreHandlerStorage> cb_) | ||||
| 			: cb(cb_), | ||||
| 		PreSubscription(BusTag tag_, std::shared_ptr<PreHandlerStorage> handler_) | ||||
| 			: handler(handler_), | ||||
| 			tag(tag_) | ||||
| 		{ | ||||
| 		} | ||||
| @@ -113,18 +113,18 @@ private: | ||||
| 		virtual ~PreSubscription() | ||||
| 		{ | ||||
| 			auto registry = E::getRegistry(); | ||||
| 			registry->unsubscribe(tag, cb, registry->preHandlers); | ||||
| 			registry->unsubscribe(tag, handler, registry->preHandlers); | ||||
| 		} | ||||
| 	private: | ||||
| 		BusTag tag; | ||||
| 		std::shared_ptr<PreHandlerStorage> cb; | ||||
| 		std::shared_ptr<PreHandlerStorage> handler; | ||||
| 	}; | ||||
|  | ||||
| 	class PostSubscription : public EventSubscription | ||||
| 	{ | ||||
| 	public: | ||||
| 		PostSubscription(BusTag tag_, std::shared_ptr<PostHandlerStorage> cb_) | ||||
| 			: cb(cb_), | ||||
| 		PostSubscription(BusTag tag_, std::shared_ptr<PostHandlerStorage> handler_) | ||||
| 			: handler(handler_), | ||||
| 			tag(tag_) | ||||
| 		{ | ||||
| 		} | ||||
| @@ -132,11 +132,11 @@ private: | ||||
| 		virtual ~PostSubscription() | ||||
| 		{ | ||||
| 			auto registry = E::getRegistry(); | ||||
| 			registry->unsubscribe(tag, cb, registry->postHandlers); | ||||
| 			registry->unsubscribe(tag, handler, registry->postHandlers); | ||||
| 		} | ||||
| 	private: | ||||
| 		BusTag tag; | ||||
| 		std::shared_ptr<PostHandlerStorage> cb; | ||||
| 		std::shared_ptr<PostHandlerStorage> handler; | ||||
| 	}; | ||||
|  | ||||
| 	boost::shared_mutex mutex; | ||||
|   | ||||
| @@ -574,7 +574,9 @@ void CTownHandler::addBonusesForVanilaBuilding(CBuilding * building) | ||||
| 	if(building->subId == BuildingSubID::NONE) | ||||
| 	{ | ||||
| 		if(building->bid == BuildingID::TAVERN) | ||||
| 		{ | ||||
| 			b = createBonus(building, Bonus::MORALE, +1); | ||||
| 		} | ||||
| 		else if(building->bid == BuildingID::GRAIL | ||||
| 			&& building->town->faction != nullptr | ||||
| 			&& boost::algorithm::ends_with(building->town->faction->identifier, ":cove")) | ||||
|   | ||||
| @@ -217,7 +217,8 @@ std::vector<JsonNode> ScriptHandler::loadLegacyData(size_t dataSize) | ||||
| 	return std::vector<JsonNode>(); | ||||
| } | ||||
|  | ||||
| ScriptPtr ScriptHandler::loadFromJson(vstd::CLoggerBase * logger, const std::string & scope, const JsonNode & json, const std::string & identifier) const | ||||
| ScriptPtr ScriptHandler::loadFromJson(vstd::CLoggerBase * logger, const std::string & scope, | ||||
| 	const JsonNode & json, const std::string & identifier) const | ||||
| { | ||||
| 	ScriptPtr ret = std::make_shared<ScriptImpl>(this); | ||||
|  | ||||
|   | ||||
| @@ -1407,12 +1407,10 @@ AttackableTiles CBattleInfoCallback::getPotentiallyAttackableHexes (const  battl | ||||
| 			{ | ||||
| 				auto st = battleGetUnitByPos(tile, true); | ||||
| 				if(st && battleMatchOwner(st, attacker)) //only hostile stacks - does it work well with Berserk? | ||||
| 				{ | ||||
| 					at.hostileCreaturePositions.insert(tile); | ||||
| 			} | ||||
| 		} | ||||
| 	} | ||||
| 	} | ||||
| 	if(attacker->hasBonusOfType(Bonus::WIDE_BREATH)) | ||||
| 	{ | ||||
| 		std::vector<BattleHex> hexes = destinationTile.neighbouringTiles(); | ||||
| @@ -1429,11 +1427,9 @@ AttackableTiles CBattleInfoCallback::getPotentiallyAttackableHexes (const  battl | ||||
| 			//friendly stacks can also be damaged by Dragon Breath | ||||
| 			auto st = battleGetUnitByPos(tile, true); | ||||
| 			if(st && st != attacker) | ||||
| 			{ | ||||
| 				at.friendlyCreaturePositions.insert(tile); | ||||
| 		} | ||||
| 	} | ||||
| 	} | ||||
| 	else if(attacker->hasBonusOfType(Bonus::TWO_HEX_ATTACK_BREATH)) | ||||
| 	{ | ||||
| 		int pos = BattleHex::mutualPosition(destinationTile, hex); | ||||
|   | ||||
| @@ -1791,6 +1791,7 @@ void CTownBonus::onHeroVisit (const CGHeroInstance * h) const | ||||
| 			} | ||||
| 			break; | ||||
| 		} | ||||
|  | ||||
| 		if(what != PrimarySkill::NONE) | ||||
| 		{ | ||||
| 			iw.player = cb->getOwner(heroID); | ||||
|   | ||||
| @@ -203,7 +203,7 @@ bool BattleSpellMechanics::canBeCast(Problem & problem) const | ||||
| 	return effects->applicable(problem, this); | ||||
| } | ||||
|  | ||||
| bool BattleSpellMechanics::canBeCastAt(Problem & problem, const Target & target) const | ||||
| bool BattleSpellMechanics::canBeCastAt(const Target & target, Problem & problem) const | ||||
| { | ||||
| 	if(!canBeCast(problem)) | ||||
| 		return false; | ||||
| @@ -618,7 +618,7 @@ std::vector<Destination> BattleSpellMechanics::getPossibleDestinations(size_t in | ||||
|  | ||||
| 				detail::ProblemImpl ingored; | ||||
|  | ||||
| 				if(canBeCastAt(ingored, tmp)) | ||||
| 				if(canBeCastAt(tmp, ingored)) | ||||
| 					ret.emplace_back(dest); | ||||
| 			} | ||||
| 		} | ||||
|   | ||||
| @@ -28,7 +28,7 @@ public: | ||||
| 	void applyEffects(ServerCallback * server, const Target & targets, bool indirect, bool ignoreImmunity) const override; | ||||
|  | ||||
| 	bool canBeCast(Problem & problem) const override; | ||||
| 	bool canBeCastAt(Problem & problem, const Target & target) const override; | ||||
| 	bool canBeCastAt(const Target & target, Problem & problem) const override; | ||||
|  | ||||
| 	void cast(ServerCallback * server, const Target & target) override final; | ||||
| 	void castEval(ServerCallback * server, const Target & target) override final; | ||||
|   | ||||
| @@ -183,7 +183,7 @@ public: | ||||
| 	virtual std::vector<const CStack *> getAffectedStacks(const Target & target) const = 0; | ||||
|  | ||||
| 	virtual bool canBeCast(Problem & problem) const = 0; | ||||
| 	virtual bool canBeCastAt(Problem & problem, const Target & target) const = 0; | ||||
| 	virtual bool canBeCastAt(const Target & target, Problem & problem) const = 0; | ||||
|  | ||||
| 	virtual void applyEffects(ServerCallback * server, const Target & targets, bool indirect, bool ignoreImmunity) const = 0; | ||||
|  | ||||
|   | ||||
| @@ -5631,9 +5631,9 @@ void CGameHandler::attackCasting(bool ranged, Bonus::BonusType attackMode, const | ||||
|  | ||||
| 			auto m = spell->battleMechanics(¶meters); | ||||
|  | ||||
| 			spells::detail::ProblemImpl ingored; | ||||
| 			spells::detail::ProblemImpl ignored; | ||||
|  | ||||
| 			if(!m->canBeCastAt(ingored, target)) | ||||
| 			if(!m->canBeCastAt(target, ignored)) | ||||
| 				continue; | ||||
|  | ||||
| 			//check if spell should be cast (probability handling) | ||||
|   | ||||
| @@ -297,7 +297,7 @@ TEST_F(CGameStateTest, issue2765) | ||||
|  | ||||
| 		auto m = age->battleMechanics(&cast); | ||||
|  | ||||
| 		EXPECT_FALSE(m->canBeCastAt(problemMock, target)); | ||||
| 		EXPECT_FALSE(m->canBeCastAt(target, problemMock)); | ||||
|  | ||||
| 		EXPECT_TRUE(cast.castIfPossible(this, target));//should be possible, but with no effect (change to aimed cast check?) | ||||
|  | ||||
| @@ -397,7 +397,7 @@ TEST_F(CGameStateTest, battleResurrection) | ||||
|  | ||||
| 		EXPECT_TRUE(m->canBeCast(problemMock)); | ||||
|  | ||||
| 		EXPECT_TRUE(m->canBeCastAt(problemMock, target)); | ||||
| 		EXPECT_TRUE(m->canBeCastAt(target, problemMock)); | ||||
|  | ||||
| 		cast.cast(this, target); | ||||
| // | ||||
|   | ||||
| @@ -25,7 +25,7 @@ public: | ||||
| 	MOCK_CONST_METHOD1(getAffectedStacks, std::vector<const CStack *>(const Target &)); | ||||
|  | ||||
| 	MOCK_CONST_METHOD1(canBeCast, bool(Problem &)); | ||||
| 	MOCK_CONST_METHOD2(canBeCastAt, bool(Problem &, const Target &)); | ||||
| 	MOCK_CONST_METHOD2(canBeCastAt, bool(const Target &, Problem &)); | ||||
|  | ||||
| 	MOCK_CONST_METHOD4(applyEffects, void(ServerCallback *, const Target &, bool, bool)); | ||||
|  | ||||
|   | ||||
		Reference in New Issue
	
	Block a user