[Minor] Add validity check for NearByLocation and fix incorrect usage…

… of TryGetCellAt and GetCellAt (#1527) - NearByLocation may return CellStruct::Empty, it should be checked. - TryGetCellAt and GetCellAt are being misused, I inferred the intended usage based on the code, but I am not sure if this is the original authors' idea.
Phobos-developers · Feb 26, 2025 · af5f6b6 · af5f6b6
1 parent 20efc64
commit af5f6b6
Show file tree

Hide file tree

Showing 10 changed files with 54 additions and 65 deletions.
diff --git a/src/Ext/Anim/Hooks.AnimCreateUnit.cpp b/src/Ext/Anim/Hooks.AnimCreateUnit.cpp
@@ -46,16 +46,7 @@ DEFINE_HOOK(0x4226F0, AnimClass_CTOR_CreateUnit_MarkOccupationBits, 0x6)
 	auto const pTypeExt = AnimTypeExt::ExtMap.Find(pThis->Type);
 
 	if (pTypeExt->CreateUnit.Get())
-	{
-		auto location = pThis->GetCoords();
-
-		if (auto pCell = pThis->GetCell())
-			location = pCell->GetCoordsWithBridge();
-		else
-			location.Z = MapClass::Instance->GetCellFloorHeight(location);
-
-		pThis->MarkAllOccupationBits(location);
-	}
+		pThis->MarkAllOccupationBits(pThis->GetCell()->GetCoordsWithBridge());
 
 	return 0; //return (pThis->Type->MakeInfantry != -1) ? 0x423BD6 : 0x423C03;
 }
@@ -69,15 +60,7 @@ DEFINE_HOOK(0x424932, AnimClass_AI_CreateUnit_ActualEffects, 0x6)
 	if (auto const pUnitType = pTypeExt->CreateUnit.Get())
 	{
 		auto const pExt = AnimExt::ExtMap.Find(pThis);
-		auto origLocation = pThis->Location;
-		auto const pCell = pThis->GetCell();
-
-		if (pCell)
-			origLocation = pCell->GetCoordsWithBridge();
-		else
-			origLocation.Z = MapClass::Instance->GetCellFloorHeight(origLocation);
-
-		pThis->UnmarkAllOccupationBits(origLocation);
+		pThis->UnmarkAllOccupationBits(pThis->GetCell()->GetCoordsWithBridge());
 
 		auto facing = pTypeExt->CreateUnit_RandomFacing
 			? static_cast<DirType>(ScenarioClass::Instance->Random.RandomRanged(0, 255)) : pTypeExt->CreateUnit_Facing;

diff --git a/src/Ext/Anim/Hooks.cpp b/src/Ext/Anim/Hooks.cpp
@@ -26,9 +26,7 @@ DEFINE_HOOK(0x423B95, AnimClass_AI_HideIfNoOre_Threshold, 0x8)
 	if (pType->HideIfNoOre)
 	{
 		auto nThreshold = abs(AnimTypeExt::ExtMap.Find(pThis->Type)->HideIfNoOre_Threshold.Get());
-		auto pCell = pThis->GetCell();
-
-		pThis->Invisible = !pCell || pCell->GetContainedTiberiumValue() <= nThreshold;
+		pThis->Invisible = pThis->GetCell()->GetContainedTiberiumValue() <= nThreshold;
 	}
 
 	return 0x423BBF;
@@ -387,12 +385,10 @@ DEFINE_HOOK(0x423061, AnimClass_DrawIt_Visibility, 0x6)
 
 	if (pTypeExt->RestrictVisibilityIfCloaked && !HouseClass::IsCurrentPlayerObserver()
 		&& pTechno && (pTechno->CloakState == CloakState::Cloaked || pTechno->CloakState == CloakState::Cloaking)
-		&& !pTechno->Owner->IsAlliedWith(pCurrentHouse))
+		&& !pTechno->Owner->IsAlliedWith(pCurrentHouse)
+		&& !pTechno->GetCell()->Sensors_InclHouse(pCurrentHouse->ArrayIndex))
 	{
-		auto const pCell = pTechno->GetCell();
-
-		if (pCell && !pCell->Sensors_InclHouse(pCurrentHouse->ArrayIndex))
-			return SkipDrawing;
+		return SkipDrawing;
 	}
 
 	auto pOwner = pThis->OwnerObject ? pThis->OwnerObject->GetOwningHouse() : pThis->Owner;

