[Freeciv-Dev] bugs and problems (PR#1040)

[jdorje@xxxxxxxxxxxxxxxxxxxxx]

While playing around with topologies where normal!=regular, I've found
some weird behavior.  Some of it is currently correct (but doesn't work
with different topologies), while some seems to be a part of subtle and
little-occuring bugs.


Here's the biggest thing: when a unit is created, its goto_dest_x and
goto_dest_y are initialized to 0.  Later, these values are used when
they really shouldn't be.  Two examples of this are in
ai_manage_ferryboat() [1] and another in is_already_assigned [2] (this
latter one has a comment from Syela "I'm still not sure this is exactly
right").  First of all, this will not work with topologies where
regular!=normal (the check_map_pos change does its job well in catching
the bug).  Secondly, it could lead to weird bugs in cases where the
check for (0,0) actually matches (for instance, the unit may think it is
ferrying to whatever city happens to lie there - I'm not exactly sure). 
It's easy to fix the problem by doing a check for is_normal_map_pos
before using the coordinates, but fixing the bug is slightly trickier. 
My best idea is to make goto_dest a pointer to a struct map_position,
that is initialized as NULL and later set up properly (and maybe
reverted to NULL later).  Another idea would be to have a separate flag
indicating whether the goto coordinates had any validity.  Using (-1,
-1) as a target to indicate invalid coordinates is not a good solution
(see below).

There is a somewhat similar problem in some of the server's plrhand.c
code.  Here it's not a bug, but will become a problem once topologies
may span all coordinates.  The function vnotify_conn_ex [3] takes a
coordinate pair (x, y).  Several pieces of code pass in (-1, -1) to
indicate no coordinates.  On a torus map, these coordinates are real and
so the check within the function behaves wrongly.  This is easy enough
to correct by checking for normal coordinates instead of real ones, but
I do not think this is the correct fix.  A better solution that I've
used is passing in a pointer to a struct map_position.  NULL is passed
in to indicate no coordinates.  I haven't looked at this all that
deeply, though - the coordinates are translated and passed on, and I'm
not sure what will happen to them then.  It's also likely that more
places like this exist in the code.


I haven't looked too deeply at any of this; I've just done the simple
fix and move on.  Hopefully someone more familiar with this code can
come up with (or point me toward) the correct fix.

jason

[1] http://www.freeciv.org/lxr/source/ai/aiunit.c#L1680
[2] http://www.freeciv.org/lxr/source/server/settlers.c#L398
[3] http://www.freeciv.org/lxr/ident?i=vnotify_conn_ex