Merge pull request #2243 from IvanSavenko/fix_ship_shipyard

Cleanup & fix shipyard / boat creation code

client/Client.h

@@ -187,6 +187,7 @@ public:
 
 	void changeSpells(const CGHeroInstance * hero, bool give, const std::set<SpellID> & spells) override {};
 	bool removeObject(const CGObjectInstance * obj) override {return false;};
+	void createObject(const int3 & visitablePosition, Obj type, int32_t subtype ) override {};
 	void setOwner(const CGObjectInstance * obj, PlayerColor owner) override {};
 	void changePrimSkill(const CGHeroInstance * hero, PrimarySkill::PrimarySkill which, si64 val, bool abs = false) override {};
 	void changeSecSkill(const CGHeroInstance * hero, SecondarySkill which, int val, bool abs = false) override {};

client/NetPacksClient.cpp

@@ -961,7 +961,7 @@ void ApplyClientNetPackVisitor::visitNewObject(NewObject & pack)
 {
 	cl.invalidatePaths();
 
-	const CGObjectInstance *obj = cl.getObj(pack.id);
+	const CGObjectInstance *obj = cl.getObj(pack.createdObjectID);
 	if(CGI->mh)
 		CGI->mh->onObjectFadeIn(obj);
 

lib/IGameCallback.h

@@ -90,6 +90,7 @@ public:
 
 	virtual void changeSpells(const CGHeroInstance * hero, bool give, const std::set<SpellID> &spells)=0;
 	virtual bool removeObject(const CGObjectInstance * obj)=0;
+	virtual void createObject(const int3 & visitablePosition, Obj type, int32_t subtype = 0) = 0;
 	virtual void setOwner(const CGObjectInstance * objid, PlayerColor owner)=0;
 	virtual void changePrimSkill(const CGHeroInstance * hero, PrimarySkill::PrimarySkill which, si64 val, bool abs=false)=0;
 	virtual void changeSecSkill(const CGHeroInstance * hero, SecondarySkill which, int val, bool abs=false)=0;

lib/NetPacks.h

@@ -384,7 +384,9 @@ struct DLL_LINKAGE ChangeObjPos : public CPackForClient
 {
 	void applyGs(CGameState * gs);
 
+	/// Object to move
 	ObjectInstanceID objid;
+	/// New position of visitable tile of an object
 	int3 nPos;
 
 	virtual void visitTyped(ICPackVisitor & visitor) override;
@@ -745,11 +747,14 @@ struct DLL_LINKAGE NewObject : public CPackForClient
 {
 	void applyGs(CGameState * gs);
 
+	/// Object ID to create
 	Obj ID;
+	/// Object secondary ID to create
 	ui32 subID = 0;
-	int3 pos;
+	/// Position of visitable tile of created object
+	int3 targetPos;
 
-	ObjectInstanceID id; //used locally, filled during applyGs
+	ObjectInstanceID createdObjectID; //used locally, filled during applyGs
 
 	virtual void visitTyped(ICPackVisitor & visitor) override;
 
@@ -757,7 +762,7 @@ struct DLL_LINKAGE NewObject : public CPackForClient
 	{
 		h & ID;
 		h & subID;
-		h & pos;
+		h & targetPos;
 	}
 };
 

lib/NetPacksLib.cpp

@@ -1019,7 +1019,7 @@ void ChangeObjPos::applyGs(CGameState *gs)
 		return;
 	}
 	gs->map->removeBlockVisTiles(obj);
-	obj->pos = nPos;
+	obj->pos = nPos + obj->getVisitableOffset();
 	gs->map->addBlockVisTiles(obj);
 }
 
