Hmm this is strange, my newest patch should have had a highly reduced
number of variables in the formula but still, much of what you said is true.
For the most part, the formula for my calculations should look something
like this: "tileY = (y / tileSize * MIN_TILE_SIZE);" rather than that
admitted longer version that you seemed to have used.
I based my formulas off the original code and modified it after finding out
many of the variable values during runtime. Indeed, however, as you said
the problem probably is not just on the focus method and that much of the
code elsewhere could use some refactoring but that stuff is beyond me for
now.
My goal was just to fix the way the minimap focus worked so that the just
the focus would work correctly. I apologize for being lazy in this aspect,
but I do not have the time to go in change (probably all of) the minimap
class. Well at least without introducing new bugs, which I highly want to
avoid. Thank you for understanding.
On Sun, Feb 10, 2013 at 7:08 AM, Pedro Rodrigues <[email protected]>wrote:
> On Sun, Feb 10, 2013 at 12:18 AM, Jonathan Simona
> <[email protected]>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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/freecol-developers
>
>
------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/freecol-developers
