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

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

At 10:19 AM 02/01/08 +0100, Raimar Falke wrote:
>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.

>Here we go:

Yes, it is time to get the religious debates and other blockages out in
the open and then behind us, so we can get on with the process of fixing 
Freeciv :-).

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

I can just embed the rearrangements in with the fixes as you do in your
patches. This should avoid your gasping for context here.

Is this suitable?

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

This reduces the differences between the corecleanups and CVS in ways
that are consistent with K&R and the style discussions. It is done
largely because they are differences that have been creeping in. They
are just obvious here because I split this out from the more serious
changes as with the above rearrangements.

This is typically done by most of your patches and those of others as 
well, even in places where it makes the code less readable. There is 
nothing that contravenes any guidelines or goals, so they can only 
improve the codebase.

I assume this is suggestion, and since there is no reason to treat this
patch any different from others, I will ignore it for the most part. 

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

It works fine as a macro by #defining map_get_tile() in the header as
with the other macroized forms. It is safe and has been run that way
in the corecleanups extensively. I believe Jason also has used it.
The change here is to remove the previous partial MAP_TILE() changes,
i.e. convert everything back to map_get_tile().

Uppercasing is a poor thing to do elsewhere. It involves lots of code 
change that is unneeded and has attendant risk and disruption. Here
is was because there was a specific macro partially implemented, and
the function definition needed to be left exposed.

It is always possible to run macro or function version if left in a 
single format.

So, your suggestions here are not particularly good from a technical
standpoint. It should be left as in the patch if you just want the 
macro form here. It should be changed to map_get_tile() in the patch
otherwise, and the header updated if you want to use the macro 
everywhere. The latter is trivial to remove at the next flipflop.

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

No, no ... definitely ok :-)

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

Neither a particularly explanatory or useful comment. There is 
considerable debate about it, you are welcome to join in and share your
reasons.

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

Again, both a useless comment, and a poor technical decision. 

There are test conditions for realness and a normalization process. 
A proper interface should expose both of these, and not combine them 
into a monolithic performance intensive whole. 

This in fact causes loops in code that correctly tests for realness 
inside normalize_map_pos() before proceeding with the operation. Thus
it is required for the rest of the patch.

Effectively, this is what the macros are doing, and consistency between
the two is worthwhile for a number of reasons.

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

Again a poor technical analysis. Then a contradictory response.

I will repeat, any normalize_map_pos() that performs any operation on
unreal coordinates is repeating the void_tile mistake of changing things
that cannot be legitimately changed.

You need to insure the coordinates are within real bounds, before you 
can proceed with the transformation.

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

Flipflop decisions are really a poor way to deal with such issues. Nor
is continually revisiting old debates.

It is a bad (possibly terminal) habit for anyone in a position of authority.

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

see above

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

see above

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

This is the patch for map.c and map.h which are "file" fixes as you 
requested. The later patches will deal with other followon changes.
There are only 3 or 4 routines that use map_inx() anyway.

These are all done in the alternate devpath, though I initially used
map_index() as suggested by previous discussion, and have changed this 
to the more or less standard format for coordinates for consistency in 
the final CVS version.

whole_map_iterate, mapgen.c uses this fairly extensively. Jason has 
proposed it be used for random_map_pos(). Plus it is part of the set
of functions for converting to and from standard coordinates.

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

As a user of this construct, you should be concerned with what it is 
supposed to do, not how it does it. And this should not change if it
does it differently down the road.

The interface is what is important. The interface should not change its
appearance just because it is implemented as a macro or function. 
Especially when this means updating many areas of the codebase. Cosmetic 
renames are the sign of foolish vanity or poor understanding unless there 
is some solid and usually self-evident reasoning behind it. 

The macros will be used unless specifically undef'd. You can do this
globally in the header file, or locally if desired. Having both forms
preserved means that one can switch back and forth for various reasons
and the codebase won't require any serious change.

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

I thought I would give you things that were relatively easy and straight
forward to deal with on the first patch. No deep thought, no technical
problems or serious issues, nothing really too controversial for you
to pick on.

I'm sure the real discussions will be much more heated than this, as we
get into real technical challenge and serious changes to Freeciv.

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

You didn't do a very good job in the past. Nor is this response/review 
of particularly beneficial quality here. 

It is good to see you plan on trying harder, though.

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

Actually, this is not a list of things to be done so much as a list of
obstacles to foul up the process. It does show the problems with the 
current system though, quite well.

Having you micromange the process with petty requests and poor suggestions
is not going to be particularly effective.

The decisions you should be making are whether you want to update the 
code with certain general features, and a list of generally applicable 
constraints that are applied consistently to all patches. When the result
is submitted and reviewed, you can make a judgment call on any contentious
issues, and certainly make the final pass/fail for hopefully reasonable
and well articulated grounds.

How they get implemented will be to a large part, I hope anyway, dealt
with by the person doing the work and people with more time, skill and 
understanding reviewing so as to result in the best code in Freeciv.

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

It would be simpler if the issues were debated on technical merit and 
not ego, ugliness, favoritism or any other of the subjective bases that
seem to have precedence currently.

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

But as you said, the only problem is the lack of patches. 

Cheers,
RossW
=====