@@ -1492,57 +1492,52 @@ void NewObject::applyGs(CGameState *gs)
 {
 	TerrainId terrainType = ETerrainId::NONE;
 
-	if(ID == Obj::BOAT && !gs->isInTheMap(pos)) //special handling for bug #3060 - pos outside map but visitablePos is not
+	if (!gs->isInTheMap(targetPos))
 	{
-		CGObjectInstance testObject = CGObjectInstance();
-		testObject.pos = pos;
-		testObject.appearance = VLC->objtypeh->getHandlerFor(ID, subID)->getTemplates(ETerrainId::WATER).front();
-
-		[[maybe_unused]] const int3 previousXAxisTile = CGBoat::translatePos(pos, true);
-		assert(gs->isInTheMap(previousXAxisTile) && (testObject.visitablePos() == previousXAxisTile));
-	}
-	else
-	{
-		const TerrainTile & t = gs->map->getTile(pos);
-		terrainType = t.terType->getId();
+		logGlobal->error("Attempt to create object outside map at %s!", targetPos.toString());
+		return;
 	}
 
+	const TerrainTile & t = gs->map->getTile(targetPos);
+	terrainType = t.terType->getId();
+
 	auto handler = VLC->objtypeh->getHandlerFor(ID, subID);
 	CGObjectInstance * o = handler->create();
 	handler->configureObject(o, gs->getRandomGenerator());
 	
-	switch(ID)
+	if (ID == Obj::MONSTER) //probably more options will be needed
 	{
-	case Obj::BOAT:
-		terrainType = ETerrainId::WATER; //TODO: either boat should only spawn on water, or all water objects should be handled this way
-		break;
-			
-	case Obj::MONSTER: //probably more options will be needed
-		{
-			//CStackInstance hlp;
-			auto * cre = dynamic_cast<CGCreature *>(o);
-			//cre->slots[0] = hlp;
-			assert(cre);
-			cre->notGrowingTeam = cre->neverFlees = false;
-			cre->character = 2;
-			cre->gainedArtifact = ArtifactID::NONE;
-			cre->identifier = -1;
-			cre->addToSlot(SlotID(0), new CStackInstance(CreatureID(subID), -1)); //add placeholder stack
-		}
-		break;
+		//CStackInstance hlp;
+		auto * cre = dynamic_cast<CGCreature *>(o);
+		//cre->slots[0] = hlp;
+		assert(cre);
+		cre->notGrowingTeam = cre->neverFlees = false;
+		cre->character = 2;
+		cre->gainedArtifact = ArtifactID::NONE;
+		cre->identifier = -1;
+		cre->addToSlot(SlotID(0), new CStackInstance(CreatureID(subID), -1)); //add placeholder stack
 	}
+
+	assert(!handler->getTemplates(terrainType).empty());
+
+	if (!handler->getTemplates(terrainType).empty())
+		o->appearance = handler->getTemplates(terrainType).front();
+	else
+		o->appearance = handler->getTemplates().front();
+
+	o->id = ObjectInstanceID(static_cast<si32>(gs->map->objects.size()));
 	o->ID = ID;
 	o->subID = subID;
-	o->pos = pos;
-	o->appearance = handler->getTemplates(terrainType).front();
-	id = o->id = ObjectInstanceID(static_cast<si32>(gs->map->objects.size()));
+	o->pos = targetPos + o->getVisitableOffset();
 
 	gs->map->objects.emplace_back(o);
 	gs->map->addBlockVisTiles(o);
 	o->initObj(gs->getRandomGenerator());
 	gs->map->calculateGuardingGreaturePositions();
 
-	logGlobal->debug("Added object id=%d; address=%x; name=%s", id, (intptr_t)o, o->getObjectName());
+	createdObjectID = o->id;
+
+	logGlobal->debug("Added object id=%d; address=%x; name=%s", o->id, (intptr_t)o, o->getObjectName());
 }
 
 void NewArtifact::applyGs(CGameState *gs)

lib/mapObjectConstructors/CommonConstructors.cpp

@@ -165,15 +165,6 @@ std::string BoatInstanceConstructor::getBoatAnimationName() const
 	return actualAnimation;
 }
 
