Bug in cmd_get_direction() cmd_core.c

[Gordon]

While going through the 4.0.5 code eliminating warnings (most of which are due to an evident complete disdain for type casting), I came across a bug in cmd_get_direction():

int cmd_get_direction(struct command *cmd, const char *arg, int *dir,
					  bool allow_5)
{
	if (cmd_get_arg_direction(cmd, arg, dir) == CMD_OK) {
		/* Validity check */
		if (dir != DIR_UNKNOWN)
			return CMD_OK;
	}

	/* We need to do extra work */
	if (get_rep_dir(dir, allow_5)) {
		cmd_set_arg_direction(cmd, arg, *dir);
		return CMD_OK;
	}

	cmd_cancel_repeat();
	return CMD_ARG_ABORTED;
}

The validity check will always pass here as it is checking for a null pointer rather than a value of zero for dir. I corrected this:

		if (*dir != DIR_UNKNOWN)

But the result wasn't pretty. Whenever I used a mouse click to move @ I would get a direction prompt followed by multiple graphical images of @ along the direction of motion. I'm not sure how to debug this since it gets into sections of the code that I don't fully understand, but I found the way to solve (hack) the problem was to just forego the validity check altogether.

int cmd_get_direction(struct command *cmd, const char *arg, int *dir,
					  bool allow_5)
{
	if (cmd_get_arg_direction(cmd, arg, dir) == CMD_OK) {
		/* Validity check */
		// if (*dir != DIR_UNKNOWN)
			return CMD_OK;
	}

	/* We need to do extra work */
	if (get_rep_dir(dir, allow_5)) {
		cmd_set_arg_direction(cmd, arg, *dir);
		return CMD_OK;
	}

	cmd_cancel_repeat();
	return CMD_ARG_ABORTED;
}

I downloaded the code for 4.1.2 and confirmed that this bug is still present there.

[Gordon]

A bit more information on this. It seems to be a feature of the run command as it happens when I press '.' also. The problem occurs because function run_step() sets up a call to do_cmd_run() and then sets the direction to zero at the end:

	/* Move the player; running straight into a trap == trying to disarm */
	move_player(run_cur_dir, dir ? TRUE : FALSE);

	/* Prepare the next step */
	if (player->upkeep->running) {
		cmdq_push(CMD_RUN);
		cmd_set_arg_direction(cmdq_peek(), "direction", 0);
	}
}

When do_cmd_run() is called, it calls cmd_get_direction():

void do_cmd_run(struct command *cmd)
{
	int x, y, dir;

	/* Get arguments */
	if (cmd_get_direction(cmd, "direction", &dir, FALSE) != CMD_OK)
		return;

	if (player_confuse_dir(player, &dir, TRUE))
		return;

	/* Get location */
	if (dir) {
		y = player->py + ddy[dir];
		x = player->px + ddx[dir];
		if (!do_cmd_walk_test(y, x))
			return;
	}

	/* Start run */
	run_step(dir);
}

Then since dir is now zero, the validity check fails and get_rep_dir() prompts for a direction again. This repeats over and over since do_cmd_run() calls run_step() again.

[Gordon]

Since everything works fine with the validity check disabled, perhaps the best solution here is to remove the validity check entirely:

/**
 * Get a direction, first from command or prompt otherwise
 */
int cmd_get_direction(struct command *cmd, const char *arg, int *dir,
					  bool allow_5)
{
	if (cmd_get_arg_direction(cmd, arg, dir) == CMD_OK) 
		return CMD_OK;
	

	/* We need to do extra work */
	if (get_rep_dir(dir, allow_5)) {
		cmd_set_arg_direction(cmd, arg, *dir);
		return CMD_OK;
	}

	cmd_cancel_repeat();
	return CMD_ARG_ABORTED;
}

[Gordon]

A more elegant solution would be to add a new value to this enum:

enum {
	DIR_UNKNOWN = 0,
	DIR_NW = 7,
	DIR_N = 8,
	DIR_NE = 9,
	DIR_W = 4,
	DIR_TARGET = 5,
	DIR_NONE = 5,
	DIR_E = 6,
	DIR_SW = 1,
	DIR_S = 2,
	DIR_SE = 3,
};

DIR_PATH say. Then use this in run_step() to indicate a continuation of a run in progress rather than 0. This might take significant recoding however.

[Nick]

Thanks for all the analysis here - I will look at including this in 4.1.3 when that happens.