tomjn / Shard

A reprogrammable AI framework for the Spring Engine
https://shard.tomjn.com
17 stars 13 forks source link

game getUnitByID() c++ api problem in native env #137

Closed pandaro closed 7 years ago

pandaro commented 7 years ago

the function getUnitByID() wont work too. he stuck the engine, the cpu run and no value is returned. UPDATE: i have tested other function: ai.game:GameName() ai.game:GetResourceCount() and works perfectly, There are something strictly regard ai.game:GetUnitByID() that call: /src/API/interfaces/IGame.h i think, and there are a problem, or i mistake.

tomjn commented 7 years ago

getUnitByID is only used in the BA config, but the reason that callin was added was for C++ usage not lua.

Shard configs should never have to deal with unit ID numbers, they should be dealing with unit objects

For example:

https://github.com/tomjn/Shard/blob/bac1103ab4bd16611d6a83a04d591b45a047a012/data/ai/BA/bombardbehaviour.lua#L51

Here we see the target handler returning a bombard cell:

            local bestCell, valueThreat, buildingID = ai.targethandler:GetBestBombardCell(self.position, self.range, valueThreatThreshold)
            if bestCell then
                local newTarget
                if buildingID then
                    local building = game:GetUnitByID(buildingID)
                    if building then
                        newTarget = building:GetPosition()
                    end
                end

The third return value is the ID of a unit, but this shouldn't be a unit ID at all. It should be the position of that unit. The Getposition() call should happen inside GetBestBombardCell.

This also means that targethandler hasn't been built correctly:

https://github.com/tomjn/Shard/blob/bac1103ab4bd16611d6a83a04d591b45a047a012/data/ai/BA/targethandler.lua#L464

                if unitTable[name].isBuilding then
                    table.insert(cell.buildingIDs, e.unitID)
                end

Here, it's e not e.unitID that should be tracked, and it should be cell.buildings not cell.buildingIDs. target handler should also clean up after itself when units are destroyed

tomjn commented 7 years ago

As for why this might cause a hang:

https://github.com/tomjn/Shard/blob/master/src/API/spring/SpringGame.cpp#L351

IUnit* CSpringGame::getUnitByID( int unit_id ) {
    return ai->GetGame()->getUnitByID( unit_id );
}

https://github.com/tomjn/Shard/blame/master/src/API/spring/SpringGame.h#L61

A year ago @abma added the override keyword, I'm unsure why, but the reported issues suggest that the native Shard is going into a loop here calling the IUnit* version repeatedly, never accessing the version that returns CSpringUnit*. Perhaps @abma can shed some light on why that change was made and what problem it fixed

tomjn commented 7 years ago

Try building the latest code, I replaced that methods contents with the other so that loops aren't a factor

abma commented 7 years ago

https://stackoverflow.com/questions/13880205/is-the-override-keyword-just-a-check-for-a-overridden-virtual-method

override didn't change anything here. Its just there that the compiler can show an error when the function doesn't override for some reason. It was added because the function already override'd a function else an error would be shown.