[Freeciv-Dev] Re: what's broken and what's not (was: Re: remove map_adj
[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
At 04:32 AM 01/12/14 -0500, Jason Short wrote:
>Ross W. Wetmore wrote:
>
[...]
>>>>normalize_map_pos to wrap, since it may change the x and y coordinates
>>>>of an unreal position.
>>
>> Only because you broke normalize_map_pos(). Fix that function and you
>> won't have this problem, as well as improving its performance.
It has been broken for a number of months. Gaute, may have done the
worst damage. My apologies for using a generic second person instead
of passive structure :-).
>First off, I haven't changed normalize_map_pos at all. But note,
>though, that it doesn't specify what values x and y become if it returns
>0. It is undefined.
First problem, though this is the current state :-).
> So if any code uses the values after it has
>returned 0, that code is wrong.
No, the function should specify its interface and in this case there is
a useful way to do this. Besides, *my* normalize_map_pos() does this, so
such code is correct for the corecleanups, care to join me :-?.
> There's a good reason for this, too: if
>you _were_ to specify what the x and y values were to become, how would
>you define them? (1) unchanged,
This is the obvious choice which follows all Unix standards and norms
such as cases like the infamous errno.
If you can't normalize because the coordinates are unreal you certainly
shouldn't change them to something else. Besides, the test for realness
is quick and doing this first means means you don't waste cycles doing
the hard computations until you know you both need them and can do them.
Note, the alternative here is to explictly separate the is_real() check
from normalize_map_pos(), then you do both stopping before you munge
the values even with a broken normalize_map_pos().
It is actually a fundamental error to try to normalize something that
is unreal, so doing things as done in CVS is both practically and
conceptually wrong. It is no wonder this causes problems :-).
> (2) x value wrapped, y unchanged, or
>(3) x value wrapped, y value clipped? All three of these have valid
>arguments in favor of them.
Yes, all various flavours of void_tile-ism in some of its most insidious
forms :-).
Doing spurious bogus fixups, just makes for endless buggy code which
has been the subject of months of cleanup operations. There is certainly
no point reintroducing such practices, no matter how convenient the
void_tile argument sound for covering up general sloppiness.
In all these cases, you can't normalize, so you should return FALSE and
let the calling code handle the error condition, aka fixup as appropriate.
map_adjust_y() dies really hard though, and mutates (nearest_real_pos())
faster than it can be stomped out.
> The only conclusion is that all should be avoided.
No, it should be specified, then used as specified :-).
>> In general, put_tile() functions should take any coordinates and do the
^^^^^^^^^^ ^
>> normalization themselves. They are called enough, that duplicating the
>> normalization and realness checks everywhere is a mess of code bloat.
>>
>> It is also the case that you should let put_tile() functions put black
>> tiles for unreal positions that lie in the gui window, for this you
>> need to give it unreal coordinates. Having separate code to handle the
>> unreal cases is foolish duplication. (See corecleanup_10)
>
>Fortunately, I don't think the code has this problem anywhere. The
>drawing code in put_tile() is something like this:
Unfortunately you didn't realize I was agreeing with you, and explaining
why this is only one example of a larger assertion, particularly the
extra normalization checks :-).
But you are forgiven ...
>
>so AFAIK there is no code that is "bad" in this regard.
>
>--------------------------
>
>But here's how the drawing code is broken, though, and I think you'll
>agree with me on this. There is currently one function, get_canvas_xy,
>that converts a map position to a canvas position. It is intended to
>take into account everything (wrapping, realness, etc.) and return the
>"correct" canvas position.
Yes
> Unfortunately, what the correct canvas
>position is depends on how we're using the function.
No.
It depends on the inputs to the function, not the intent. Computers are
pretty pedantic or reproducible about such things - you only get one
value back. You need to make sure the inputs you give will return the
right values for what you want before using it.
>There are two ways the GUI draws to the screen. The first is when it
>loops over "absolute" GUI coordinates, drawing a tile for each position
>to update a whole segment of the screen. For each position, it should
>take the gui coordinates (gui_x, gui_y), convert them to map coordinates
>(abs_x, abs_y),
Yes.
> and convert them to canvas coordinates (canvas_x,
>canvas_y).
Yes and no. At least, only if it doesn't care about the original canvas
coordinates. And there may be a slight misfit here in the example seems to
focus on non-canvas coordinates, i.e. (un)scaled to tile granularity,
and thus is using this to do your scaling. If that hurts, don't do it!
> Since we're talking about a conversion from _gui_
>coordinates to canvas coordinates, no wrapping should be necessary, and
>unreal tiles should be drawn correctly.
Yes, if you want just scaling, then just scale, and don't expect some
other incantation to magically solve all problems.
But in this case, the GUI is doing the correct thing. Tiles should
only be drawn once. Thus when you loop over all positions in a "big"
GUI screen, you want to only draw the tiles to the position that
corresponds to a single unnormalized set. Thus you need the unnormalize
function in the algorithm.
> We only do a normalize_map_pos
>at the very end to figure out what it is we're looking at.
Normalization is almost always the first thing you do to a map
coordinate, so I'm ignoring the "end" part as not understandable.
I will assume you mean to lookup the tile values from storage to be
precise about which layer requires the normalization, instead of being
vague and getting into trouble because you forget later why this was
needed (and ok for this), and start applying it to something else ...
... like the unnormalization step above which incidentally doesn't
care whether the map coordinates were first normalized or not.
> If we want
>to limit ourselves to drawing a tile only once, we should (but don't) do
>it by limiting the bounds for the loop.
Actually, not really true.
First, the best way to limit tiles to being drawn once is to make
sure you unnormalize them all to a given set. Of course you pick this
set so it overlaps with your GUI window, i.e. has +ve offsets from
the GUI window origin in GUI coordinates which means is aligned with
the GUI window axes.
You can also limit the clipping in the final normalize_canvas_pos() or
equivalent is_real_canvas_pos() since the canvas topology has no wrap.
You can loop over anything you want, and it is just potentially throw
away work as you state.
You will find that the EXTRA_BOTTOM_ROW stuff actually makes this
boolean condition apply to a slightly larger window than it would
otherwise. But this is GUI specific and does not change underlying
transformations or layers.
>The second way is that we have a map position (map_x, map_y) that we
>wish to update. In this case we need to convert this to a gui position,
>using an appropriate wrapping function
>(find_representative_map_pos/unnormalize_map_pos), then do a straight
>conversion from this to canvas coordinates. Here we don't even need to
>worry about unreal coordinates.
Well, assuming you already worried about them, you don't need to again.
This is exactly the same as the above, i.e. you want to take a given
map position (no matter how you got it) and put it into the one GUI spot
in the appropriately chosen normal set.
>Now, these two cases are very different and should be handled by
>entirely different code.
The case of scaling GUI coordinates to canvas postions vs unnormalizing
GUI coordinates corresponding to a unique map position to a viewable
canvas position, are different, you are correct.
> The problem (and it's a big one) is that
>get_canvas_xy is supposed to handle both cases. AFAICT this was
>originally written this way as an "easy" way to avoid drawing a tile
>multiple times: since get_canvas_xy will wrap coordinates, we don't even
>have to worry about bounding the loop in the first-case drawing
>scenario.
You are correct, technically, it shouldn't be used for pure scaling.
Except that if the conditions for unnormalize (like normalize) do not
touch the coordinates unless they can change them and you do your code
architecture right, then scaled unreal coordinates just sort of fall
out as a side effect and everything works swimmingly.
Until, there is a counter case. It is a pity to have to fix this to
do things in a more complicated correct way :-).
> But the result is that get_canvas_xy has to both (1) check if
>the coordinate is real, and if not then just treat it as-is and (2) wrap
>it into the appropriate representative set (using an appropriate
>wrapping function).
Normalize and unnormalize do this for you if they aren't broken :-).
>The result is that writing get_canvas_xy using topology functions
>(namely, find_representative_map_pos) is very ugly and probably very
>slow as well. This will become apparent as soon as I propose a patch to
>make this change.
It is a lot cleaner without find_representative_map_pos() plus one
doesn't have to figure out what that means or pronounce it.
if( unnormalize(&map_y, &map_y, view_x0, view_y0, is_iso) )
then use new map_x, map_y
else use old map_x, map_y
transform_and_normalize_canvas_pos(map_x, map_y)
is the basic algorithm which is equivalently slow or ugly in either case.
I think your argument boils down that to use
find_reprehensible_something_or_other() you have to hack a lot of
current code (i.e. the callers).
If you choose a different way that is more logical and cleaner, you
don't have to hack much existing code (i.e. where called).
This seems to point more at find_repugnant_algorithmic_solutions() than
at get_canvas_xy(), no?
>jason
Cheers,
RossW
=====
|
|
