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

[Jason Short <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

Ross W. Wetmore wrote:
> At 01:15 PM 02/08/15 +0000, Per I. Mathisen wrote:
>
> > On Tue, 13 Aug 2002, Ross W. Wetmore wrote:
> >
> > > Normalize_map_pos() is the swiss army knife for coordinate testing.
> > >
> > > It returns false if the coordinate is unreal - generally terminal error.
> > >
> > > It returns true and a valid normalized coordinate pair otherwise. This
> > > may involve normalizing the coordinates in the process but that is always
> > > possible if they are real.
> > > This should be used at all portal points, i.e. places where you should
> > > check for unreal coordinates. This includes source creation, network
> > > input points or any place where one might want to check for corruption
> > > of any sort, or even just a change in the definition of "normalized".
> > >
> > > In some cases it is better to use unnormalized or differently normalized
> > > coordinates for a given operation. In this case one would normalize before
> > > handing off such coordinates to more picky code outside of this module,
> > > or do it on input to any module that might receive such coordinates.
> > > This should always be the case. It doesn't hurt to fix things up and
> > > continue in any situation, and a few judiciously placed checks like
> > > this will be useful to catch the unreal cases which are terminal.
> > > So even the big reason for normalizing everywhere is not really that
> > > sensible with truly generalized topologies.
> > So what you are suggesting is that normalize_map_pos() is used at the
> > lowest level where coordinates are generated, changed or received from the
> > client, and that the the higher level functions receive properly
> > normalized map positions which are checked with is_normal_map_pos().
> >
> > Right?
>
> I'm juggling lower/higher but I think I know what you mean.
<snip>

Very nice explanation, Ross.  I only disagree with you on one point:

> Any real coordinates are fine. They are always fine as they can always
> be normalized on the fly with comnplete safety in all cases. They are
> never wrong, and can never be made more wrong. Any other interpretation
> is pure religious hogwash used to justify an unnecessary constraint.
This is not true.  In the vnotify_conn_ex case, it is certainly not true.

Here's the explanation again, from scratch.  Suppose we are dealing with 
a topology where (-1,-1) is real (but not normal).
Then, first of all, we certainly can't call normalize_map_pos() in 
vnotify_conn_ex on the coordinates without first checking for this 
special case condition; that would result in "invalid" (special-case) 
coordinates being translated into "valid" (normal) ones.
Secondly, note that (-1,-1) is a real, non-normal position.  Now what 
happens if the calling code happens to pass in real, non-normal 
positions to vnotify_conn_ex?  Usually this would be a soft error; we 
could just call normalize_map_pos and be done with it.  But in this case 
there is a special problem introduced by the special-case coordinates. 
If a caller can pass in real, non-normal positions then eventually it 
may pass in (-1,-1) - and when it does this the function will erronously 
treat it as the special-case condition.  The special-case will be passed 
on to the client where it will cause some slightly weird behavior: (I 
think) the message window will know about the event, but double-clicking 
it will have no effect.  The player will probably think nothing of it, 
and the bug will never be reported.
There is only one way to detect this bug.  We can make sure that no 
real, non-normal coordinates *except* (-1,-1) are passed in to 
vnotify_conn_ex.  Although this isn't strictly an error, it is the only 
indication that an error may be happening.  It is easy to have a single 
assertion to check this case.
This is what the current vnotify_conn_ex does.

jason