[Freeciv-Dev] Re: [PATCH] Corecleanup_07Part2 has been put in incoming

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

At 06:29 PM 01/08/28 +0200, Raimar Falke wrote:
>On Sun, Aug 26, 2001 at 03:17:37PM -0400, Ross W. Wetmore wrote:
>> Attached is the ReadMe for second Part of the corecleanup_07 update to 
>> cvs-Aug-25. 
>
>I finally found time to look at part2.
> - corecleanup_07f isn't in unified diff
Context diff is one of the allowed forms. It is a little easier to 
spot things in this case. Unified diff goes berserk matching blank
lines and turns things into a goulash.

> - corecleanup_07f doesn't apply cleanly

Did you follow the dependency instructions in the patch, or did you
try to add it to your hacked subsets of the earlier patches?

Start from cvs-Aug-25 and apply the patches in order.

> - there should be no *_x and *_y version just one which takes both
> coordinates

There appears to be some context missing here. Are you referring to
map_adjust_x() and map_adjust_y()? These are untouched versions of the 
CVS functions at this point, I believe.

> - map_trunc isn't currently needed and should have another name
> (find_nearest_real_tile for example)

This would not be in keeping with the names of all the existing map_
functions, and in any event is not particularly descriptive of the
general behaviour.

It is used in corecleanup_07g where the functions that implement the
changes in 07f were placed as part of the bite-sizing that has been 
requested. I can go back to putting everything in one patch if desired.
This certainly reduces the effort to produce and test patches, and it
is the same amount of code change to review in the end.

You did apply and review all of Part2 as you stated above, yes?

> - there should be no *wrap* methods (encapsulation of the topology)

Wrap methods here have little to do with the topology and should not be
used for generic gaming coordinate manipulations except as the low-level 
operations in specialized code. They are there for special cases in GUI 
code and elsewhere that understand what they are doing and need to wrap 
values for their own data use as in gui-gtk:mapview.c. 

    else if(map_x < map_wrap_x(map_view_x0+map_canvas_store_twidth))
      *canvas_x = map_x+map.xsize-map_view_x0;                          

A number of the residual map_adjust_* functions in the GUI probably need 
to be changed to map_wrap_* or map_trunc_* as appropriate.

> - (unsigned)(POS) < (unsigned)(BASE) should be expressed as a macro

It is in a macro :-)

> - map_inx should be map_index (or remove it completely, map_get_tile
> should handle most of the cases)

map_inx is an untouched long standing CVS macro. It was not modified as part
of the requirement to make minimal impact on code for purely cosmetic changes.

But I agree that map_index is much more descriptive without really going
overboard with long-winded-German-style-compound-names-for-things :-).

> - what is the reason for this:
>-      put_line(map_canvas_store, dest_x, dest_y, DIR_REVERSE(dir));
>+      put_line(map_canvas_store, dest_x, dest_y, 7-dir);
>
>  Everybody keeps telling me the gui uses another system, but which
>  one? Either make a GUI_DIR_REVERSE(dir) OR leave it and change the
>  rest of the code to use the system of the gui OR tell me the reason.

This restores CVS to its self consistent state prior to Jason's patch and
unlinks the GUI from coordinate system changes being made elsewhere that 
could break it.

The GUI currently uses at least two direction systems. Thue's changes for 
isometric employed the DIR_DX2 or rotational direction system starting at 
north.

Much of the earlier GUI used the array order system, and I believe this
actually goes down to code that reads graphics data from data files. Until
someone reformats the graphics data or isolates this, it is dangerous to 
touch the GUI direction system, and certainly one should not link it to 
some global system that can be changed without realizing the impact on 
the GUI.

The pre-Jason-patch state should be the starting point for GUI cleanups 
rather than one partially implemented and filled with bugs and pitfalls
because it is linking things it is not yet safe to link.

When we are ready to change the GUI I vote for giving it its own set of
names separate from elsewhere. When everything has stabilized and the
two systems are in equivalent form, then there can be a quick rename of
the core elements everywhere to a single system.

In fact this is the strategy that has been employed in the corecleanups.
The new system is augmenting the DIR_DX2 usage by moving things into 
accordance with this, and not touching anything that is not explicitly
being updated.

Much of the direction stuff in map.h refers to the things it needs to be
consolidated with in tilespec.h. A good way to do this is to move it
all there. This is possible once the core has fully implemented DIR_DX2.

This has been discussed in numerous emails on this list.

> - +#define FC__CITY_C     1        /* pickup city_map_indices */
>   +#include "city.h"
>   This is ugly. Why?
> - -  if (city_x < 0 || city_y < 0
>   -      || city_x >= CITY_MAP_SIZE
>   -      || city_y >= CITY_MAP_SIZE)
>   +  if (((unsigned)city_x) >= CITY_MAP_SIZE
>   +   || ((unsigned)city_y) >= CITY_MAP_SIZE)
>   Please leave it so or make a macro. We discussed RANGE_CHECK.

Ugliness is not a technical term. This code is in full conformance with
the published coding styles for submission to CVS and should be treated
as such. If there are technical objections I am happy to address them.

If there is agreement that RANGE_CHECK is the name and function that
should be used in all BOUNDS_CHECK or MAP_CHECK operations I will make 
the necessary changes and put out a patch for this. Do you want a simple 
zero based change or the more complicated arbitraty integer range?

