[Freeciv-Dev] Re: normalize_map_pos and invalid map positions

[Jason Short <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

Mike Kaufman wrote:
> On Fri, Aug 09, 2002 at 06:04:36PM +0000, Per I. Mathisen wrote:
>
> > Ok, this is probably a stupid question, but anyway.
> >
> > I take it the point of normalize_map_pos is to ensure the given map
> > positions don't point to an illegal point on the map that would case an
> > error.
> >
> > However, sometimes, as with goto_dest_?, I believe the correct thing to do
> > is to make the positions illegal, so that they don't get used and if they
> > get used, an error is generated so that the problem can be fixed.
> >
> > So what would be an illegal map position? (-1, -1)?
Passing around illegal map positions is generally a bad idea.  Back 
before normalize_map_pos, there was a void_tile which was the tile all 
invalid coordinates pointed to.  Although this may sound like a good 
idea, it (supposedly) lead to all sorts of hard-to-track errors 
downstream.  These days illegal map positions are usually detected and 
dealt with on creation - for instance in the iterator macros they are 
just skipped over.  But there are some places where this is difficult.
Using (-1,-1) as an "illegal" map position is already done: for 
instance, vnotify_conn_ex() takes (-1,-1) as the position indicating 
that a given event has no valid position.  This can be made to work so 
long as (-1,-1) is not a *normal* map position.  Soon it will be a 
*real* map position, but as long as all valid coordinates used are 
*normal* we can tell (-1,-1) from a valid position.
Elsewhere in the code, the goto_dest_x and goto_dest_y fields of a unit 
are initialized to 0, and only later are they correctly set.  If you 
were to play on a map where (0,0) is a worthwhile position, I don't know 
if there would be problems.
Both of these situations are both ugly and dangerous.  A better solution 
would be desirable.
> urk, -1,-1 is (will be) a valid position. (at least that's my
> understanding of the whole thing...)
> The solution should be to add an illegal position bit?
>
> example: a unit at (0,0) is ordered to move "northwest" by:
> a) the server (by proxy of the ai)
> b) a remote client.
>
> What should ideally happen in a general topological framework?
Assuming we're talking about a torus map (wraps in both directions), the 
coordinates of the destination should have been normalized at startup. 
So for instance on a 100x80 map, a unit at (0,0) who moves northwest 
will move to position (99,79).  Using (-1,-1) as a special-case would be 
workable (but again a hack).
The problem comes in when we consider worst-case scenarios.  Suppose a 
particular piece of code does not correctly normalize the coordinates 
before passing them in.  Usually we could just normalize them and be 
done with it, but in this case that's dangerous.  If (-1,-1) is passed 
in we cannot tell if we should normalize or treat it as a special case. 
 And what if (-2,-2) is passed in?  Ross disagrees with me here, but I 
think that (1) such a situation should be avoided and (2) if we do code 
like this, liberal assertions are needed to make sure things are working 
properly (otherwise they could fail, and it would be very hard to track).
> Jason, Ross, could we have a consensus on this particular point, devoid of
> any other considerations? If other considerations, what do they need to be?
The ideal solution IMO would be to have a real special-case indicator.

One way would be to use a struct map_pos *, which could then be NULL to 
indicate no coordinates (or invalid coordinates).  This would be ideal 
for vnotify_conn_ex, but less helpful in other places since it would 
involve a lot of allocating and deallocating.  Raimar also doesn't like 
it because pointer handling is slow.  Unfortunately, this method will 
not help when we're sending information over the network.
Another would be to introduce a new validity flag.  In the 
vnotify_conn_ex case, this would be an extra parameter indicating 
whether there were coordinates associated with the event.  This data 
could then be passed over the network (without breaking network 
compatability).
The biggest problem with any system is implementing it.  The 
vnotify_conn_ex case is pretty isolated, but other code that has similar 
problems (for instance the goto_dest_[xy] mentioned above) are spread 
throughout the code.  In many cases it's possible to ignore the problem, 
but this has the potential to cause weird, hard-to-trace, usually minor 
bugs.
Ross?  Raimar?

jason