ihhub / fheroes2

fheroes2 is a recreation of Heroes of Might and Magic II game engine.
https://ihhub.github.io/fheroes2/
GNU General Public License v2.0
2.69k stars 375 forks source link

Game crash during AI turn on failed assertion on _pathStart #8809

Closed Canop closed 3 months ago

Canop commented 3 months ago

Preliminary checks

Platform

Linux

Describe the bug

The game crashes on a failed assertion:

fheroes2: ../fheroes2/world/world_pathfinding.cpp:699: virtual void AIWorldPathfinder::processWorldMap(): Assertion `_cache.size() == world.getSize() && Maps::isValidAbsIndex( _pathStart )' failed.

To reproduce: load the provided file, then end the turn.

What seems to happen: In a Kingdom turn,

  1. the AI computes the _enemyArmies map
  2. The AI hero Celia kills the AI hero Vatawna which was defending a castle
  3. Vatawna is thus dismissed, which sets its index to -1
  4. The Atlas hero looks for a target (getPriorityTarget)
  5. As the _enemyArmies map hasn't been recomputed or updated, Atlas finds Vatawna
  6. When pathfinding, as Vatawana has a -1 index, the _pathStart is invalid, hence the failed assertion

Workaround: In ai_normal_hero.cpp:2143 change

            if ( enemyArmy.hero == nullptr) {

to

            if ( enemyArmy.hero == nullptr || !enemyArmy.hero->isActive()) {

Save file

AUTOSAVE.zip

Additional info

No response

Canop commented 3 months ago

I can make a PR with the workaround I mention, or we may look for another solution (maybe update _enemyArmies on a hero being dismissed, assuming I correctly understood what happens ?)

ihhub commented 3 months ago

We root cause of this assertion is that we do not update priority targets properly for AI after the action within Normal::updatePriorityTargets() method. More specifically, _enemyArmies is not updated. We must erase tile ID after capturing a town which we don't do. In this example the AI tries to clear a DEFEND task under if ( hero.GetIndex() != tileIndex ) { condition. I think this comment is not valid:

// Either the castle has just been captured, or the hero meets the guest hero of a friendly castle. No task should be updated.
// If any of these assertions blow up, then this is not one of these cases.
ihhub commented 3 months ago

It looks like a regression from #7379.

ihhub commented 3 months ago

Hi @idshibanov and @oleg-derevenetz , I would like to hear your opinion about a possible solution. One of them to simply add _enemyArmies.erase( tileIndex ); but I have a feeling it is not a proper solution. What do you think?