diff --git a/src/Ext/House/Body.cpp b/src/Ext/House/Body.cpp
@@ -324,6 +324,9 @@ CellClass* HouseExt::GetEnemyBaseGatherCell(HouseClass* pTargetHouse, HouseClass
 	auto cellStruct = CellClass::Coord2Cell(newCoords);
 	cellStruct = MapClass::Instance->NearByLocation(cellStruct, speedTypeZone, -1, MovementZone::Normal, false, 3, 3, false, false, false, true, cellStruct, false, false);
 
+	if (cellStruct == CellStruct::Empty)
+		return nullptr;
+
 	return MapClass::Instance->TryGetCellAt(cellStruct);
 }
 

diff --git a/src/Ext/RadSite/Body.cpp b/src/Ext/RadSite/Body.cpp
@@ -101,17 +101,16 @@ void RadSiteExt::ExtData::CreateLight()
 	if (pThis->LightSource)
 	{
 		pThis->LightSource->ChangeLevels(Game::F2I(nLightFactor), nTintBuffer, update);
-		pThis->Radiate();
 	}
-	else
+	else if (auto const pCell = MapClass::Instance->TryGetCellAt(pThis->BaseCell))
 	{
-		auto const pCell = MapClass::Instance->TryGetCellAt(pThis->BaseCell);
 		auto const pLight = GameCreate<LightSourceClass>(pCell->GetCoords(), pThis->SpreadInLeptons, Game::F2I(nLightFactor), nTintBuffer);
 		pThis->LightSource = pLight;
 		pLight->DetailLevel = 0;
 		pLight->Activate(update);
-		pThis->Radiate();
 	}
+
+	pThis->Radiate();
 }
 
 // Rewrite because of crashing craziness

diff --git a/src/Ext/Techno/Body.Update.cpp b/src/Ext/Techno/Body.Update.cpp
@@ -558,14 +558,9 @@ void TechnoExt::ExtData::UpdateLaserTrails()
 		if (pThis->CloakState == CloakState::Cloaked)
 		{
 			if (trail.Type->CloakVisible && trail.Type->CloakVisible_DetectedOnly && !HouseClass::IsCurrentPlayerObserver() && !pThis->Owner->IsAlliedWith(HouseClass::CurrentPlayer))
-			{
-				auto const pCell = pThis->GetCell();
-				trail.Cloaked = !pCell || !pCell->Sensors_InclHouse(HouseClass::CurrentPlayer->ArrayIndex);
-			}
+				trail.Cloaked = !pThis->GetCell()->Sensors_InclHouse(HouseClass::CurrentPlayer->ArrayIndex);
 			else if (!trail.Type->CloakVisible)
-			{
 				trail.Cloaked = true;
-			}
 		}
 
 		if (!this->IsInTunnel)

