[Freeciv-Dev] Re: [PATCH] Map cleanups (PR#1208)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Jan 07, 2002 at 09:32:44PM -0500, Ross W. Wetmore wrote:
> 
> I lost you here. This is the only part that looks like a "request", 
> but it is confusing.
> 
> At 12:39 PM 02/01/07 +0100, Raimar Falke wrote:
> >On Mon, Jan 07, 2002 at 12:31:10AM -0500, Ross W. Wetmore wrote:
> >> At 03:45 PM 02/01/06 +0100, Raimar Falke wrote:
> >> >On Sun, Jan 06, 2002 at 04:32:02AM -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx
> wrote:
> >> >> Ross W. Wetmore wrote:
> >> >> 
> >> >> > This series of patches cleans up a number of cosmetic and real issues
> >> >> > in map.c and map.h.
> [...] 
> >> Much of this changes with the topology fixes, since it is these few basic
> >> elements that form the valid interface, and the implementation here is
> where 
> >> the core topology changes will largely go. But first one needs to get the 
> >> interfaces in so the code can be updated to use them and there is something
> >> to build on.
> >> 
> >> Note, this is in the 5-10% range of easy things to get out of the way.
> >
> >I agree that the goal is ok. However I had some problems with the
> >iterators of an old corecleanup version. So I have to take a look at
> >the current version.
> [...]
> >The change in mapclean_0* is ok. You have to make a proposal for the
> >further changes (patch or description).
> [...]
> >> In summary, there are 2 minor fixes (will do in next pass), 4 major
> religious 
> >> wars and one skirmish to be fought over two comments and three functional 
> >> changes, and one previously agreed on and requested feature Raimar now
> wants 
> >> dropped.
> >> 
> >> Does this summarize the current status?
> >
> >Besides the requested feature: yes it looks so.
> >
> >     Raimar
> >-- 
> > email: rf13@xxxxxxxxxxxxxxxxx
> > "The very concept of PNP is a lovely dream that simply does not translate to
> >  reality. The confusion of manually doing stuff is nothing compared to the
> >  confusion of computers trying to do stuff and getting it wrong, which they
> >  gleefully do with great enthusiasm." 
> >    -- Jinx Tigr in the SDM
> 
> If you mean the map_clean_0 comment and map_clean is what is up for 
> discussion and being summarized I thought, map_clean is one patch. It is 
> split into three sections to make it easier for people to distinguish 
> non-functional or purely local changes from the more serious parts. 
> There is no such things as map_clean_0, but there is a current patch up for 
> consideration, with a fairly complete listing of all its elements. There
> is nothing more to be "requested" or no new patch submitted here as far as 
> I can tell. The current one just needs to be dealt with, hopefully in a
> way that will prove fruitful to all, but especially the codebase.

Ok. But what happends if I say this piece of the patch won't get in? 
Will you make a new patch? If I would have the time I would really
like to rip of all the stuff of the map_clean patch(es) which I
like. Unfortunately I don't have the time.

I'm really happy that I see that you split the corecleanup into
smaller patches.

> If you mean a proposal or patch request for future patches, as in the
> various iterator change, then you need to spell out a bit more of what 
> you are looking for or set some priorities.

Here we go:

> 0)  mapclean_00.dif
>     This is a pure cut and paste line rearrangement of the existing
> files intended to improve the grouping of related functionality and
> simplify/localize the subsequent changes. The line and character
> count (wc) should be unchanged. One can repeat this by doing cut and 
> paste operations on a clean CVS file to match side-by-side views with 
> the patch result, and use diff as a final verify step if you are
> consciencious or lacking in trust :-).

For inclusion you have to provide at least a description of the
sections. For macro, enum movement a style guide fixing is necessary.

> 1)  mapclean_01.dif
>     This is a series of local cosmetic and minor updates which should
> have no global scope or significant impact on game execution. They can
> largely be scanned in a superficial way, i.e. no heavy thinking.
> 

> a)  miscellaneous K&R2 style changes, mostly added spaces. This is not
>     a complete pass, but resolves discrepancies between changes done 
>     during the corecleanups and CVS which improve general readability.

The "feed the whole code through indent" problem again. Can you
postpone these?

> b)  complete map_get_tile() -> MAP_TILE() changes for consistency and
>     improved performance (direct array vs function call access)

If we go for consistency I would make the use of the public function
map_get_tile mandatory. If we go for speed make map_get_tile a
macro. Problem: either you have to review all calls and uppercase it
(e.g. MAP_GET_TILE).

> c)  updated commentary in a number of places

I have to look deeper in this.

