[Freeciv-Dev] (PR#1180) [PATCH] cleanup to canvas_pos_to_map_pos

["Guest via RT" <rt@xxxxxxxxxxxxxx>]

Sorry if you get this mail N times, I am not sure what RT would do right
now.

This is what I did: after understanding what the problem was I went and
did some analytic geometry and arrived to a simple formula:

  map_x = floor( canvas_y/TILE_HEIGHT - canvas_x/TILE_WIDTH ),

where floor rounds floats towards -infinity, not towards zero. 
Something similar is true for map_y.

Then I drew a picture (grid) and realized that this formula is rather
straightforward.

Then I compared it to the existing code and realised that it is doing
the same (upto some plus/minus signs that I lost).  Then I looked at
Jason's code and realised that he is doing the same thing too.  He also
gives a lot of explanations which originally did nothing but befuddle
me.  Jason's code is better because it does the arithmetic operations is
the correct order; in the current code there are additional corrections
which compensate for the order they round floats in and it is this
correction which for some inexplicable reason hardcoded to demand
assert(NORMAL_TILE_WIDTH == 2 * NORMAL_TILE_HEIGHT).

We are dealing with geonetry here and in geometry on picture is worth
three pages of explanations.

So I'd recommend Jason to remove his comments, draw a picture in xfig
instead (from which the validity of the above formula would be quite
obvious), put the "mathematical" version of the formula in the comment,
together with a link to the picture and commit the code.

I am off for the weekend :(
if you want me to draw the picture, you'll have to wait until monday.

G.

[jdorje - Thu Nov 14 18:02:26 2002]:

> Raimar Falke via RT wrote:
> 
> It is a slightly complicated issue, yes.  I believe the goal should be 
> to make it understandable to future readers.
> 
> To begin with, don't read the patch as a patch.  IMO the old code is 
> almost entirely incomprehensible, and trying to comare the two is
doomed 
> to failure.  Instead you should look at the code itself once the patch 
> has been applied.  You may look at the old code as well.  But don't 
> focus on whether or not the two pieces of code do the same thing - 
> again, this is nearly impossible to determine.  Try to see why each 
> separate implementation is correct on its own.
> 
> Then if you find the old code more correct/understandable, then 
> obviously I'm doing something wrong.  But I'm pretty confident that 
> won't happen :-).  And if you don't understand the new code, then why 
> not?  What more can be said to make the explanation clearer?
> 
> The transformation used here is at the heart of the use of isometric 
> view.  You may also want to take a look at map_pos_to_canvas_pos,
which 
> does the opposite transformation (it is a bit simpler).  There are
also 
> corresponding function city_pos_to_canvas_pos/canvas_pos_to_city_pos, 
> which are special cases of this conversion (and thus also simpler).
> 
> jason
>