On Sun, Feb 10, 2013 at 12:18 AM, Jonathan Simona <jssimona2...@gmail.com>wrote:
> I think that I finally solved, well pretty much solved, the minimap bug. I
> cut down much of the formula to just the bare minimum but still had to keep
> 1 magic number because it was part of the original code. My patch
> acceptance is due this Monday, so I apologize if I sound like that I am
> rushing but I understand if I must wait.
>
> Thank you.
>
Sorry everyone for the long post, but its necessary to make the point
clear.
You are relying too much on trial&error to fix this, instead of trying to
understand the code and math behind it (which i admit is hairy).
I can tell just by looking at your code that the second part of the
exception (which im not entirely sure an exception is needed, but have not
reached a definitive proof of) is wrong:
By analyzing your code line mathematically, one can detect a couple of
errors:
"tileY = ((y - adjustY) / tileSize * MIN_TILE_SIZE) + firstRow + adjustY";"
Lets start by checking the meaning (and units, very important) of each
variable of the formula:
- tileY is a value defined in units of tiles;
basically we want to find the Y coordinate of the tile in the main map
corresponding to the clicked one in the minimap
- y is the y coordinate (in pixels) of the mouse click in the minimap panel
and adjustY/adjustX is also expressed in pixels, to adjust for a possible
border (here there is some confusion in the calculations both in this class
and in GUI, where its built) and empty space (which is why both values are
normally 0 at lower zoom level values - which measure the zoom OUT, see
Minimap::setZoomOption() - where the minimap normally fills the entire
panel space assigned to it)
- tileSize is the tile size (in pixels) that a tile has in the minimap at
the current zoom value, and MIN_TILE_SIZE is the minimal size (in pixels) a
tile in the minimap can have.
- firstRow/firstCollumn (calculated elsewhere) are derived values (in tile
units) and have the x/y coordinates of the tile in the map that corresponds
to either topleft tile in the minimap (0,0 in minimap coordinates) or the
closest to it, if space is added to the minimap (which is where adjustX and
adjustY will enter, by pointing towards the first pixel of that tile in the
minimap)
- adjustX/adjustY are, like explained above, values (in pixels) used to
adjust for possible borders (which again seem to be sometimes considered in
calculations, sometimes not) and space added (if the zoom is such that the
minimap is smaller than the area it can occupy in the panel), so that the
topleft most pixel of the topleft most tile in the minimap is found.
(all these result from a study of the code that goes toward that method,
which is why i said you would need to look deeper; that is also required
since the variable names arent explicit enough which give rise to
calculation errors like the one below).
This done, we can now start to decompose the formula:
- (y - adjustY) is clearly used to normalize the minimap to put the topleft
most pixel of the topleft tile next to Y axis (that, along with "x -
adjustX" would normalize the minimap to the X/Y axis, with (0,0) in the
topleft most part of the minimap space, as per usual in computer graphics;
note that im assuming normal axises, see note below about isometric view);
also of important notice that the result is still expressed in pixels since
we are subtracting pixel values
- (tileSize * MIN_TILE_SIZE) is one of the problems i see, since it appears
to make no sense (to me at least) why we would want to multiply the current
tile size (in pixels) of a tile with the minimal value of pixels per tile;
i think that you wrongly associated the "4" there to MIN_TILE_SIZE, and is
indeed one such instance of that mysterious magic number instead.
- dividing (y - adjustY) by tileSize (per the previous expression) gives
the y coordinate (in tile units) of the tile clicked in the minimap
- adding firstRow to the previous result should translate to the y
coordinate (in tile units) of the corresponding tile in the main map (which
is where the previous formula ended, with "tileY = ((y - adjustY) /
tileSize * 4) + firstRow ;" (which makes mostly sense, except for that
mysterious 4 value.
- (1) but you also add "adjustY", which is where i can tell theres a
mistake because you are adding values of different units (tile units +
pixel units), which, by basic laws of math, you should not do (unless you
want garbled results).
Regarding the mysterious "4", it either is an element of isometric math
(which i know all calculations until now have ignored the isometric nature
of the minimap, and the lack of graphic math knowledge is whats probably
keeping me from getting it) or an extraneous value to increase the size of
the minimap area (see Gui::createMiniMapThumbNail() for the use of such
value in calculating the height of the area of the minimap), and not
clearly marked as a named constant.
Im also betting (although without much proof to support it) that the
problem stems not only from this method (Minimap::Focus()), which is
wrongly ignoring the isometric nature of the minimap, but also from
possible bad calculations of the adjustX /adjustY, done elsewhere.
Anyway, at least from (1), i know your formula (and thus the patch) is
wrong.
I urge you to, based on the preliminary findings i did above, recheck my
findings and fix the patch (since you have been working on it, and of it
depends an actual good work regarding your assignment, if it matters at
all).
I also ask that someone with deeper knowledge of graphic math to assist
you, since i already humbling declared my lack of knowledge on the subject
and lack the time to do a more serious study on the matter (which i will
try to find, as it may be relevant again in the future).
------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb
_______________________________________________
Freecol-developers mailing list
Freecol-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/freecol-developers
