[Freeciv-Dev] Re: client/tilespec.[ch] cleanup

[Jason Short <vze2zq63@xxxxxxxxxxx>]

Vasco Alexandre Da Silva Costa wrote:

> Hello,
>
> I've cleaned this up a bit and removed the dir8_to_dir4() function.
> My cleanup target was fill_tile_sprite_array_iso() so if you probably
> should check that one out.
>
> Comments?
All changes look sane, but I'm no expert on this code (although I did 
work on it a bit a while ago).
In general, I would suggest making fill_tile_sprite_array and 
fill_tile_sprite_array_iso look as much alike as possible.  It might 
even be possible to make them identical (i.e. merge them), although I 
doubt this.  You've removed SAFE_MAPSTEP from one function but not the 
other, which makes them vary at that point.

As Raimar says, SAFE_MAPSTEP is not used anywhere else.  This macro is 
"dangerous" because it should not be used by code that doesn't know what 
its doing (the "SAFE" is misleading here), so I think we agreed to at 
least remove it from map.h.  This doesn't directly relate to the changes 
you've made, though...it'd be easy enough to just move it to tilespec.c 
at any time.  But if you remove it altogether, so much the better.

Whether it's better to go dir8_to_dir4 or dir4_to_dir8 is mostly an 
academic question.  But I do see that using a function to do so may give 
a performance problem; this is time-critical code (although I have no 
idea how fast it is now).  An array can be used for either one (it's 
slightly cleaner for dir4_to_dir8, as you've found), but in each case 
you give up error checking to do so.  If it were made a macro the 
checking could be included:
#define DIR4_TO_DIR8(dir4)        \
  (assert(dir4 >= 0 && dir4 < 4), \
   dir4_to_dir8[dir4])

but such checking probably isn't really necessary, so whatever.  The 
array method is a bit cleaner under dir4_to_dir8 (once you're willing to 
make the necessary changes to the code to get this to work, as you've done).

Note that the DIR4_TO_DIR8 array hard-codes the DIR4 directions.  I'd 
recommend tagging a comment by the DIR4 enum to label this.

Just an idea: if you're concerned with pure speed, rather than use 
simple loops like
  for (dir = 0; dir < 8; dir++)
    tspecial_near[dir] = S_NO_SPECIAL;

  adjc_dir_iterate(x, y, x1, y1, dir) {
    tspecial_near[dir] = map_get_special(x1, y1);
  } adjc_dir_iterate_end;

you could put everything into one ugly loop:

  for (dir = 0; dir < 8; dir++) {
    int x1, y1;
    if (MAPSTEP(x1, y1, x, y, dir))
      tspecial_near[dir] = map_get_special(x1, y1);
    else
      tspecial_near = S_NO_SPECIAL;
  }

This could also be done as an "else" within the adjc_dir_iterate itself 
(you may have to change the macro accordingly), but it is by far better 
to avoid such constructs IMO.
jason