fix for game-breaking issue - serfs occasionally become stuck in StateWaitIdleOnPath and are never freed

[tlongstretch]

This happens occasionally, often enough it was a major game-breaking problem when I was working on my AI. I had put in some code in my AI to detect the state and work-around it by "booting" and serf that was in StateWaitIdleOnPath for too long by changing it to StateLost, un-jamming the road but sometimes causing other issues (the serf never gets to wherever he was going, meaning a building might not become occupied)

Anyway, I finally took the effort to track this down and I think I found the issue: Serf::change_transporter_state_at_pos doesn't actually check to see if a given serf at a flag pos is a Transporter, so any serf in one of the four wait states it applies to is a non-transporter, they are put into SateWaitIdleOnPath and can never be freed of it, because only Transporters should be in this state (because the transporter call logic is the only wait out of the state)

To fix I added to Serf::change_transporter_state_at_pos a simple if type == transporter check

/* Find a transporter at pos and change it to state. */
static int
change_transporter_state_at_pos(Game *game, MapPos pos, Serf::State state) {
for (Serf *serf : game->get_serfs_at_pos(pos)) {
// originally this function was trying to call change_transporter_state_at_pos on ANY serf
// at the post that had one of the four wait states, occasionally causing non-transporters to become stuck forever
if (!serf->get_type() == Serf::TypeTransporter)
continue;
if (serf->change_transporter_state_at_pos(pos, state)) {
return serf->get_index();
}
}

return -1;
}

[p1plp1]

hmm maybe better testing if serf is transporter && change transporter state -
In C and C++, the && and || operators "short-circuit". That means that they only evaluate a parameter if required. If the first parameter to && is false, or the first to || is true, the rest will not be evaluated:

static int
change_transporter_state_at_pos(Game *game, MapPos pos, Serf::State state) {
for (Serf *serf : game->get_serfs_at_pos(pos)) {
if ((serf->get_type() == Serf::TypeTransporter) && (serf->change_transporter_state_at_pos(pos, state)))
return serf->get_index();
}
return -1;
}

[p1plp1]

but yes this fixes that problem with broken transporting

[tlongstretch]

I am thinking this may be the actual cause:

Serf::change_transporter_state_at_pos(MapPos pos_, Serf::State _state) {
  if (pos == pos_ &&
      (_state == StateWakeAtFlag || _state == StateWakeOnPath ||
       _state == StateWaitIdleOnPath || _state == StateIdleOnPath)) {
    set_state(_state);
    return true;
  }

I am thinking that it may be intended to check the serf's CURRENT STATE before assigning the new state, to make sure it is a state that only a transporter working this road could have. Instead, it is checking the NEWLY ASSIGNED state, as if intended to prevent programmer errors calling the function to invalid states

I am removing the if(transporter) check and instead trying this:

Serf::change_transporter_state_at_pos(MapPos pos_, Serf::State _state) {
  if (pos == pos_ &&
      (state == StateWakeAtFlag || state == StateWakeOnPath ||
       state == StateWaitIdleOnPath || state == StateIdleOnPath)) {
    set_state(_state);
    return true;
  }

The problem with the if(transporter) check is that it could be triggering on an unrelated transporter serf that just happens to be walking on the road in the same pos as an idle transporter. So it still happened occasionally but less often