[Freeciv-Dev] Re: update for isometric scrolling problem (PR#1222)

[jdorje@xxxxxxxxxxxxxxxxxxxxx]

Ross W. Wetmore wrote:

> There is too much code duplication in this. For example, there should not
> be a separate center_mapview_from_scrollbar() and center_tile_map_canvas()
> that do essentially the same operation, i.e. update map_view_{x0,y0}.
The problem is that center_tile_mapcanvas does two operations: takes a 
center position (x,y) and finds the origin (x0,y0) from it, then changes 
the mapview to match.  The scrollbar code starts with an origin - so to 
unify will require the creation of a separate function that will be 
called from both.  Eventually this will be desirable, but it's not going 
to all be accomplished in one patch.
Note this "code duplication" is there now; just look at 
scrollbar_jump_callback().  I've just moved the code...

> The fact that you really don't fix the corner cases means that more work
> is required on this patch before it is ready for CVS. 
How do you suggest fixing the corner cases?

In the patch as it is I just step in the appropriate direction on a 
scroll and then call nearest_real_pos.  In the corner case for the 
current topologies, this means you step 1 unit diagonally (from the map 
perspective) and then move to the nearest_real_pos so that the effect is 
of moving 1 unit cardinally (from the map perspective) or 1 unit 
diagonally (from the mapview perspective).  This part should also be 
topology-independent.
Getting this to work with the GUI, which then needs to update itself in 
such a case is pretty tricky.

> I think it is a mistake to introduce a number of the incomplete topology
> operations in this way. Too much will need to be ripped out and redone
> when it is finally done right. Doing a half-assed job instead doesn't
> make a lot of sense and will just clutter the codebase unnecessarily.
Actually, I'm not introducing any new out-of-place topology operations, 
just moving them from gui-gtk/mapview.c into mapview_common.c.
One advantage of this patch is that it can unify the scrollbar code. 
This means a future patch (assuming the same interface, which seems 
reasonable given the different GUI setups) will need only deal with this 
code.  (Of course, this patch doesn't unify the scrollbar code.  But it 
will be fairly trivial to do so once this code is in place.)

> An alternative to all the messy iso tricks is to attach the scrollbars
> to the overview map and have them move the center of the overview_rect.
>

> I'm not sure I like this for some things, but it is a more natural map
> movement with a clear object that is affected by the sliders. And it
> keeps the current context of the scrollbars intact with no conceptual 
> change that depends on oddities of tileset views.
> This doesn't do as radical a change to the way things work as you are
> trying to do, and consistency with the current code, while moving the
> obvious disconnect associations into a more connected context will
> also fix the issue.
>
> A useful feature to this is that the screen real estate around the 
> map_canvas is freed up, removing some of the need for EXTRA_BOTTOM_ROWS
> and such, while space around the overview is more compact and usable.
An interesting idea.  But this has many problems of its own, most of 
which are unsolvable (AFAICT) GUI issues:
- Most people expect the scrollbars to be attached to the main window.

- Attaching the scrollbars to the overview will give the exact same 
behavior as we have right now.  The only advantage is that the user can 
see the overview window is actually moving in the correct 
direction...but he's probably looking at the mapview anyway.
- Under the corecleanups implementation of the overview, this would mean 
scrolling moved properly along one axis and in a zig-zag along the 
other.  Way bad.

> The x_offset/y_offset elements are unnecessary. There is no need to
> "compress" the iso coordinates of the scrollbar if in fact you always
> uncompress them before using. In any event, only a single offset is
> required as the x and y values are constrained by the topology to be
> in phase or out of phase (sum is even or odd) depending on your choice
> of map origin. 
My first implementation was how you suggest, and I quickly realized it 
wouldn't work.  Compression must happen somewhere because there must be 
a concept of a "unit" of movement that can be accomplished with a single 
click on the scrollbar.  Every GUI (that I've seen) behaves in the exact 
same way in this regard (albeit with a different interface), so the code 
might as well go into mapview_common.
In other words, doing it that way meant you had to click twice on the 
scrollbar (endpoints) to get a single unit scroll.

> There is something wrong with your map_to_iso_pos() and reverse 
> transforms. These functions should definitely not have map.ysize
> dependencies. You need to figure out where the dependency really is and 
> fix it there, i.e. in the GUI part. Note map_view_topleft should be at 
> window 0,0 or an EXTRA_BOTTOM_ROW or so away.
The map_to_iso_pos is a global operation to transform a normal map 
position into a *positive* (x,y) isometric position.
 ____           _____
|    |         |*/\  |
|    |    ->   |/  \ |
|____|         |\   \|
               | \  /|

              |__\/_|


In other words, for this topology we are transforming the flat-rectangle 
given in the first picture into the iso-rectangle given in the second 
picture (the inner rectangle).  Because we wish to have all x,y>=0 after 
the transformation (a simple enough requirement), a shift is necessary 
and this depends on map.ysize (look at the picture and you'll see why). 
 All of this is explained in the code (and better than I have here, 
although I say "positive" instead of "non-negative" - oops).
This requirement is quite useful to the GUI, but placing the added 
restriction on the conversion is quite reasonable.  The alternative is 
to introduce another function get_iso_position_minimum() which the GUI 
can use to do its own conversions.  This would be uglier at both ends.

> get_iso_position_range() is a poor interface choice, both by name and
> function. In general it is probably undesirable to force a constraint on 
> x and y like this. It is an example of a partial topology hack that will
> almost certainly vanish.
Please explain.  It seems to follow naturally from the definition of the 
isometric conversion.

> Nit:
> You are still calling 2-D maps flat which is redundant, or incorrect
> if you are referring to a standard WRAP_X cylinder world, and not
> restricting these scrollbar functions to a true Flat-Earth topology :-).
>
> Until the terminology is formally agreed on, you should probably not
> put "flat" into code or code comments.
I only used the word "flat" twice, and in neither of them did it 
describe the map or topology.
In the e-mail, I said "flat-rectangular" (which should be obvious as the 
inverse of iso-rectangular).
In a comment in the code I said "overhead (flat) view".  This is just to 
distinguish it from overhead (isometric) view.  Although most people 
just call the standard view "overhead", this is misleading in a formal 
sense because isometric view is overhead too.
Some discussion and agreement would be good.  Is there a better term 
than "flat" to use as the opposite of "isometric"?
jason