[Freeciv-Dev] Re: PATCH: cleanup in fill_tile_sprite_array(PR#1016)

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

"Ross W. Wetmore" wrote:
> 
> Why do you *always* go for the O(n) solution to any problem?
> Is this a conspiracy to slow down the codebase?

Technically, this solution is O(8)==O(1) since n==8 in all cases.  Any
future change to extend the directional system (as you have proposed
separately) will have to clean this code up anyway.

> In this case do the computation once and stash it in an array.
> Then do an array lookup.

Do you have actual evidence that that is faster?  Several comparisons
are necessary to look up an index in an array, whereas Raimar's code
makes 4.5 comparisons to constant values (memory positions) on average.

> If you put this where the enums are assigned, then you can even
> define it statically with a note to make sure you change the two
> elements in tandem.
> 
> Every time you add one of the "little" inefficiencies you are
> killing the performance. It may be the death of a thousand cuts,
> but you seem to be very prolific at cutting, and the fixes will
> be a thousand fixes to recover.

One at a time...

I wasn't entirely happy with the CW/CCW code either, but (for the above
reasons) I didn't think it was worth arguing about.

jason



The code in question was:

> At 05:36 PM 01/10/17 -0400, Jason Dorje Short wrote:
> >Jason Dorje Short wrote:
> >>
> >> Raimar Falke wrote:
> >Index: common/map.h
> >===================================================================
> >RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
> >retrieving revision 1.99
> >diff -u -r1.99 map.h
> >--- common/map.h       2001/10/15 13:42:51     1.99
> >+++ common/map.h       2001/10/17 21:32:57
> >@@ -220,6 +220,34 @@
> >      (dest_y) += (src_y),                             \
> >      normalize_map_pos(&(dest_x), &(dest_y)))
> >
> >+/*
> >+ * Returns the next direction clock-wise
> >+ */
> >+#define DIR_CW(dir) \
> >+  ((dir)==DIR8_WEST ? DIR8_NORTHWEST : \
> >+   ((dir)==DIR8_EAST ? DIR8_SOUTHEAST : \
> >+    ((dir)==DIR8_NORTH ? DIR8_NORTHEAST : \
> >+     ((dir)==DIR8_SOUTH ? DIR8_SOUTHWEST : \
> >+      ((dir)==DIR8_NORTHWEST ? DIR8_NORTH : \
> >+       ((dir)==DIR8_NORTHEAST ? DIR8_EAST : \
> >+        ((dir)==DIR8_SOUTHWEST ? DIR8_WEST : \
> >+         ((dir)==DIR8_SOUTHEAST ? DIR8_SOUTH : \
> >+         (dir)/0))))))))
> >+
> >+/*
> >+ * Returns the next direction counter-clock-wise
> >+ */
> >+#define DIR_CCW(dir) \
> >+  ((dir)==DIR8_WEST ? DIR8_SOUTHWEST : \
> >+   ((dir)==DIR8_EAST ? DIR8_NORTHEAST : \
> >+    ((dir)==DIR8_NORTH ? DIR8_NORTHWEST : \
> >+     ((dir)==DIR8_SOUTH ? DIR8_SOUTHEAST : \
> >+      ((dir)==DIR8_NORTHWEST ? DIR8_WEST : \
> >+       ((dir)==DIR8_NORTHEAST ? DIR8_NORTH : \
> >+        ((dir)==DIR8_SOUTHWEST ? DIR8_SOUTH : \
> >+         ((dir)==DIR8_SOUTHEAST ? DIR8_EAST : \
> >+         (dir)/0))))))))
> >+
> > struct city *map_get_city(int x, int y);
> > void map_set_city(int x, int y, struct city *pcity);
> > enum tile_terrain_type map_get_terrain(int x, int y);