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

["rwetmore@xxxxxxxxxxxx" <rwetmore@xxxxxxxxxxxx>]

Jason Short wrote:
> [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;
>>>>>
[...]
>>>>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

If it is not the special case, then the new topology code has a bug.

[...]
>>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). 

Go look again.  This was the cuse of the original bug report ...

There are no special cases or need for special cases today except (-1,-1)
that I am aware of, and not likely to be a lot of others, but they can be
handled in the same way as this should be.

Note, (255, 255) as uint8 is a bug in the special case handling that needs
to be fixed. Conversion of the special case to and from a network packet
transmission format needs to preserve the special case.

And please quit making these false or (self-)contradictory statements :-).

[...]
> 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?

Please (re)read the postings more carefully ...

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

Up to the gen-topology patch - suggestions were provided, but to repeat I
suspect that the standard pre-normalize at source will take care of most of
the issues, since passing an unnormalized coordinate as argument is largely
verboten now. All one needs to do is insure that by the time it reaches
the network code before it ships out, any special coordinate has undergone
at least one standard check.

But *note* this should not be done at the network level client or server
which has no way to know if this is a flagged bogus coordinate or mistake
nor what to do with the unreal condition.

 > If the client
> doesn't do the normalization itself it can end up sending these values.

All it *ever* needs to do is normalize or provide an alternate unnormalized
value from any of the possible normal sets for this one special coordinate
which is a fast condition to check and fix. The rest are not special and
will be handled by the server just fine in case 3).

Note, the server can only fault on unreal coordinates, so the usual
arguments about normalizing within the locally aware source creation code
module still apply. This is the only place where an unreal coordinate can
be properly handled. This also implies that the client network code is not
the place to do this either, any more than the server layer one down.

It does not require you to forbid the server to fix all other unnormalized
coordinates in passing just for some silly religious hangup that has
negative technical merit.

>  Even if the client does do the normalization it can end up with these
> values if they are normal positions. 

No, this one value is *never* normal, only special. If you ever define a
normal set that wants to use this value, then figure out the workaround
for that normal set. Normal sets are always user defineable and it is a
real problem for you that you do not ever seem to understand this point.

We have no case where (-1,-1) lies in the normal set today. Only some sort
or spherical and infinite plane map would need to deal with this in some
serious way, and they will never be implemented at the rate that this set
of updates is proceeding :-).

> The only safe solution to this
> problem is to say that server and client must have the same definition
> of normal positions, 

Yes, the serve and client rules for their shared normal sets need to agree
unless safety is carefully crafted and documented for any given cases.

> so that all special-cases (not just -1,-1) are

To repeat ...

There are no special cases or need for special cases today except (-1,-1)
that I am aware of, and not likely to be a lot of others, but they can be
handled in the same way as this should be.

> safe.  But if we do this we're back where we started and the is_normal
> check is the correct one.

is_normal is not a hard condition.
is_normal was introduced as a temporary hack and born deprecated.
is_normal is more inefficient and less user friendly than 
normalize_map_pos(), so has no redeeming qualities.

It is *never* the correst solution :-).

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

Some pitfalls and bugs are hard to find. Live eith it, and don't make the
code do unnecessary handsprings because you haven't thought of a better
solution to find them.

If people *are* adding new special cases, then there is every reason to
figure out how to do this right and put the code in to handle it in one
spot so there are not a number of ad hoc flavours to track down. Your
solution to try to avoid handling it is the wrong way to go.

Besides, "not-a-coordinate", there might be a "link-coordinate" or a
very small number of reserverd coordinate values that need to be dealt
with in any topology that chooses to define its normal set poorly. In
the case of (map.xsize, map.ysize), choosing a positive value, moreover
one that is context dependent (map sizes can change) is the first big
mistake. Most normal sets are going to be chosen with a view to
minimizing errors in array lookups and such operations.

One suggestion might be indexed coordinates starting at -1 be the source
of reserved special coordinates, or possibly reserving the high bit(s) in
indexed values for flagging special sets. But this need not be fully 
architected or implemented at this point, just leave room for the future
options along these lines.

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

I almost never play on non-iso-maps unless I am doing dual debugging now
that the iso-trident tileset is usable :-).

I think your code is just not as clean and/or user friendly in dealing with
errors. I have avoided adding or removed most of the asserts you and Raimar
have been putting in CVS, fixing the underlying problems in passing.

Not doing normalize_map_pos() at portal points and not understanding the
real difference and concerns between unreal and unnormal coordinates gets
you into a lot more trouble. That is why you need to get this one right.

> jason

Cheers,
RossW
=====