[Freeciv-Dev] Re: [PATCH] more small directional cleanups

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

At 12:46 AM 01/08/22 -0400, Jason Dorje Short wrote:
>"Ross W. Wetmore" wrote:
>> 
>
>> We are close. This is the version in corecleanup_06c, slightly more
compact.
>> Note normalize_map_pos, does the is_real_tile check, and only normalizes
>> on the way out when it returns true. Local vars are probably not that much
>> of an improvement, since there is just one assignment in typical case, so
>> it might as well be the right one.
>
>You're right, but I think Trent's loop is even better.

I actually like Trent's coding tricks, and if I thought there was a real
problem here to solve I would go for them.

But his code is about 2-3 times more expensive for the standard case and
I think that the problem of calling rand_neighbour() to generate a random
adjacent tile when there are no adjacent tiles is a programming error that
should be fixed by the caller, and not caught by the runtime. Whether you 
assert or run into an endless loop in this case, you will probably notice 
the bug.

If you really are interested in optimizing the loop for the 3% or so 
cases where the random sequence might take a few more iterations to hit
an acceptable tile, I think you are optimizing the wrong hotspot at the
expense of the right one.

>> In the second one, you break tilespec by using DIR_DX directions without
>> also fixing up the hardcoded ttype_north = ttype_near[0] lines just after.
>> If you load up a client and find the coastlines have odd sandbars, you will
>> know you mussed up tilespec.c.
>
>No, I haven't switched to using the DIR_DX directions...I've switched
>DIR_DX to use the DIR_DX2 directions.  This means everything works.
>
>However, those magic numbers should be replaced by the proper
>enumeration value.  This cannot be done until DIR_DX2 is unified with
>DIR_DX, though.  Hmmmm.
>
>> The correct patches for this are in corecleanup_06b, where the switch is
>> from the original DIR_DX2 to DIR_DX2(NW origin). Note. this is the only
>> place in the GUI where DIR_DX2 directions were used and it was (locally)
>> coded correctly.
>
>I wouldn't call this "correct", since all you've really done is replace
>one set of magic numbers with another.  It's a good example, but you
>should use enumerated values instead.

You are correct on both counts. My examples were poorly worded, didn't 
properly highlight the point I was trying to make and contain their own
obvious shortcomings that can be picked on instead.

But I think you sort of got the point that tilespec.c has locally coded
consistent direction mappings that become dangerous when the direction
array is amalgamated with a global system, and the magic numbers aren't
(which includes my fixes as well since they increased the global aspect
of DIR_DX2).

>> The GUI must use DIR_DX, as there are datafiles for graphics components
>> that have been laid down in this order. Up until your previous patch the
>> GUI was self contained and completely separate from the core server DIR_DX
>> stuff. I actually reversed the changes in corecleanup_06b in the GUI when
>> all the DIR_DX2 directions were propagated into the main server. I left the
>> name as DIR_DX2, and didn't delete the DIR_DX pieces until all the GUI
>> implications were resolved.
>
>I don't quite understand this paragraph.  Are you saying the DIR_DX
>directional system is necessary for the GUI code, and that you fixed
>this?  I didn't have any problems when I changed it (although I have not
>tried changing it since integrating with DIR_DX2, which wouldn't work as
>you explain above).

Try to do goto lines after you switch to DIR_DX2. I think you will find
that changes you made to consolidate the GUI direction handling with 
DIR_DX will now break the GUI when you use DIR_DX2 order. The places where
I backed out your initial patch changes are in corecleanup_06b, goto.c
mapview.c(s) and mapclass.c.

Another reason to keep the origin at NW == (0,0) in a DIR_DX2 system is
that at least the first few entries overlap with the original DIR_DX. So
it is a minimum change fix with less risk or performance impact.

I don't think we should do DIR_DX2 changes twice, so we should agree on
the final rotation sequence, start and end up front.

>> Likewise most of the GUI normalize_map_pos fixes and a few more are in
>> corecleanup_06a.
>
>Yes...I had done that work before I saw your patches (although it was
>just the work of a few minutes).
>
>> But, when both of us do pretty much the same thing, it is clear that it
>> probably needs to be done!
>
>Certainly.  The question is how best to get into CVS.  Are your patches
>stable enough to be applied, or are more breakdowns necessary?

The corecleanup_06a patches are just the bugfixes and "non-controversial"
changes like iterate macros, map_adjust_* to normalize_map_pos and removing
explicit unchecked (x+1,y-1) map accesses. Apart from slipping in a 
city_map_iterate_checked() to handle the normalize_map_pos stuff in pcity 
accesses correctly, there should only be changes that are needed for the 
current code base.

These changes have run in probably over 200 hours of autogaming, and are
more often than not the fix for an autogame assert.

It is good that two pairs of eyes are turning these up, and at least two
separate viewpoints involved in the fix. But we should probably consolidate
the patches into a single joint submission, rather than have overlapping
sets.

>BTW, the commit of dir-patch1 means some of the corecleanup patches will
>no longer apply cleanly to CVS.  Hopefully CVS can cleanly merge the
>changes, which should practically duplicate what you have...
>
>jason

Yes, they were consistent for DIR_DX orders, but not correct when you move
beyond. I don't see a problem with this as long as the intermediate points
were stable. This is part of the reason I didn't raise the issue until we
go to the next stage where it mattered.

Corecleanup_06a is a more comprehensive cleanup that apart from the things
it inherited from the first dir_patch, tries to do what is needed for the
long term with as few intermediate sidetracks as possible. But this sort of
comprehensive patch appears not as easily accepted by the maintainers as your
quickie stepwise versions. I haven't had any real feedback or direct
acknowledgment of these changes yet, though if the clock started running
officially last Sunday, it is not unreasonable for someone to still be
trying to digest the full meal. It is also overlayed on your dir_patch and
I haven't seen this go in either so there is a potential queuing delay.

IMHO the best thing to do is discuss the set of changes and make sure that 
any gotchas are resolved. This will also help anyone putting it into CVS 
as many answers to their questions will already be out there. And if they
add questions to the process, they can be resolved as well.

Cheers,
RossW
=====