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

["Per I. Mathisen" <per@xxxxxxxxxxx>]

On Fri, 9 Aug 2002, Jason Short wrote:
> 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.

Not to mention that it hides real errors in the code that should be dealt
with.

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

Ok, I've been skipping my homework. Care to clue me in to the difference
between *real* and *normal* map positions?

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

I doubt there would be spectacular errors. The whole setup now relies on
the fact that the AI recalculates its gotos and actions every turn, so it
does not need to rely on goto_dest_? for very long after they have been
set.

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

So my goto coords will be (99,79) and the goto code will realize somehow
that we will walk over the wraparound edge instead of 99 tiles down south.
I can buy that. That means (-1,-1) is available for "bad unit, no goto for
you".

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

Just make a strict policy and interface, then enforce it? For each data
structure, designate whether it will contain normalized or raw
coordinates. The latter I would think should only be generated in a very
few places in the code. Or am I wrong?

> And what if (-2,-2) is passed in?

If we encounter that in a piece of data where coords should be normalized,
then obviously we should assert and die (or freelog and return). No?

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

The hard to track bit would depend on the number of tests done to see that
coords are always valid, I would guess.

> 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 sendinginformation over the network.

The last catch is bad. Otherwise it sounds like a good solution to me, at
least for goto code. If there is an error and we shouldn't be accessing
the faulty coords, we'll core and be able to fix it.

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

Ugh. Please not even more parameters and parameter interdependencies.

How about combining the two above: struct normally, and then use a new
parameter when sending it over the network, so that it is reassembled
correctly on the other end?

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

Just give me a sane implementation and I'll go fix up goto_dest_?.

Yours
Per