> 2)  mapclean_02.dif
>     This contains a small number of more significant changes plus an
> extensive update of map.h non-directional macros and fixes outstanding
> bugs with normalize_map_pos() and friends. While the functional versions
> are both equivalent and fixed, most program use should now be via macro 
> for increased performance. These features have been in the corecleanups 
> for 6 months and are completely compatible with the current CVS codebase,
> so there should be little or no concern about risk.
> 
> a)  map_city_radius_iterate -> city_map_checked_iterate
>     This fixes unnormalized access to map_get_terrain which can no longer
>     deal with real but non-normal coordinates.

Ok.

> b)  && -> || mistake
>     One T_UNKNOWN state can be handled, two cannot, and none is excessive
>     restriction on the server AI.

Probably ok.

> c)  is_normal_map_pos() -> normalize_map_pos()
>     Returned values must be normalized, so just do it and return.

No.

> d)  is_real_tile() is a test, not a full normalization, make it so.

No.

> e)  normalize_map_pos() should test for realness before it undertakes
>     any transformation. This fixes any potential bugs from side effects.

Idea yes. Implementation no.

> f)  nearest_real_pos() is defined consistently with normalize_map_pos()
>     to return FALSE for unreal coordinates and TRUE for real. The only
>     difference is that it returns arbitrary but normal results in the 
>     unreal case, i.e. controlled side effects.

Yes.

> g)  RANGE_CHECK macros are added and used to improve performance by
>     reducing execution in most cases where applicable by a factor of two.

I need numbers. It looks like I have to gather them myself. Either
from the mail archive or by testing.

> h)  Wrap and clipping macros are added to standardize code use. Overtime
>     all map code that does local flavours should be updated to use these.

No.

> i)  map_adjust_x() and map_adjust_y() are redefined to be explicit
>     wrap and clip operations for the standard cylindrical map, as the
>     first instance of standardization :-).

No.

> j)  map_to_index() is defined as the start of the map_inx() removal.
>     When the corecleanup and topology fixes are all applied, this will
>     be completely gone. index_to_map_pos() as the reverse operation is
>     defined in the standard manner, i.e. returns updated coordinates
>     and a status return indicating success or failure.

Yes. But rename it in all the code. Where would index_to_map_pos be
needed?

> k)  Macro forms for is_real_tile() and family are defined for the
>     cylindrical map. These will be upgraded to full topological forms
>     when further capabilities (i.e. topological state) are added.
> l)  The generic border tile macro is added and used in the current case.

I haven't looked at these. I'm confused of the macros and functions
with the same name. Why?

> <ASIDE>
> Note I carefully excluded the rotational direction system from this patch, 
> so map.c and map.h are still significantly divergent from the corecleanups 
> :-).

I'm looking forward for another thread about this topic ;)

> Look at places like this to see what ordering they use for their direction
> enums. It is 2/3 of the way down.
> 
> This also shows how the coordinate system for native isometric is typically
> handled. It corresponds to the term "compressed native isometric" in the
> corecleanups.
> 
> http://www.gamedev.net/reference/articles/article747.asp
> </ASIDE>
> 
> To state some obvious points ...
> 
> There will probably be 20-25 patches to merge the corecleanups with CVS.
> There were 13 in August of which only 5 were even partially looked at, 
> and none of the rotational or topology sections were ever reached. It
> bogged down in the bugfixes and preliminaries like the current patch.
> Including the 3 month straightest_dir() bugfix debate.

> I believe you recently stated that the issue with corecleanups was not
> getting patches. If you are waiting for these, there are probably 3-4
> areas that can be run in parallel, so you will get a few every week
> until things bog down and it becomes clear the effort is going to be as
> useful as the earlier one.

Go ahead. I will try to answer every patch which gets submitted to the
bug tracking.

> The contents of the corecleanups have been available and elements of them
> sometimes discussed. This is a working prototype with general documentation
> in the code to explain much of what is being done in various parts. This 
> is slightly more than a proposal tends to provide. But I assure you the
> read is probably far more useful. 
> 
> There is an outstanding Freeciv TODO for the cleanups and the topology, 
> particularly the isometric parts. Consider this as a combined submission 
> to deal with them.

> I really would *like* to see discussion of various aspects thrashed out
> in advance, then the final patches submitted primarily for code review
> and sanity testing before going into CVS. It is a mistake to turn every
> patch into a from scratch dogfight bringing every past issue that can be
> tied to it into the process as part of a general ongoing guerilla war :-).

I understand you. So if you start with a list such as above than much
hassle can be avoided (well at least I hope so).

> This means discussion and requirements precede final implementation - but 
> this is not really part of Freeciv policy, just as significant updates or
> alternate devpaths are generally killed in favour of small nitfixes. It
> will be interesting to see how this goes :-).

I agree with you that it is difficult.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "At the beginning of the week, we sealed ten BSD programmers
  into a computer room with a single distribution of BSD Unix.
  Upon opening the room after seven days, we found all ten programmers 
  dead, clutching each other's throats, and thirteen new flavors of BSD."