Bug in cmd_get_direction() cmd_core.c

Collapse
X
Collapse
+ More Options
Posts
 
  • Time
  • Show
Clear All
new posts
  • Gordon
    Scout
    • Jan 2010
    • 31

    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 15, 2018, 23:00.
  • Gordon
    Scout
    • Jan 2010
    • 31

    #2
    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.

    Comment

    • Gordon
      Scout
      • Jan 2010
      • 31

      #3
      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;
      }

      Comment

      • Gordon
        Scout
        • Jan 2010
        • 31

        #4
        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.

        Comment

        • Nick
          Vanilla maintainer
          • Apr 2007
          • 9364

          #5
          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.

          Comment

          Working...
          ๐Ÿ˜€
          ๐Ÿ˜‚
          ๐Ÿฅฐ
          ๐Ÿ˜˜
          ๐Ÿคข
          ๐Ÿ˜Ž
          ๐Ÿ˜ž
          ๐Ÿ˜ก
          ๐Ÿ‘
          ๐Ÿ‘Ž
          โ˜•