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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Aug 22, 2001 at 10:44:35AM -0400, Ross W. Wetmore wrote:
> 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.

The current version (from Trent and Jason) is cleaner than the old
one. If rand_neighbour() shows up in the top places of a profile and
you can provide a faster version fine. Just look it like an iteration
this cycle was for cleanup and the next maybe for performance.

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

Have I understand this correct that something in tilespec and the
tiles by itself depend on a certain direction schema? If this is the
case this schema should be adopted.

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

I'm sorry Ross. I looked at some of the patches and I also tought I
sent comments and so I waited for a new version from you. Looking at
the archive I have only commented on the autogame.

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

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  A supercomputer is a computer running an endless loop in just a second