Re: Bug in acp-independent action code

[Hans Ronne <hronne at comhem dot se>]

I have now figured out what is going on with your game. It is actually a
quite interesting bug that raises several issues.

What happens is that the last build task before a builder runs out of ores
fails if and only if all ores a consumed in the process. The reason for
this is that in addition to setting unit-consumption-per-cp (which is what
you are supposed to use with advanced units) to (u* ores 1), you have also
set material-to-build to (u* ores 1).

The latter table is not a consumption table but rather sets a minimal
amount of a unique "material" which is required to carry out any building.
It is analogous to material-to-attack and material-to-fire, as opposed to
consumption-per-attack and consumption-per-fire. I don't know if you
remember the discussion about these tables some months ago. Basically, the
materials-to-* tables are supposed to represent non-perishable "materials"
such as guns, as opposed to ammo. I guess that in the building case, it
might represent tools as opposed to raw materials.

Now, I never anticipated that somebody might use this table with the
acp-independent units, and furthermore that the material would be the same
as the one consumed per cp. Acp-independent units differ from normal units
in that building material is consumed before the build task is executed, in
run_construction. This would be risky unless we can be sure that the build
task never fails, which is the normal case for acp-independent units (they
don't use acps and they don't consume materials-per-build, which is what
check_build_action tests for).

However, by using material-to-build and setting it to the same material as
unit-consumption-per-cp, you have discovered a way to make do_build_task
fail for acp-independent units. This is because run-construction uses up
the ores, so there is nothing left when check_build_action checks the very
same material in material-to-build.

So why does it matter if the build task fails? Now, here comes another
interesting point. The task execution code has a safety valve that will
kill any task that fails a certain number of times. There is also a fixed
probability of killing a task that fails irrespective of how many times it
failed previously. Finally, there is a fixed probability of putting the
unit into reserve.

When a build task is killled in your game, building will proceed as normal
after a new build task has been created, using the incomplete unit as
target. However, one consequence of killing the task is that the run
length, which is a task argument, is lost. Which is exactly what you see.

Here is what the task execution code looks like:

      case TASK_FAILED:
        ++task->retrynum;
	DMprintf("failed try %d, ", task->retrynum);
	/* If a task fails, it might be because the task cannot be
	   completed, or just because conditions are temporarily
	   unfavorable, such as a passing unit blocking the way while
	   moving through.  So we need to retry a couple times at
	   least; the variables here control how hard to keep
	   trying. */
	/* (should be doctrine, since these affect human-run units too) */
	if (probability(g_ai_badtask_remove_chance())
	    || task->retrynum >= g_ai_badtask_max_retries()) {
	    pop_task(plan);
	    DMprintf("removed it");
	    /* We might be buzzing, so maybe go into reserve. */
	    if (probability(g_ai_badtask_reserve_chance())) {
		plan->reserve = TRUE;
	    	DMprintf(" and went into reserve");
	    }
	} else {
	    DMprintf("will retry");
	}
	break;

I have long had my doubts about this code. I think that if there is buzzing
because the AI is trying to do something impossible or there is a bug in
the game module, the correct thing to is to fix the bug. Moreover, I doubt
that it is a good idea to have this code execute for human-controlled
units. If their tasks fail, it would be better to prompt the human player
for what to do.

You will notice the gvars g_ai_badtask_remove_chance,
g_ai_badtask_max_retries and g_ai_badtask_reserve_chance. I added them
several years ago because I wanted to be able to turn off this piece of
code (previously, hardcoded numbers were used).

So there are actually three problems at different levels and therefore also
three solutions to your bug.

1. The easiest fix is to get rid of material-to-build, at least until it is
supported for acp-independent units.

2. Another fix would be to move part of the acp-independent build code into
do_build_action, so that materials are consumed only when the action is
executed (i.e. after check_build_action).
This would make sense, and is something I have planned to do. However, it
is not a trivial thing to do since the acp-independent build code differs a
lot from the normal build code.

3. The third fix would be to get rid of the safety valve in task execution
and allow units to retry forever with the failed task. Granted, tasks may
be temporarily impossible, in which case going into reserve and retrying
next turn make sense. However, as you can see in the above code, the unit
goes into reserve only after it killed the task. I think it might make
sense to instead save the task (whether a build task or a move task) and
try again next turn when the conditions have changed.

Hans