And will you apply all the updates if I put out the effort?

> - What is is_border2_tile? Why?

This was documented in map.h in the changes in 07f.
 *
 * is_border_tile(x,y) is TRUE if position(x,y) is on the outer border of
 *   the standard rectangular map or is outside the map. It is useful as
 *   a fast check for tile validity inside adjacent loops.
but it appears I have lost the version that had both names listed in the 
section shown. I will fix this.

It is used in the adjacent loops as stated. See core_iterate2.

A quick look at the definition of both of these at this location should
make the difference between the two quite clear.

>Let me state my position/plan:

I am glad to see the maintainers have finally come on board with this 
effort and are willing to take over the process and see it gets proper
attention to move it quickly to completion.

> - make a is_normal(ized)_position
> - insert an "assert(is_normal(ized)_position);" into every access
> method of map.h
> - debug

I will first point out that assert is a void function, and thus cannot
be inserted into macro expressions. There are no access macro methods
returning values in map.h anyway.

The corresponding trick of doing a divide-by-0 is used in the map_adjust_*
versions in the map topology updates in 07l of Part3.

07f does something like this to the access "functions" in map.c. It does
not to check whether a position is "normalized" or not as the current 
functions (or majority of them) assumed the need to normalize, and there
is a lot of code like the tilespec example awhile back that require this. 
This behaviour is not being changed in CVS by any of the current patches.

To restate this, there has been no effort to insure that coordinates are 
normalized before passing them into functions, and thus the suggested test 
would break CVS badly at the current time. I strongly recommend against 
such a rash action.

07f does an assert for "unreal" at this point, which is the state at 
which the current cleanups are safe except for residual nits. This is 
the requirements set for the past couple months work and reason for
submission of this series of patches.

It is all that is also required to handle alternate topologies, whereas
the is_normal(ized) check is purely for the performance optimization of 
doing these checks once at source, rather than at every level.

The first step is to make sure things are real. 

With the patches submissions prior to 07f this is expected to be complete
and 07f will test this in DEBUG mode. If anything has been omitted, 07f
should find it pretty quickly.

At this point if there is a desire to enforce a further rule that only 
normalized coordinates be passed around, that can be done in new corecleanup 
series in a parallel development track until most of the problems are fixed 
and submitted for inclusion into CVS - the maintainers should commit 
themselves to take all such changes and not pick and choose for spurious 
reasons leaving bugs, or I doubt anyone we really want to do it.

>
> - remove map_adjust_x and map_adjust_y
> - debug

To a large extent these are removed by the time you apply all the patches
as instructed. But in the GUI code in particular, this needs to be done in
a more comprehensive manner as part of some careful overhauls of its
assumptions about map topologies.

To do this, you do need to implement the truncate and wrapping functions
that are what map_adjust_* was being used for in various places. These
are required to correctly perform the current alogorithms, although
it may be possible to rewrite major sections of the GUI to handle things
under a different consistent model.

> - come to an agreement what the final direction system should be
> - migrate
> - debug

We are all in agreement, I believe, that the rotational direction system 
implemented by Thue (with the minor change of more closely overlaying it 
with the array order system by starting at the NW origin :-) is what the 
final direction system should be.

07j implements this for the core, and it starts gaining the benefits and 
efficiency as soon as this goes in.

In 07j there is ample justification of its benefits in reducing things like

#define DIR_IS_CARDINAL(dir)                           \
  ((dir) == DIR8_NORTH || (dir) == DIR8_EAST ||        \
   (dir) == DIR8_WEST || (dir) == DIR8_SOUTH)
   

to much simpler and more efficient forms like

#define DIR_IS_CARDINAL(n) ((n)&1)                  

>I'm won't do anything before I heard Gautes position/comments on your
>patch. Jason?

Actually, I think a good discussion is very much in order. If the 
maintainers can contribute to the process of improving the codebase and 
help move the cleanup the current load of Freeciv baggage along, we will
sooner move on, or get back to more productive and enjoyable things like 
alternate map topologies or better map generation. 

I would really appreciate this.

>> Note, this is a request for inclusion of these changes into CVS, as
opposed 
>> to just a RFC.
>
>I'm sorry but in its current form it is unacceptable for me.

Once we have discussed the issues and resolved them I am more than 
willing to do another iteration on the code. The inability of the Freeciv
maintainers to accept most things without somehow touching it is legendary 
and to be expected :-).

But there isn't much point of doing things incompletely and repeating the
effort 20 or 30 times to resync with the moving CVS codebase or any of the
other anti-pattern development practices that seem to be endemic with this
effort.

So, I think there are a number of non-technical as well as technical ones
to address and I suggest that the sooner this is done and the sooner we 
can get to the complete list of agreed changes, the sooner we can get the
current codebase in proper shape and move on to the more enjoyable things. 

So if you have completed your review of either the Part2 or Parts 2 and 3
and have the list to start the ball rolling, let's have at it ...

>
>       Raimar
>
>-- 
> email: rf13@xxxxxxxxxxxxxxxxx
> "We've all heard that a million monkeys banging on a million typewriters
>  will eventually reproduce the entire works of Shakespeare.
>  Now, thanks to the Internet, we know this is not true."

Cheers,
RossW