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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Aug 28, 2001 at 10:55:47PM -0400, Ross W. Wetmore wrote:
> 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.

I still prefer unified diff. But you are correct in this case it may
be inappropriate.

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

My mistake.

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

Yes but I hoped to get rid of them and you don't touch them.

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

IMHO a more descriptive name is needed here since map_trunc can be
confused with map_wrap.

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

No. Please don't make any bigger patches.

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

No. Were have I stated this?

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

These function should still not be called wrap. I would prefer it if
such gui methods are not declared in map.[ch] but instead in some
client/ file.

> > - (unsigned)(POS) < (unsigned)(BASE) should be expressed as a macro
> 
> It is in a macro :-)

Please make it an extra one.

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

I never used it.
$ grep -Irc map_inx . |grep -v :0
./common/game.c:8
./common/map.h:1

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

So I would for example like a patch which either remove map_inx OR
rename it to map_index and convert all code.

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

Can't we instead of going back to the "raw" form (7-dir) replace
DIR_REVERSE with GUI_DIR_REVERSE? Now?

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

Correct.

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

Let me restate: Why are you using "#define FC__CITY_C 1" if this isn't
needed in the current version?

You have to acknowledge that casting to unsigned and comparing it is a
trick to gain performance and this trick isn't immediate obvious to a
normal programmer. It has to be documented somewhere or we went up
like in the ai case.

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

Arbitraty range or the name has to have a "zero" in it.

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

See below.

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

map_get_tile is defined/declared in map.h

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

I see no problem in used this divide-by-0 trick by having a method like:

int test_normal(ized)_position(int x,int y)
{
   asert(s_normal(ized)_position(x,y));
   return 1;
}

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

I didn't vote for inserting them in the cvs and then debug it. Insert,
debug, commit.

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

Can you explain this in more detail?

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

That is another nice idea. Putting the checks in the current CVS and
you have to enable them.

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

Mhh still haven't looked at corecleanup_07e a second time

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

See above.

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

We are the maintainers ;)

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

I understand you: you have put work into this and want it get
applied. The current situation is that you work by yourself and than
show a big amount of patches which will lead to a long discussion and
some part of your work will be rejected. IMHO one solution to prevent
this in the future is to make smaller steps. Any maybe also
concentrating on one patch at a time. You gain immediate
feedback. 

Just think that your working tree is the right one in the long term
and you now feed small bits to the cvs. Currently I'm not happy with
the arrangement, constitution and order of the bits.

You should have an idea what bits I want. Over and above I also accept
any micro patch like the cartesian_adjacent_iterate one. I think it
was you who said that he can send a small patch every 5
minutes. Please do so.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Despite all the medical advances of the 20th century, the mortality 
  rate remains unchanged at 1 death per person."