[Freeciv-Dev] (PR#4594) topology fix for goto route packet

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

[rwetmore@xxxxxxxxxxxx - Tue Jul 29 03:04:06 2003]:

> 
> Baumans wrote:
> > ----- Original Message -----
> > From: <rwetmore@xxxxxxxxxxxx>
> > To: <jdorje@xxxxxxxxxxxxxxxxxxxxx>
> > Sent: Monday, July 28, 2003 6:13 PM
> > Subject: [Freeciv-Dev] Re: (PR#4594) topology fix for goto route
> packet
> [...]
> >>>The options are:
> >>>
> >>>   if (!is_normal_map_pos(x, y)) break;
> >>>
> >>>   if (!normalize_map_pos(x, y)) break;
> >>>
> >>>   if ((x == 255 && y == 255) || !normalize_map_pos(x, y)) break;
> >>>
> >>>The first has the disadvantage that client must send only normal
> [...]
> >>>The third has the disadvantage that if the client actually sends
> any
> >>>non-normal positions you'll end up with buggy behavior: eventually
> >>>they'll send (255,255) and have their goto route prematurely
> terminated.
> >>
> >>No disadvantage ... (-1,-1) has always been a special case, and can
> >>certainly continue to be one in the future.
> >
> > What if the client sends (-1,-1), in the torus topology?  The server
> can't
> > tell what this means: is it supposed to terminate the route or go to
> the
> > normalized (-1,-1)?  The only way it can know what to do is to
> require that
> > the client not send (-1,-1) except as a special case, and we're back
> to
> > restricting the client.  (-1,-1) used to work because that never
> used to be
> > a real coordinate.  This is also applicable to (255,255).
> 
> First, (-1,-1) has always been the invalid coordinate flag, it has
> always
> been special and has been used by the clients in the past for this. It
> is
> used on the server too and is being used in new code going in daily.
> This
> problem needs to be fixed everywhere for gen-topologies. One really
> does
> need a "not-a-coordinate" coordinate concept and gen-topologies breaks
> the
> current definition. One should not just ignore the underlying problem
> of
> how to deal with this special coordinate with local hacks like (1).
> 
> This has never been a concern before gen-topologies since (-1,-1) was
> never
> a valid aka real coordinate, thus only gen-topology code changes are
> needed
> to *fix* any clients, i.e. old clients don't need any fix, but they
> will
> continue to send (-1,-1) to new servers. Jason's *fix* makes this a
> bug,
> while (3) preserves the backwards compatible standard and works with
> all
> clients. (3) is actually more efficient code too.

The old clients don't send (-1,-1), they send (map.xsize,map.ysize). 
But either way this isn't a bug since it is an unreal position
triggering the server to end the goto - which is exactly the intention.

You still haven't answered Baumans' (and my) argument.  If (-1,-1) and
(map.xsize,map.ysize) are valid, real positions, how is the server
supposed to know when it receives this position if it is intended as the
special case or as a real position?

It's all well and good to say "the client will only send this as the
special case", but how is this going to be enforced?  If the client
doesn't do the normalization itself it can end up sending these values.
 Even if the client does do the normalization it can end up with these
values if they are normal positions.  The only safe solution to this
problem is to say that server and client must have the same definition
of normal positions, so that all special-cases (not just -1,-1) are
safe.  But if we do this we're back where we started and the is_normal
check is the correct one.

This pitfall is demonstrated by the bug this code caused in
gen-topologies.  Because map.xsize,map.ysize was a normal position that
was being used as a special case, it caused rare, hard-to-trace errors
in goto routes.  Only when I succeeded in reproducing and tracking down
this error did I find the code that caused it.  The new code has
sufficient asserts (on the sender side) and checks (on the receiver
side) so that if this situation ever arises again we will quickly be
able to debug and fix it.

> I have also played a lot with this *not* fixed on the client. It
> really
> isn't an earth shattering problem to have some client operations fail
> cleanly even when they should have worked. Thus the full client fix is
> the lesser part of this problem if you have the server (3) version in
> place. Breaking all old clients and forcing unnecessary conditions on
> clients for the forseeable the future is much worse of an issue.

Since this bug is still present in the corecleanups, I can only conclude
you haven't played much on iso-maps.  Trust me, the bug is (was) very
annoying for the player.

jason