Complete.Org: Mailing Lists: Archives: freeciv-dev: January 2002:
[Freeciv-Dev] Re: update for isometric scrolling problem (PR#1222)
Home

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

[Top] [All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
To: freeciv-dev@xxxxxxxxxxx
Cc: bugs@xxxxxxxxxxxxxxxxxxx
Subject: [Freeciv-Dev] Re: update for isometric scrolling problem (PR#1222)
From: jdorje@xxxxxxxxxxxxxxxxxxxxx
Date: Thu, 17 Jan 2002 23:37:08 -0800 (PST)

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




[Prev in Thread] Current Thread [Next in Thread]