Yesterday was a productive day for fixing bugs, thanks to info from Bruce Nielsen, author of Locutus. See comments to Steamhammer 1.4.3 change list. Because tournaments are coming up, it may be a while before I release another public version of Steamhammer, but bugfixes are important for everyone with a fork, short of the devil.
After fixing these bugs, I can no longer reproduce the “wrong reserves” error messages that the building manager used to kick out several times per game. Other mysterious misbehavior is probably fixed too, though I’m not sure what.
BuildingManager shouldn’t copy buildings
The methods BuildingManager::validateWorkersAndBuildings() and BuildingManager::checkForCompletedBuildings() loop through the vector of Building data structures and delete those that are no longer needed. To avoid deleting from the vector of Building objects while in the midst of looping through it, it stashes the buildings to delete in another vector toRemove and passes toRemove to a deletion method.
std::vector<Building> toRemove;
Well, it’s copying the building data. I haven’t uncovered any bug that this causes, but it is wasteful at best and at worst an invitation to errors. If code updates the copies, or updates the real buildings before the copies are sent in for deletion, no real building or the wrong real building might be deleted because of how equality is defined in the Building class. undoBuildings() has to undo data structure dependencies and could easily make such a mistake.
There are several steps to the fix, but the idea is to keep a vector of references instead of a vector of copies. To keep references in a Standard Template Library container, we have to uglify the type signature with a wrapper.
std::vector< std::reference_wrapper<Building> > toRemove;
The rest of the code doesn’t change, only the declaration. Then we also have to update the declarations of the deletion routines that undo data structure dependencies and perform the deletion.
void undoBuildings(const std::vector< std::reference_wrapper<Building> > & toRemove);
void removeBuildings(const std::vector< std::reference_wrapper<Building> > & toRemove);
canceling buildings is incorrect
BuildingManager::cancelQueuedBuildings() and BuildingManager::cancelBuildingType() also delete unwanted buildings, and do it while looping through the vector of buildings. It is unsafe and can cause errors. It’s my fault, I wrote these. I rewrote them like this:
// It's an emergency. Cancel all buildings which are not yet started.
void BuildingManager::cancelQueuedBuildings()
{
std::vector< std::reference_wrapper<Building> > toCancel;
for (Building & b : _buildings)
{
if (b.status == BuildingStatus::Unassigned || b.status == BuildingStatus::Assigned)
{
toCancel.push_back(b);
}
}
for (Building & b : toCancel)
{
cancelBuilding(b);
}
}
and
// It's an emergency. Cancel all buildings of a given type.
void BuildingManager::cancelBuildingType(BWAPI::UnitType t)
{
std::vector< std::reference_wrapper<Building> > toCancel;
for (Building & b : _buildings)
{
if (b.type == t)
{
toCancel.push_back(b);
}
}
for (Building & b : toCancel)
{
cancelBuilding(b);
}
}
releasing workers from a squad
I realized I was not sensitive enough to this category of bug, and surveyed the codebase to see whether there were any more. Almost all risky loops were correct, whether written by Dave Churchill or by me, but I found 2 more. One was inconsequential because it was in the BuildingData class, which is unused (so I deleted it). The other was in Squad::releaseWorkers() and should be fixed.
// Remove all workers from the squad, releasing them back to WorkerManager.
void Squad::releaseWorkers()
{
for (auto it = _units.begin(); it != _units.end(); )
{
if (_combatSquad && (*it)->getType().isWorker())
{
WorkerManager::Instance().finishedWithWorker(*it);
it = _units.erase(it);
}
else
{
++it;
}
}
}
finding the next expansion to take
Bruce also pointed out an unrelated typo in MapTools::nextExpansion(). It's a serious bug. Here is the Locutus commit fixing it.
Change this:
for (int x = 0; x < player->getRace().getCenter().tileWidth(); ++x)
{
for (int y = 0; y < player->getRace().getCenter().tileHeight(); ++y)
{
if (BuildingPlacer::Instance().isReserved(x,y))
{
// This happens if we were already planning to expand here. Try somewhere else.
buildingInTheWay = true;
break;
}
To this:
for (int x = 0; x < player->getRace().getCenter().tileWidth(); ++x)
{
for (int y = 0; y < player->getRace().getCenter().tileHeight(); ++y)
{
if (BuildingPlacer::Instance().isReserved(tile.x + x, tile.y + y))
{
// This happens if we were already planning to expand here. Try somewhere else.
buildingInTheWay = true;
break;
}
x and y were treated as absolute map coordinates instead of relative to the building location, so the check gave completely wrong results. The next check, immediately below, is correct; the bug is a typo. But the social status of the bug does not matter. What matters is that it broke stuff.