Complete.Org: Mailing Lists: Archives: freeciv-dev: August 2002:
[Freeciv-Dev] Re: normalize_map_pos and invalid map positions

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

[Top] [All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
To: freeciv-dev@xxxxxxxxxxx
Subject: [Freeciv-Dev] Re: normalize_map_pos and invalid map positions
From: Jason Short <jdorje@xxxxxxxxxxxxxxxxxxxxx>
Date: Fri, 16 Aug 2002 00:40:37 -0500

Raimar Falke wrote:
On Thu, Aug 15, 2002 at 11:43:39AM -0500, Jason Short wrote:

Per I. Mathisen wrote:

So what it comes down to is three different concerns: speed, safety, and error-catching. And it seems everyone values each concern differently (I put the most emphasis on error-catching).

We didn't catch a single error with CHECK_MAP_POS. So it was
disabled. However I have no problem to reenabling it if we work heavy
on the map.

We caught several existing errors with CHECK_MAP_POS - for instance air gotos were not getting normalized properly, and failed the CHECK_MAP_POS check. The last such segfault happened months ago. There is a high likelihood there are no more such bugs in the current code.

But new code is written all the time, and CHECK_MAP_POS is needed to help detect bugs there. For instance, Stewart's borders patch initially did not correctly use normalize_map_pos; this caused a CHECK_MAP_POS to fail and the bug was easily spotted. If a similar mistake was made again now it would take longer to detect since most people don't compile with DEBUG.

I agree with you that it can be changed at any time. But I do think that there's a big advantage to be gotten from having it enabled so that the majority of people who play with development code (including normal players who play the CVS version and don't know anything about DEBUG vs NDEBUG) can help us to debug just by playing.

In theory the set of "normal" positions could include (-1,-1). I think we all agree that it should not, though.

Yes. Any normal position is a regular position (see
common/map.h:regular_map_pos_is_normal). So we have

real positions are a (proper) subset of all map positions
  are equal if wrapping in both axises is enabled
regualr positions are a (proper) subset of real positions
  are equal if no wrapping is enabled
normal positions are a (proper) subset of regualr positions
  are equal in "classic" (non-iso) maps

"regular" positions are not, in general, a subset of real positions (consider for instance an unwrapping iso-rectangular map). Thus normal positions are a subset of both regular and real positions.

With the discovery of native/compressed coordinate systems, most uses of regular positions are no longer needed.

Most of this can be seen in action in Ross's corecleanups. He has a clean system of converting from one type of coordinate to another - map_to_native_pos, native_to_map_pos, etc. But when these things try to make their way into CVS, we always run into minor stumbling blocks that hold things up indefinitely (like the speed vs safety vs error-catching issue above).

And this is the part where I lost Jason and Ross the last time. In
addition to the three sets of map positions we now get at least two

It is confusing, that we have both *sets* of positions as well as *representations*. One thing we haven't done well is clarify this distinction; a statement like "all native coordinates are normal" is very misleading.

The corecleanups are hard to read since they change so much code. However, a change like this is pretty significant and will change a fair amount of code...eventually.


[Prev in Thread] Current Thread [Next in Thread]