[Freeciv-Dev] Re: (PR#7287) Extended Topologies

["rwetmore@xxxxxxxxxxxx" <rwetmore@xxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=7287 >


Marcelo should provide a proper writeup explaining a lot of the coding
tidbits like the ratios and the changes to current practices, or at least
beef up the local code commentary.

For the most part the patch needs a lot more work in such areas so that
a reviewer can actually understand the logic as opposed to implementation.
I started to write review comments, and then realized it would amount to
a rejection of just about every line as being incomprehensible or breaking
any number of current practices. His implementation seems different merely
for the sake of being different in some spots, and that needs to be reversed.

For example, I think the breakup of normalize_map_pos() into virtual
functions has completely masked the concept of what a normal set is and how
this function deals with it. It is now a random collection of wrapping
optimizations that have no clear relationships between them.


The wrapping extensions are good though. They just need to be thought
through from a more generalized perspective and documented as ways to
do the same basic operation, but with different outcomes and
implications. The pictographic layout of tile relationships that this
produces would be a good start to making the sell, for instance.


Cheers,
RossW
=====

Jason Short wrote:
> <URL: http://rt.freeciv.org/Ticket/Display.html?id=7287 >
> 
>>[mburda - Sat Feb 07 14:31:54 2004]:
> 
> 
>>I wait for your comments.
> 
> 
> First of all, the patch needs to be split up.  You can start doing this
> now, if you want.  One example: the city_map_iterate_known macro can
> become its own patch.  There are many other examples.
> 
> Next, I'm not happy with the xsize/ysize/size/ratio setup.  If you
> change the xsize nothing happens, and you can't figure out why.  At a
> minimum the user needs an error message here.  (This is also something
> that should be its own patch.  I'm not convinced that we need this.)
> 
> The topology_id isn't fully explained (in "help topology").  What are
> topologies 5-15?  Why don't we have 22-31?
> 
> I'd like to set up the topology_id as a combination of bitfields:
> 
> Xwrap: 2 bits, 4 values:
> Ywrap: 2 bits, 4 values:
>   NONE, SIMPLEST, MOBIUS, REVERSED
> iso: 1 bit
> 
> This is similar to what you have (I think), but the bit order is different.
> 
> so we end up with 32 different topologies (as you have).
> 
> Finally, examples.  The biggest problem we have is convincing people
> that we should provide these topologies.  The biggest selling point will
> probably be the quincunx topologies, which make a good approximation of
> a sphere.  But I can't figure out how to turn your earth scenario into a
> quincuncial earth, since I don't know what the topology values are.
> 
> jason