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

[Raimar Falke <rf13@xxxxxxxxxxxxxxxxx>]

On Fri, Aug 09, 2002 at 05:24:40PM -0500, Jason Short wrote:
> 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.

I can't remember saying so but you may be right.

> 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?

First I would like to point out that these issues didn't cause any bug
I'm aware of. So the current situation isn't nice but also doesn't
create a new bug every month.

If we agree that we only use normal positions (de facto we use only
normal positions and also (from doc/HACKING):

  Almost all functions expect that any passed map positions are normal
  (is_normal_map_pos returns true). Currently there is no known
  exception of this rule. To assert this the CHECK_MAP_POS macro
  should be used.

) we have (-1, -1) as a "free" value. Which can be used to indicate
something.

So while we have the usage of normal posistions for functions fixed in
the code and the docu the usage of normal position at the network is
currently only in the code. So an entry like "Map position in packets
should be normal or the packet is removed entirely." should be added
to doc/HACKING. This would close this case. Than you can use (-1,-1)
as a flag value for goto_dest and maybe other stuff.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Understanding is a three-edged sword; 
  your side, their side, and the truth."
    -- a well-known Vorlon