-void BoatInstanceConstructor::afterLoadFinalization()
-{
-	if (layer == EPathfindingLayer::SAIL)
-	{
-		if (getTemplates(TerrainId(ETerrainId::WATER)).empty())
-			logMod->warn("Boat of type %s has no templates suitable for water!", getJsonKey());
-	}
-}
-
 void MarketInstanceConstructor::initTypeData(const JsonNode & input)
 {
 	for(auto & element : input["modes"].Vector())

lib/mapObjectConstructors/CommonConstructors.h

@@ -97,7 +97,6 @@ protected:
 	
 public:
 	void initializeObject(CGBoat * object) const override;
-	void afterLoadFinalization() override;
 
 	/// Returns boat preview animation, for use in Shipyards
 	std::string getBoatAnimationName() const;

lib/mapObjects/CGHeroInstance.cpp

@@ -451,12 +451,7 @@ void CGHeroInstance::onHeroVisit(const CGHeroInstance * h) const
 			{
 				smp.val = maxMovePoints(false);
 				//Create a new boat for hero
-				NewObject no;
-				no.ID = Obj::BOAT;
-				no.subID = getBoatType().getNum();
-
-				cb->sendAndApply(&no);
-
+				cb->createObject(boatPos, Obj::BOAT, getBoatType().getNum());
 				boatId = cb->getTopObj(boatPos)->id;
 			}
 			else
@@ -958,11 +953,15 @@ BoatId CGHeroInstance::getBoatType() const
 
 void CGHeroInstance::getOutOffsets(std::vector<int3> &offsets) const
 {
-	// FIXME: Offsets need to be fixed once we get rid of convertPosition
-	// Check issue 515 for details
-	offsets =
-	{
-		int3(-1,1,0), int3(-1,-1,0), int3(-2,0,0), int3(0,0,0), int3(0,1,0), int3(-2,1,0), int3(0,-1,0), int3(-2,-1,0)
+	offsets = {
+		{0, -1, 0},
+		{+1, -1, 0},
+		{+1, 0, 0},
+		{+1, +1, 0},
+		{0, +1, 0},
+		{-1, +1, 0},
+		{-1, 0, 0},
+		{-1, -1, 0},
 	};
 }
 

lib/mapObjects/CGTownInstance.cpp

@@ -585,7 +585,7 @@ bool CGTownInstance::passableFor(PlayerColor color) const
 
 void CGTownInstance::getOutOffsets( std::vector<int3> &offsets ) const
 {
-	offsets = {int3(-1,2,0), int3(-3,2,0)};
+	offsets = {int3(-1,2,0), int3(+1,2,0)};
 }
 
 CGTownInstance::EGeneratorState CGTownInstance::shipyardStatus() const

lib/mapObjects/IObjectInterface.cpp

@@ -92,10 +92,13 @@ int3 IBoatGenerator::bestLocation() const
 
 	for (auto & offset : offsets)
 	{
-		if(const TerrainTile *tile = getObject()->cb->getTile(getObject()->getPosition() + offset, false)) //tile is in the map
+		int3 targetTile = getObject()->visitablePos() + offset;
+		const TerrainTile *tile = getObject()->cb->getTile(targetTile, false);
+
+		if(tile) //tile is in the map
 		{
 			if(tile->terType->isWater()  &&  (!tile->blocked || tile->blockingObjects.front()->ID == Obj::BOAT)) //and is water and is not blocked or is blocked by boat
-				return getObject()->getPosition() + offset;
+				return targetTile;
 		}
 	}
 	return int3 (-1,-1,-1);

lib/mapObjects/MiscObjects.cpp

@@ -1294,22 +1294,6 @@ CGBoat::CGBoat()
 	layer = EPathfindingLayer::EEPathfindingLayer::SAIL;
 }
 
-int3 CGBoat::translatePos(const int3& pos, bool reverse /* = false */)
-{
-	//pos - offset we want to place the boat at the map
-	//returned value - actual position as seen by game mechanics
-
-	//If reverse = true, then it's the opposite.
-	if (!reverse)
-	{
-		return pos + int3(1, 0, 0);
-	}
-	else
-	{
-		return pos - int3(1, 0, 0);
-	}
-}
-
 void CGSirens::initObj(CRandomGenerator & rand)
 {
 	blockVisit = true;
@@ -1371,9 +1355,18 @@ void CGShipyard::getOutOffsets( std::vector<int3> &offsets ) const
 	// A x S x B
 	// C E G F D
 	offsets = {
-		int3(-3,0,0), int3(1,0,0), //AB
-		int3(-3,1,0), int3(1,1,0), int3(-2,1,0), int3(0,1,0), int3(-1,1,0), //CDEFG
-		int3(-3,-1,0), int3(1,-1,0), int3(-2,-1,0), int3(0,-1,0), int3(-1,-1,0) //HIJKL
+		{-2, 0,  0}, // A
+		{+2, 0,  0}, // B
+		{-2, 1,  0}, // C
+		{+2, 1,  0}, // D
+		{-1, 1,  0}, // E
+		{+1, 1,  0}, // F
+		{0,  1,  0}, // G
+		{-2, -1, 0}, // H
+		{+2, -1, 0}, // I
+		{-1, -1, 0}, // G
+		{+1, -1, 0}, // K
+		{0,  -1, 0}, // L
 	};
 }
 
@@ -1384,11 +1377,10 @@ const IObjectInterface * CGShipyard::getObject() const
 
 void CGShipyard::onHeroVisit( const CGHeroInstance * h ) const
 {
-	if(!cb->gameState()->getPlayerRelations(tempOwner, h->tempOwner))
+	if(cb->gameState()->getPlayerRelations(tempOwner, h->tempOwner) == PlayerRelations::ENEMIES)
 		cb->setOwner(this, h->tempOwner);
 
-	auto s = shipyardStatus();
-	if(s != IBoatGenerator::GOOD)
+	if(shipyardStatus() != IBoatGenerator::GOOD)
 	{
 		InfoWindow iw;
 		iw.type = EInfoWindowMode::AUTO;
@@ -1409,8 +1401,7 @@ void CGShipyard::serializeJsonOptions(JsonSerializeFormat& handler)
 
 BoatId CGShipyard::getBoatType() const
 {
-	// In H3, external shipyard will always create same boat as castle
-	return EBoatId::CASTLE;
+	return createdBoat;
 }
 
 void CCartographer::onHeroVisit( const CGHeroInstance * h ) const

lib/mapObjects/MiscObjects.h

@@ -359,7 +359,6 @@ public:
 	std::array<std::string, PlayerColor::PLAYER_LIMIT_I> flagAnimations;
 
 	CGBoat();
-	static int3 translatePos(const int3 &pos, bool reverse = false);
 
 	template <typename Handler> void serialize(Handler &h, const int version)
 	{

lib/spells/AdventureSpellMechanics.cpp

@@ -201,7 +201,7 @@ ESpellCastResult SummonBoatMechanics::applyAdventureEffects(SpellCastEnvironment
 	{
 		ChangeObjPos cop;
 		cop.objid = nearest->id;
-		cop.nPos = CGBoat::translatePos(summonPos);
+		cop.nPos = summonPos;
 		env->apply(&cop);
 	}
 	else if(schoolLevel < 2) //none or basic level -> cannot create boat :(
@@ -216,7 +216,7 @@ ESpellCastResult SummonBoatMechanics::applyAdventureEffects(SpellCastEnvironment
 		NewObject no;
 		no.ID = Obj::BOAT;
 		no.subID = BoatId(EBoatId::NECROPOLIS);
-		no.pos = CGBoat::translatePos(summonPos);
+		no.targetPos = summonPos;
 		env->apply(&no);
 	}
 	return ESpellCastResult::OK;

server/CGameHandler.cpp

@@ -4429,11 +4429,7 @@ bool CGameHandler::hireHero(const CGObjectInstance *obj, ui8 hid, PlayerColor pl
 	if (getTile(hr.tile)->isWater())
 	{
 		//Create a new boat for hero
-		NewObject no;
-		no.ID = Obj::BOAT;
-		no.subID = nh->getBoatType().getNum();
-		no.pos = hr.tile + int3(1,0,0);
-		sendAndApply(&no);
+		createObject(obj->visitablePos(), Obj::BOAT, nh->getBoatType().getNum());
 
 		hr.boatId = getTopObj(hr.tile)->id;
 	}
@@ -5653,16 +5649,8 @@ bool CGameHandler::buildBoat(ObjectInstanceID objid, PlayerColor playerID)
 		return false;
 	}
 
-	//take boat cost
 	giveResources(playerID, -boatCost);
-
-	//create boat
-	NewObject no;
-	no.ID = Obj::BOAT;
-	no.subID = obj->getBoatType().getNum();
-	no.pos = tile + int3(1,0,0);
-	sendAndApply(&no);
-
+	createObject(tile, Obj::BOAT, obj->getBoatType().getNum());
 	return true;
 }
 
@@ -5803,12 +5791,7 @@ bool CGameHandler::dig(const CGHeroInstance *h)
 	if (h->diggingStatus() != EDiggingStatus::CAN_DIG) //checks for terrain and movement
 		COMPLAIN_RETF("Hero cannot dig (error code %d)!", h->diggingStatus());
 
-	//create a hole
-	NewObject no;
-	no.ID = Obj::HOLE;
-	no.pos = h->visitablePos();
-	no.subID = 0;
-	sendAndApply(&no);
+	createObject(h->visitablePos(), Obj::HOLE, 0 );
 
 	//take MPs
 	SetMovePoints smp;
@@ -6875,7 +6858,9 @@ void CGameHandler::spawnWanderingMonsters(CreatureID creatureID)
 		{
 			auto count = cre->getRandomAmount(std::rand);
 
-			auto monsterId  = putNewObject(Obj::MONSTER, creatureID, *tile);
+			createObject(*tile, Obj::MONSTER, creatureID);
+			auto monsterId = getTopObj(*tile)->id;
+
 			setObjProperty(monsterId, ObjProperty::MONSTER_COUNT, count);
 			setObjProperty(monsterId, ObjProperty::MONSTER_POWER, (si64)1000*count);
 		}
@@ -7404,12 +7389,11 @@ scripting::Pool * CGameHandler::getContextPool() const
 }
 #endif
 
-const ObjectInstanceID CGameHandler::putNewObject(Obj ID, int subID, int3 pos)
+void CGameHandler::createObject(const int3 & visitablePosition, Obj type, int32_t subtype)
 {
 	NewObject no;
-	no.ID = ID; //creature
-	no.subID= subID;
-	no.pos = pos;
+	no.ID = type;
+	no.subID= subtype;
+	no.targetPos = visitablePosition;
 	sendAndApply(&no);
-	return no.id; //id field will be filled during applying on gs
 }

server/CGameHandler.h

@@ -153,6 +153,7 @@ public:
 	//do sth
 	void changeSpells(const CGHeroInstance * hero, bool give, const std::set<SpellID> &spells) override;
 	bool removeObject(const CGObjectInstance * obj) override;
+	void createObject(const int3 & visitablePosition, Obj type, int32_t subtype ) override;
 	void setOwner(const CGObjectInstance * obj, PlayerColor owner) override;
 	void changePrimSkill(const CGHeroInstance * hero, PrimarySkill::PrimarySkill which, si64 val, bool abs=false) override;
 	void changeSecSkill(const CGHeroInstance * hero, SecondarySkill which, int val, bool abs=false) override;
@@ -276,7 +277,6 @@ public:
 	void engageIntoBattle( PlayerColor player );
 	bool dig(const CGHeroInstance *h);
 	void moveArmy(const CArmedInstance *src, const CArmedInstance *dst, bool allowMerging);
-	const ObjectInstanceID putNewObject(Obj ID, int subID, int3 pos);
 
 	template <typename Handler> void serialize(Handler &h, const int version)
 	{

test/mock/mock_IGameCallback.h

@@ -41,6 +41,7 @@ public:
 
 	void changeSpells(const CGHeroInstance * hero, bool give, const std::set<SpellID> &spells) override {}
 	bool removeObject(const CGObjectInstance * obj) override {return false;}
+	void createObject(const int3 & visitablePosition, Obj type, int32_t subtype = 0) override {};
 	void setOwner(const CGObjectInstance * objid, PlayerColor owner) override {}
 	void changePrimSkill(const CGHeroInstance * hero, PrimarySkill::PrimarySkill which, si64 val, bool abs=false) override {}
 	void changeSecSkill(const CGHeroInstance * hero, SecondarySkill which, int val, bool abs=false) override {}