Call to TheTerrainVisual->addFactionBib is potentially missing in BuildAssistant::isLocationClearOfObjects

[xezon]

Call to TheTerrainVisual->addFactionBib is potentially missing in BuildAssistant::isLocationClearOfObjects.

		//Kris: Patch 1.01 - November 5, 2003
		//Prevent busy units (black lotus hacking from being moved by trying to place a building -- exploit).
		if (rel == ALLIES)
		{
			if (them->testStatus(OBJECT_STATUS_IS_USING_ABILITY) || them->getAI() && them->getAI()->isBusy())
			{
				// --> Needs to call TheTerrainVisual->addFactionBib( them, TRUE ) here?
				return LBC_OBJECTS_IN_THE_WAY;
			}
		}

[Stubbjax]

I've had this on my to-do list for a while. My only hesitation is the existing violation of the single responsibility principle.

The function name isLocationClearOfObjects suggests that it's a query function that checks state, yet it also comes with the side effect of modifying (visual) state. Functions should ideally either return information or modify state, not both.

A better approach would be to move all instances of addFactionBib from these functions to their respective call sites, for example:

if (isLocationClearOfObjects() == LBC_OBJECTS_IN_THE_WAY)
{
  TheTerrainVisual->addFactionBib();
}

This would also solve the visual issues caused by calling this function via game logic pathways, which can leave bibs stuck on the terrain.

[xezon]

That is a very valid concern and I agree that this ideally was a const function and modified no such state.