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

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

At 11:37 PM 02/01/17 -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
>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 want to use the "head" of the scrollbar rather than the
"middle" as your working value here should not require you to duplicate
all the processing code from this point down.

You are the one now duplicating it, no matter where you claim you got it 
from :-). And presumably the purpose of this patch is to eliminate a lot of
the duplication of code, so it shouldn't be creating more duplicate 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. 

Key signal for you to suspect that this algorithm has bugs it is just 
trying to coverup.

> 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.

Tricky or not, it is required if you are going to change the semantics
of the scrollbars. This is part of the change, and the change should not
go into CVS pre-broken.

Personally, I would just assume the scrollbars moved the centre of the
overview_rect, call centre_tile_map_canvas() with these coordinates and
let it do the normal things it does to deal with this case. It might be
useful to reset the scrollbar variables after this returns so they
reflect the current view of mapview_{x0,y0} rather than letting them try 
to reset it next time they are called, ad nauseum.

The advantage of this is that there is only one place to deal with such
problems, and no need to chase down all the oddities because part of the
interface does something differently than another part, or is broken
because it was too difficult for the submitter to figure out 
yet-another-local-hack at this point.

>> 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.)

The functions you are putting into map.c were the ones specifically cited.
This answer doesn't address the issue.

>> 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:

Just like your "unsolvable" corner case above? 

I think that this AFAICT sort of response typically means one does not 
yet understand, or is not willing to figure out any alternative ideas but
the original one.

This probably means you haven't really thought the problem through enough
to convince people that you have not only a workable solution, but one
which is in some sense the proper solution for the longterm.

Looking at alternatives with pro/con analysis used to decide which is
preferred avoids a lot of the immature subjective debates about whose
way is more "ugly", and the need to revert things when the real 
technical problems start to surface.

>- 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.

This is a very confused analysis of something. Would you care to explain?

BTW: in the corecleanups, the overview is in "native" coordinates so all
the overview maps we are dealing with are rectangular and aligned with the 
overview axes.

The scrolling for true isometric map works exactly as it does in the
current standard map. Things move off to the left and back on at the right
at the same y-value.

Only in the generalized-topologies do you have a pi/4 rotated overview
rectangle where one "zigzags" or jumps from the bottom half of the screen 
to the top when you wrap over one rectangle edge into the opposite one.

This is one of the problems you have with doing things like overview,
savegame or debug print in "regular" coordinates.

>> 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.

So fix the click (one spot) to understand the granularity concept, and 
simplify the rest of the code, rather than all the complications to the 
rest of the code so you don't need to fix the click.

>> 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.

I don't know what it follows "naturally" from. The function name is like
no other. The constraint between the x and y dimensions is certainly not
a "natural" one for a map, or in any way useful as a general interface.

>> 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.

They should not be used in code or comments until there is a clean
definition and concensus as to what the terms mean.

>In the e-mail, I said "flat-rectangular" (which should be obvious as the 
>inverse of iso-rectangular).

In discussions everyone has a vague understanding of what your current
meaning is, although it would be better if there was a standard term
we all agreed on and used consistently.

>In a comment in the code I said "overhead (flat) view".  This is just to 
>distinguish it from overhead (isometric) view. 

This should be removed. It is an example of slipping partial things into
CVS that just need to be corrected as soon as they are discoverd or used.

> Although most people 
>just call the standard view "overhead", this is misleading in a formal 
>sense because isometric view is overhead too.

In the "overhead" view it is still overhead. Once the topology patch is
in and used and people are able to play on a "natural" isometric map 
instead of seeing it only as your internal "regular" or "standard" map 
view of the current non-isometric map, I suspect some of the confusion
about overview/standard/isometric will decrease.

Currently it is always the "unnatural" way to view the maps, and I think
this has a lot to do with its lack of acceptance.

>Some discussion and agreement would be good.  Is there a better term 
>than "flat" to use as the opposite of "isometric"?
>
>jason

The only term other than "flat" you were happy with was "map" because
this is the internal prefix for this coordinate system. But map maps are
not a particularly intuitive concept.

My terms are "game" or "standard". I tend to use "game" as in game
coordinates when referring to the internal working representation and
operational concepts. I use "standard" as in standard map to refer to
one that reflects the current rectangular grid with every grid point
a valid map tile. 
True/natural isometric or hexagonal maps use every other grid point of a 
standard map for instance, and if transformed to standard map coordinates
i.e. the internal working game coordinates, produce the pi/4 rotated 
rectangle/shape.

Cheers,
RossW
=====