[Freeciv-Dev] Re: review of corecleanup_06

[Jason Dorje Short <jshort@xxxxxxxxxxxxx>]

"Ross W. Wetmore" wrote:
> 

> >2.  IMO all of your /* The GUI uses DIR_DX directions */ comments should
> >have a FIXME in there.  The directions shouldn't be hardcoded into the
> >GUI!
> 
> I'm not a GUI person, so take what I say with a grain of salt. I tried to
> track things down to find out why, and reached a point where I think the
> GUI is reading in graphics tile information from disk data files in DIR_DX
> order.
> 
> But the GUI also has DIR_DX2 order in tilespec.c at least, so it is
> schizoid, and nothing it does should be assumed lightly.
> 
> All this stuff is completely self-contained and separate from the core
> server directions though so GUI cleanup and/or resolution can be done
> by people that know what they are doing at a later date.
> 
> The trick is not to cross pollinate the direction fixes and thus keep things
> separate.

I'm also not a GUI person...but I think it's obvious that this stuff
needs fixing and adding a FIXME to the comment is more likely to get it
done.

I now understand your separate use of DIR_DX and DIR_DX2, and I really
hate it (I see why you did it; the whole situation is pretty
unfortunate).  In my opinion we should do this:

- Get rid of the current DIR_DX2/DIR_DY2 arrays and usages.  Unify with
DIR_DX/DIR_DY.  Then we will have just one directional system, even
though it is a bad one.

- Find and fix whatever is necessary to change the directional system
(anything dependent on the directional system).

- Switch DIR_DX/DIR_DY to use the rotational (DIR_DX2) directional
system.  I think we can all agree that this one is better in several
ways.

> >4.  In server/gotohand.c/1190 you add a normalize_map_pos call without
> >checking it's output.  Every time a normalization is done the realness
> >should be verified also - either by checking the return value of
> >normalize_map_pos() or with an assert(is_real_tile(...)).
> 
> No! But what you are saying is the safe bet if you aren't sure.
> 
> In this case, you just need to make sure the coordinates are normalized,
> not that they are real - that is guaranted. The original comment is a clue.
> 
> But I can accept an assert as a paranoid measure at this point. It is
> just that sometime they will all need to be taken out, as the clutter of
> a lot of unnecssary checks will become excessive the way things are going.

I disagree with you here.

Any time you call normalize_map_pos, you obviously need quite different
results depending on the tile's realness.  If you're checking realness
at the same time, this is obvious (you'll have an
if(normalize_map_pos(...)) construct), so all is well.  If you already
"know" the tile to be real, then you are free to proceed about your
business...but it is safer to add in an assert(is_real_tile(...)) here. 
It also makes the code easier to read IMO because it is clear that
you've considered the unreal case and know what you're doing.  If a
realness check happened immediately earlier this is not necessary.

A new function/macro, normalize_real_map_pos, would make this a little
simpler but isn't really necessary IMO.

> >5.  The same is done countless times throughout the client.  I think
> >once this patch is into CVS asserts should be tried out in the GUI code
> >and we see what breaks; until then that's too much to deal with. [1]
> 
> No! Again! Understand the client - which I don't - but in this accept
> the fact that GUI stuff can handle and should handle non-real coordinates.
> They are drawn as "black" tiles.
> 
> There may be some cases which I haven't left comments on or got wrong.
> But a blind application of server principle here is not reasonable.

I don't understand the client, but I'm confident that from a theoretical
point of view I'm right.  The client should never "handle" unreal
coordinates themselves, but it should have separate handling of
coordinates that are unreal.

Consider the following uglificized (as Trent would say) pseudo-code:

void draw_tile(int x, y)
{
        normalize_map_pos(x, y);
        ...
        if (is_real_tile(x, y))
                draw_black_unreal_tile(x, y);
        else {
                tile *ptile = get_map_pos(x, y);
                pixmap *tile_pixmap = get_pixmap_for_tile(ptile);
                draw_tile_pixmap(x, y, pixmap);
        }
}

This code won't work!  In fact it will give similar wrapping problems to
what FreeCiv has now.  What is needed is the following code instead:

void draw_tile(x, y)
{
        int mapx = x, mapy = y;
        ...
        if (normalize_map_pos(&mapx, &mapy))
                draw_black_unreal_tile(x, y);
        else {
                tile *ptile = get_map_pos(mapx, mapy);
                pixmap *tile_pixmap = get_pixmap_for_tile(ptile);
                draw_tile_pixmap(x, y, pixmap);
        }
}

As to the more general point, either (1) you're sure the tile is real or
(2) you need different handling of real and non-real cases.  In either
case there should be something indicating that right by the
normalize_map_pos call.  Checking for realness later is useless...you
should have just waited until then to normalize.

> >8.  In server/cityturn.c this code also seems unhelpful.
> >
> >-        if (cur > best) {
> >-          bx=x;
> >-          by=y;
> >-          best = cur;
> >-      }
> >+        if (cur > best)
> >+          bx= x, by= y, best= cur;
> 
> Comma operators group things that need to be grouped. It is a foolish
> style that keeps splitting things to separate lines where a misplaced
> or missing if bracket will seriously damage the required behaviour.

It also seems foolish to remove brackets around code that could later be
split by someone else and broken.  I don't have a problem with the use
of , at all, but I think your zeal is misplaced.  As I see it, you've
only replaced one pitfall by another, in the process changing arbitrary
code to fit your standard.

Unlike the loop you changed, I find this code easier to read your way.

> >[1] No assumptions should be made about unreal coordinates.  The calling
> >function may be calling normalize_map_pos just to wrap the coordinates,
> >but this is the incorrect usage!  Special handling is always needed for
> >unreal coordinates.  The current drawing system may be broken - what it
> >should do is use the absolute coordinates to determine where to draw,
> 
> Note that normalize_map_pos does not touch either x or y values of unreal
> coordinates, and thus they are passed through in such GUI code. It isn't
> the usual way one thinks of using normalize_map_pos, but it does respect
> the original flavour and comments, with the added fix that x is left alone.
> 
> I think one *really* needs a crash course in GUI tricks to really understand
> this, but I think that a GUIcleanup will be needed shortly if not already.

Counting on normalize_map_pos to leave X and Y values of unreal
coordinates alone is both unsafe (since it certainly does not guarantee
this condition) and a sign of problems elsewhere (as described above). 
Calling code should treat x and y as undefined values after a call to
normalize_map_pos(&x, &y) fails.  (I believe this is the most "correct"
solution, but it's easier just to leave the values as-is and hope
everything works out.)

> Apart from the abuse of subjective terms like ugly which we should all
> probably strive to suppress, it looks like you've beat on me and the
> code pretty successfully.

Heh heh...any time I (or anyone else, I think) say "ugly" I really mean
"IMO ugly".  No offense was intended.

jason