[Freeciv-Dev] Re: review of corecleanup_06

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

At 11:34 PM 01/08/23 -0400, Jason Dorje Short wrote:
>"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 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.

No problem with the FIXME.

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

I think relegating the GUI stuff to the GUI is sufficient. It never was
and now is not the time to link the two together.

Consolidating on DIR_DX2 and cleaning up the current usage of it is also 
best. If there is a global name change down the road so be it. Do not, 
switch *anything* to DIR_DX until it has been resolved - just leave that 
mess alone or push it back to the GUI. I think this includes CART_DIR.

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

The rest of the codebase is clean and can move forward now. There is 
nothing that really needs to be fixed here that is not already touched
in the corecleanups - AFAIK.

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

Yes. So much so, that waiting for someone to unscramble the GUI mess
is a bad choice. The name is immaterial calling it DIR_DX2 is fine.

Remember these are corecleanups. I'm for not expanding the scope, but
rather getting the proper resources to open a new cleanup task.

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

We agree, the assert is a redundant indication that the lack of an if
says isn't necessary. I can accept it as a comment/clue that reinforces
what the code already says.

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

Some people need to be told things multiple times. That's fine.

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

Right.

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

You should have stopped at "I don't understand the client".

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

Both of these execute identically, except (apart from the missing &s typo) 
the first tells itself multiple times before it believes it. The second 
caches the identical values so it thinks it is doing something different. 
Remove all mapx,mapy stuff in the second one and you have the right pseudo 
code.

Before you respond, think it through ... carefully. 

You really need to get past the theoretical and understand reality here.

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

There is. If you are worried about realness, normalize_map_pos protects
the if clause. If you are dealing with unreal coordinates then you want
to put the code in the !normalize_map_pos clause. If you don't care or
have pre-resolved the condition, and so don't care, you just continue as
above. (Note, the only sensible pre-resolve is that they are real).

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

Wrong on both counts. 

It does currently guarantee safeness. There is an optimization if you 
aren't worried about safeness, but this has carefully not been implemented 
in all flavours of normalize+map_pos to date.

The GUI *may* be dependent on this. I actually think it works with any
bad coordinate values, i.e. the values don't matter as long as they are 
unreal. If this is the case the optimization can probably be done.

The problems are with not understanding that normalize_map_pos is coded
in a very specific way for very specific reasons, and proper use of this
is *not* a problem.

If normalize_map_pos returns false, it COULD NOT NORMALIZE, and thus the
coordinates are unreal. Unreal coordinates should always be considered
as indeterminate, but this is not a must.

The flip is that if it returns true coordinates CAN BE and ARE NORMALIZED
hence safe to use.

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

That is the safest association for (new) code to make. But I don't think
it is required, and existing code should not be "corrected" to this
extreme position without careful checking.

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

I know, it is just it seems to be one of those four letter words that
keep automatically popping out. Such things tends to type-cast people
after awhile :-).

My comment I thought said I ignored any suggestions that had that attached
as the proper way to treat such.

>jason

Cheers,
RossW
=====