[Freeciv-Dev] Re: (PR#2608) replace key_move_xxx with key_unit_move

["Jason Short via RT" <rt@xxxxxxxxxxxxxx>]

On Thu, 2002-12-19 at 20:54, rwetmore@xxxxxxxxxxxx via RT wrote:
> At 11:38 PM 02/12/18 -0800, Jason Short via RT wrote:
> >
> >This patch replaces key_move_xxx functions with a single function
> >key_unit_move.  This new function takes a dir8 parameter giving the
> >_GUI_ direction of the unit's movement.  This GUI direction is converted
> >into a _map_ direction by the backend.
> 
> On the surface there is little difference between  function_param() and 
> function(param) forms except to reduce the number of functions. One just
> moves the constant argument definition up a layer.

Yes.  It does allow a small amount of code to be removed.

> If this reduces switches that index gui_dir to call functions, that would
> make a valid reason. (I note that you can now remove all hardcoded switches 
> below and instead define a data array that maps various keys to enum dirs 
> for a reduction to single line of code (a much simpler form :-):
> 
>   key_unit_move(key_to_guidir[ev-keyval]);
> 
> or with partial filtering flavours like ...
> 
>   if (DIR8_UNKNOWN != (guidir = key_to_guidir[ev-keyval]))
>     key_unit_move(guidir);
> 
> This may make it easier to map keys to arbitrary directions in the future
> on a dynamic basis as opposed to recompiling, just pass a different
> key_to_guidir mapping array.

I do not think this solution is workable.  It hard-codes the order of
the GUI keys.  For instance in GTK there are 4 different keys for each
direction, and no guarantee that they folow a similar ordering or are
even related in order at all.

It also imposes a gui-specific solution on a gui-independent conversion.

> >This provides three advantages:
> >
> >- It gets rid of lots of unnecessary code.
> >
> >- It provides native support for isometric, non-isometric, and (I think)
> >any future view modes we may come up with.  (Ross, if you think this is
> >an insufficient design please propose a better one.)
> 
> I like the sentiment, but haven't drunk enough of the code yet to get
> over the initial bad taste.
> 
> First, the conversion is from enum direction8 to the same enum. I'm
> happy enough to associate the DIR8_XXX enums with GUI directions, but
> think one needs to split map and GUI by creating a corresponding set
> of map enums that are clearly different. I'd do this now to make it
> clear that is what is happening.

I considered an enum gui_direction8 or similar.  But this introduces
extra complexity without really solving anything.

As a comparison, we don't use different types for native/map/gui/index
coordinates - they all just use integers.  A typedef or #define might be
useful in both cases just to help us keep things clear (like
Unit_Type_id does now), but changing the underlying types doesn't help
much.

The current direction8 values correspond to directions on the current
map.  But in a gen-topologies solution, a "map direction" does not
correspond to a real direction.  DIR8_NORTH is not north on the map,
it's northeast.  So we now need three direction systems:

  GUI directions: up, down, left, right, etc.

  MAP directions: north, south, etc.

  NATIVE directions: northeast, southwest, etc.

IMO it is easiest to keep the same underlying enum for all of them. 
This allows us to take advantage of code like dir_cw in the conversions,
at the cost of making things misleading in some cases.

Side note: when playing on an iso-rectangular map with iso-view, the GUI
direction DIR8_NORTH corresponds to the map direction DIR8_NORTHWEST,
which corresponds to the native direction DIR8_NORTH.  Thus "up" on the
GUI is north on the iso-map.

-----

One design question is what changes are needed for it to work with a
hexagonal tile system?  In such a system the current DIR8 directions
could be kept, but some of them would be invalid.  So we would have to
either introduce a DIR8_INVALID or an is_valid_dir() function (for
instance).  This does not seem like a serious design issue at this
point.  This does introduce a 25% inefficiency into loops, but I suspect
that making the directions truly "generalized" would mean an even larger
inefficiency.

> I'm also not completely happy with assigning directions like "NORTH"
> to *any* coordinate axes, because as with the rotation of the iso, 
> DIR8_NORTH is now DIRMAP_NORTHWEST, and the compass direction is 
> becoming a very fuzzy concept. I can relate to GUI NORTH as "up"
> on a cartesian screen (i.e. (0,-1)) but this is because of standard
> map convention. Standard/internal map NORTH which may be NORTHWEST or 
> who knows what depending on the topology and other factors such as
> how best to represent it in a cartesian grid, is where I start to 
> lose it. 

An alternative is to introduce a new enumeration:

enum gui_direction8 {
  GUIDIR8_UP,
  GUIDIR8_UPLEFT,
  GUIDIR8_LEFT,
  /* ... */
  GUIDIR8_UPRIGHT
};

However, note that introducing this is a significant change in itself
since there are many places where it should be used in place of the
current directional system.

> So a clear definition of what directions mean in each type of coordinate 
> system with the sorts of mappings you are starting on here prominently 
> documented as the way to transform things from one to the other, may 
> help people who see NORTH and think that means something constant 
> everywhere - they need to at least think ISO-NORTH, GUI-NORTH etc. 
> instead, or one gets rid of the NORTH labels to avoid linkage.
> 
> I definitely think the gui_to_map_dir() and map_to_gui_dir() 
> transformations should end up in a "GUI_map".h (currently tilespec.h
> is the dumping ground for gui coordinate stuff, at least it has the
> DIR4_XXX and if DIR8_XXX are GUI they should move out of map.h to a
> GUI location - the corecleanups really abused tilespec.h. but CVS 
> can get it right if it starts early).

tilespec.c is currently very overloaded:

- Tilespec code.
- Drawing code.
- Map code (although not much).

> >- It provides the beginnings of a distinction between GUI and map
> >directions.  This can be helpful in many places in the current code
> >(like this one!), and provides a lot of flexibility for future changes. 
> >See
> >http://lists.complete.org/freeciv-dev@xxxxxxxxxxx/2002/01/msg00547.html.gz
> >
> >One counter-argument could be that the key_move_xxx functions take no
> >parameters and are easily used directly as callback functions by the
> >GUI.  But currently no GUI does this - even XAW which has specific
> >callback functions for each key command needs a different prototype for
> >them, and uses wrappers.  So this is a non-issue.
> 
> Because one can always use wrappers if needed (and may need multiple 
> wrappers for flexibility), plus there are corresponding arguments (like 
> switch reduction) on the otherside, one may end up with both flavours
> with the current suggestion the lower layer. This change is then one of
> first consolidation and second elimination of (current) dead code, but
> not precluding adding new wrappers in the future. This is good ...
> 
> >The argument that this method is inefficient is also pointless.  dir_ccw
> >is efficient (and could be made more so in the future), and in any case
> >this code is not called intensively.
> 
> Initial implementation should probably be as macros in tilespec.h or the
> *correct* file location. The "not intensively" is never a good argument 
> if there are reasonable efficient alternatives. If one doubles the cost
> of every snippet of code one writes, then soon the program will be twice
> as slow, and soon thereafter twice as slow again and I have noticed you 
> always use this same argument with every checkin :-)

There is no need for a macro.  The code is efficient; the only
inefficiency is that an extra function call is needed.  And I do think
that macros should only be introduced for "intensively" called code, not
based purely on the percentage of time that would be saved.

jason