diff --git a/src/Ext/Techno/Body.cpp b/src/Ext/Techno/Body.cpp
@@ -182,14 +182,11 @@ CoordStruct TechnoExt::PassengerKickOutLocation(TechnoClass* pThis, FootClass* p
 	if (maxAttempts < 1)
 		maxAttempts = 1;
 
-	CellClass* pCell;
-	CellStruct placeCoords = CellStruct::Empty;
-	auto pTypePassenger = pPassenger->GetTechnoType();
-	CoordStruct finalLocation = CoordStruct::Empty;
-	short extraDistanceX = 1;
-	short extraDistanceY = 1;
-	SpeedType speedType = pTypePassenger->SpeedType;
-	MovementZone movementZone = pTypePassenger->MovementZone;
+	const auto pTypePassenger = pPassenger->GetTechnoType();
+	auto placeCoords = CellStruct::Empty;
+	short extraDistance = 1;
+	auto speedType = pTypePassenger->SpeedType;
+	auto movementZone = pTypePassenger->MovementZone;
 
 	if (pTypePassenger->WhatAmI() == AbstractType::AircraftType)
 	{
@@ -198,20 +195,25 @@ CoordStruct TechnoExt::PassengerKickOutLocation(TechnoClass* pThis, FootClass* p
 	}
 	do
 	{
-		placeCoords = pThis->GetCell()->MapCoords - CellStruct { (short)(extraDistanceX / 2), (short)(extraDistanceY / 2) };
-		placeCoords = MapClass::Instance->NearByLocation(placeCoords, speedType, -1, movementZone, false, extraDistanceX, extraDistanceY, true, false, false, false, CellStruct::Empty, false, false);
+		placeCoords = pThis->GetMapCoords() - CellStruct { static_cast<short>(extraDistance / 2), static_cast<short>(extraDistance / 2) };
+		placeCoords = MapClass::Instance->NearByLocation(placeCoords, speedType, -1, movementZone, false, extraDistance, extraDistance, true, false, false, false, CellStruct::Empty, false, false);
 
-		pCell = MapClass::Instance->GetCellAt(placeCoords);
-		extraDistanceX += 1;
-		extraDistanceY += 1;
+		if (placeCoords == CellStruct::Empty)
+			return CoordStruct::Empty;
+
+		const auto pCell = MapClass::Instance->GetCellAt(placeCoords);
+
+		if (pThis->IsCellOccupied(pCell, FacingType::None, -1, nullptr, false) == Move::OK)
+			break;
+
+		extraDistance++;
 	}
-	while (extraDistanceX < maxAttempts && (pThis->IsCellOccupied(pCell, FacingType::None, -1, nullptr, false) != Move::OK) && pCell->MapCoords != CellStruct::Empty);
+	while (extraDistance <= maxAttempts);
 
-	pCell = MapClass::Instance->TryGetCellAt(placeCoords);
-	if (pCell)
-		finalLocation = pCell->GetCoordsWithBridge();
+	if (const auto pCell = MapClass::Instance->TryGetCellAt(placeCoords))
+		return pCell->GetCoordsWithBridge();
 
-	return finalLocation;
+	return CoordStruct::Empty;
 }
 
 bool TechnoExt::AllowedTargetByZone(TechnoClass* pThis, TechnoClass* pTarget, TargetZoneScanType zoneScanType, WeaponTypeClass* pWeapon, bool useZone, int zone)
@@ -246,7 +248,11 @@ bool TechnoExt::AllowedTargetByZone(TechnoClass* pThis, TechnoClass* pTarget, Ta
 			auto cellStruct = MapClass::Instance->NearByLocation(CellClass::Coord2Cell(pTarget->Location),
 				speedType, -1, mZone, false, 1, 1, true,
 				false, false, speedType != SpeedType::Float, CellStruct::Empty, false, false);
-			auto const pCell = MapClass::Instance->GetCellAt(cellStruct);
+
+			if (cellStruct == CellStruct::Empty)
+				return false;
+
+			auto const pCell = MapClass::Instance->TryGetCellAt(cellStruct);
 
 			if (!pCell)
 				return false;

diff --git a/src/Ext/Techno/Hooks.Firing.cpp b/src/Ext/Techno/Hooks.Firing.cpp
@@ -479,7 +479,7 @@ DEFINE_HOOK(0x6FE19A, TechnoClass_FireAt_AreaFire, 0x6)
 				int rand = ScenarioClass::Instance->Random.RandomRanged(0, size - 1);
 				unsigned int cellIndex = (i + rand) % size;
 				CellStruct tgtPos = pCell->MapCoords + adjacentCells[cellIndex];
-				CellClass* tgtCell = MapClass::Instance->GetCellAt(tgtPos);
+				CellClass* tgtCell = MapClass::Instance->TryGetCellAt(tgtPos);
 				bool allowBridges = tgtCell && tgtCell->ContainsBridge() && (pThis->OnBridge || tgtCell->Level + CellClass::BridgeLevels == pThis->GetCell()->Level);
 
 				if (EnumFunctions::AreCellAndObjectsEligible(tgtCell, pExt->CanTarget, pExt->CanTargetHouses, pThis->Owner, true, false, allowBridges))

diff --git a/src/Ext/TechnoType/Body.cpp b/src/Ext/TechnoType/Body.cpp
@@ -124,22 +124,26 @@ TechnoClass* TechnoTypeExt::CreateUnit(TechnoTypeClass* pType, CoordStruct locat
 	HouseClass* decidedOwner = pOwner && !pOwner->Defeated
 		? pOwner : HouseClass::FindCivilianSide();
 
-	auto pCell = MapClass::Instance->GetCellAt(location);
+	auto pCell = MapClass::Instance->TryGetCellAt(location);
 	auto const speedType = rtti != AbstractType::AircraftType ? pType->SpeedType : SpeedType::Wheel;
 	auto const mZone = rtti != AbstractType::AircraftType ? pType->MovementZone : MovementZone::Normal;
 	bool allowBridges = GroundType::Array[static_cast<int>(LandType::Clear)].Cost[static_cast<int>(speedType)] > 0.0;
-	bool isBridge = allowBridges && pCell->ContainsBridge();
+	bool isBridge = allowBridges && pCell && pCell->ContainsBridge();
 	int baseHeight = location.Z;
 	bool inAir = location.Z >= Unsorted::CellHeight * 2;
 
 	if (checkPathfinding && (!pCell || !pCell->IsClearToMove(speedType, false, false, -1, mZone, -1, isBridge)))
 	{
-		auto nCell = MapClass::Instance->NearByLocation(CellClass::Coord2Cell(location),
-			speedType, -1, mZone, isBridge, 1, 1, true,
-			false, false, isBridge, CellStruct::Empty, false, false);
+		auto nCell = MapClass::Instance->NearByLocation(CellClass::Coord2Cell(location), speedType, -1, mZone,
+			isBridge, 1, 1, true, false, false, isBridge, CellStruct::Empty, false, false);
 
-		pCell = MapClass::Instance->TryGetCellAt(nCell);
-		location = pCell->GetCoords();
+		if (nCell != CellStruct::Empty)
+		{
+			pCell = MapClass::Instance->TryGetCellAt(nCell);
+
+			if (pCell)
+				location = pCell->GetCoords();
+		}
 	}
 
 	if (pCell)

diff --git a/src/Ext/TerrainType/Hooks.cpp b/src/Ext/TerrainType/Hooks.cpp
@@ -193,7 +193,7 @@ DEFINE_HOOK(0x48381D, CellClass_SpreadTiberium_CellSpread, 0x6)
 		{
 			unsigned int cellIndex = (i + rand) % size;
 			CellStruct tgtPos = pThis->MapCoords + adjacentCells[cellIndex];
-			CellClass* tgtCell = MapClass::Instance->GetCellAt(tgtPos);
+			CellClass* tgtCell = MapClass::Instance->TryGetCellAt(tgtPos);
 
 			if (tgtCell && tgtCell->CanTiberiumGerminate(pTib))
 			{

diff --git a/src/Misc/Hooks.Crates.cpp b/src/Misc/Hooks.Crates.cpp
@@ -36,6 +36,9 @@ DEFINE_HOOK(0x56BD8B, MapClass_PlaceRandomCrate_Sampling, 0x5)
 		isWater ? SpeedType::Float : SpeedType::Track,
 		-1, MovementZone::Normal, false, 1, 1, false, false, false, true, CellStruct::Empty, false, false);
 
+	if (cell == CellStruct::Empty)
+		return SkipSpawn;
+
 	R->EAX(&cell);
 
 	return SpawnCrate;