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

[Jason Dorje Short <jshort@xxxxxxxxxxxxx>]

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

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

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

> Don't use % 8, use & 7 to mask off the dirbits. Divide operations are
> expensive compared to logical bit ones.

Good point.  I had thought the compiler should be able to optimize it,
but I now realize the two are not synonymous.  I hate C's definition of
the % operator!

> The aiunit code should be a square_iterate, as I think it is in
> corecleanup_06a.

Outside of the scope of this patch.

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

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