Angband.oook.cz
Angband.oook.cz
AboutVariantsLadderForumCompetitionComicScreenshotsFunniesLinks

Go Back   Angband Forums > Angband > Development

Reply
 
Thread Tools Display Modes
Old April 15, 2018, 23:50   #1
Gordon
Scout
 
Join Date: Jan 2010
Posts: 31
Gordon is on a distinguished road
Bug in cmd_get_direction() cmd_core.c

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():

Code:
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:

Code:
		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.

Code:
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.

Last edited by Gordon; April 16, 2018 at 00:00.
Gordon is offline   Reply With Quote
Old April 17, 2018, 01:03   #2
Gordon
Scout
 
Join Date: Jan 2010
Posts: 31
Gordon is on a distinguished road
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:

Code:
	/* 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():

Code:
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 is offline   Reply With Quote
Old April 17, 2018, 01:32   #3
Gordon
Scout
 
Join Date: Jan 2010
Posts: 31
Gordon is on a distinguished road
Since everything works fine with the validity check disabled, perhaps the best solution here is to remove the validity check entirely:

Code:
/**
 * 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 is offline   Reply With Quote
Old April 18, 2018, 00:05   #4
Gordon
Scout
 
Join Date: Jan 2010
Posts: 31
Gordon is on a distinguished road
A more elegant solution would be to add a new value to this enum:

Code:
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.
Gordon is offline   Reply With Quote
Old April 18, 2018, 01:47   #5
Nick
Vanilla maintainer
 
Nick's Avatar
 
Join Date: Apr 2007
Location: Canberra, Australia
Age: 53
Posts: 7,188
Donated: $60
Nick is on a distinguished road
Thanks for all the analysis here - I will look at including this in 4.1.3 when that happens.
__________________
One for the Dark Lord on his dark throne
In the Land of Mordor where the Shadows lie.
Nick is offline   Reply With Quote
Reply


Currently Active Users Viewing This Thread: 1 (0 members and 1 guests)
 
Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off

Forum Jump

Similar Threads
Thread Thread Starter Forum Replies Last Post
3.5 dev bug: refresh bug at edge of labyrinth maps Nomad Vanilla 5 July 29, 2013 14:37
Bug or no bug: Mushrooms identified immediately Zababa Vanilla 2 June 16, 2010 02:32


All times are GMT +1. The time now is 23:07.


Powered by vBulletin® Version 3.8.11
Copyright ©2000 - 2018, vBulletin Solutions Inc.