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

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

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

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.

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.

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. 

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.

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.

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.

Cheers,
RossW
=====

At 03:16 PM 02/01/16 -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
>After some extended thought, I think I've come up with the correct 
>solution for the isometric scrolling problem.
>
>I won't describe it in detail here, but there is extensive comment 
>explanation in the patch itself.  As far as I can tell it now does 
>everything that it should.
>
>
>There are issues with isometric scrolling on a flat-rectangular map: the 
>scrollbars basically cover the entire bounding isometrically-shaped box 
>of the rectangular map:
>
>________
>|  /\  |
>| /  \ | <- the outer box is the scrolling area, the inner box
>|/    \|    is the map
>|\    /|
>| \  / |
>|__\/__|
>
>So if you try to scroll to (0,0), you'll wrap around long before you get 
>there - this is pretty confusing.  If you try to scroll to the top-right 
>corner, you'll hit the north pole and won't be able to make it the whole 
>way.
>
>But, as I said, these problems are all because the map doesn't line up 
>with the scrolling.  This patch separates the topology calculations from 
>the scrollbar calculations, so the problems should go away on an 
>isometric map.
>
>jason
>Index: client/mapview_common.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/client/mapview_common.c,v
>retrieving revision 1.4
>diff -u -r1.4 mapview_common.c
>--- client/mapview_common.c    2001/12/21 16:53:10     1.4
>+++ client/mapview_common.c    2002/01/16 21:51:41
>@@ -20,6 +20,7 @@
> #include "map.h"
> #include "mapview_g.h"
> 
>+#include "civclient.h" /* for get_client_state() */
> #include "mapview_common.h"
> #include "tilespec.h"
> 
>@@ -306,4 +307,192 @@
>   /* rule e */
>   last_pcity = pcity2;
>   return pcity2;
>+}
>+
>+/*
>+ * The Mapview Scrollbars
>+ *
>+ * This is common code that should enable proper scrolling to work in
>+ * all GUI's.
>+ *
>+ * In the basic case, we have a rectangular map (size xsize x ysize)
>+ * with a rectangular mapview window (size twidth x theight) sitting on
>+ * top of it.  In this case, scrolling is easy: the scrollbar may take
>+ * on a position between ([0..xsize),[0..ysize)), and the scrollbar
>+ * sliders have their sizes set to twidth and theight.  Now the user
>+ * can slide the mapview window anywhere in the range of
>+ * ([0..xsize-twidth),[0..ysize-theight)) which allows full coverage of
>+ * the window [1].
>+ *
>+ * In the isometric-view case, things are substantially more complicated.
>+ * The solution is to use "isometric coordinates" as an intermediary
>+ * between the scrollbar coordinates and map coordinates.  So instead of
>+ * having a rectangular map, we will consider it to be iso-rectangular
>+ * in isometric coordinates:
>+ *                         1
>+ *           123          4 2
>+ *   the map 456 becomes 7 5 3
>+ *           789          8 6
>+ *                         9
>+ * and we get the scrollbar positions by dividing the isometric
>+ * coordinates by 2.  The horizontal and vertical sliders both take
>+ * sizes of (twidth + theight)/2.  This means that if your mapview is
>+ * at position (6) and you scroll up, you will move up 1 scrollbar
>+ * unit to position (2).  This then gives us another problem: position
>+ * (5) cannot be scrolled to, nor is it possible to represent it as a
>+ * scrollbar position at all.  Under GTK, this means that if we manually
>+ * move the mapview to position (5), the scrollbar callback will be
>+ * called and we'll end up at position (4).  This is solved by
>+ * introducing scrollbar "offsets" so that position (5) would be
>+ * represented by the scrollbar coordinates for position (4) with a
>+ * (+1, +1) offset (note the offsets are in isometric units).  The
>+ * offsets are not changed when scrolling, so you can scroll up from
>+ * position (5) and get to position (1).  But any time you convert from
>+ * scrollbar positions to map positions (or vice versa) they are dealt
>+ * with. [1]
>+ *
>+ * [1] We deal with the fact that the bottom/right row of the mapview
>+ *     may not cover a full tile by adding on EXTRA_BOTTOM_ROW to
>+ *     the scrollbar range if necessary.
>+ */
>+struct mapview_scrollbar mapview_scrollbars = {-1, -1, 0, 0};
>+
>+/**************************************************************************
>+  This function determines the scrollbar positions and offsets for the
>+  scrollbars used by the mapview window, given the mapview's origin.
>+
>+  See comment "The Mapview Scrollbars" up above.
>+**************************************************************************/
>+void set_scrollbar_positions(int map_view_topleft_map_x,
>+                           int map_view_topleft_map_y)
>+{
>+  /*
>+   * In overhead (flat) view, this is easy: just take the window origin
>+   * as the scrollbar index.  In isometric (iso) view, it is substantially
>+   * more complicated since we need to convert to isometric coordinates
>+   * and compress them.
>+   */
>+  if (is_isometric) {
>+    int iso_x, iso_y;
>+    map_to_iso_pos(&iso_x, &iso_y,
>+                 map_view_topleft_map_x, map_view_topleft_map_y);
>+    mapview_scrollbars.x = iso_x / 2;
>+    mapview_scrollbars.y = iso_y / 2;
>+    mapview_scrollbars.x_offset = iso_x % 2;
>+    mapview_scrollbars.y_offset = iso_y % 2;


>+  } else {
>+    /* If not isometric... */
>+    mapview_scrollbars.x = map_view_topleft_map_x;
>+    mapview_scrollbars.y = map_view_topleft_map_y;
>+    mapview_scrollbars.x_offset = mapview_scrollbars.y_offset = 0;
>+  }
>+}
>+
>+/**************************************************************************
>+  This function determines the size of the sliders in the horizontal and
>+  vertical scrollbars for the mapview.
>+
>+  See comment "The Mapview Scrollbars" up above.
>+**************************************************************************/
>+void get_scrollbar_slider_sizes(int *scroll_slider_width,
>+                              int *scroll_slider_height,
>+                              int map_view_map_width,
>+                              int map_view_map_height)
>+{
>+  if (is_isometric) {
>+    *scroll_slider_width = *scroll_slider_height =
>+      (map_view_map_width + map_view_map_height) / 2;
>+  } else {
>+    /* If not isometric... */
>+    *scroll_slider_width = map_view_map_width;
>+    *scroll_slider_height = map_view_map_height;
>+  }
>+}
>+
>+/**************************************************************************
>+  This function determines the total sizes of the scrollbars (both
>+  horizontal and vertical) for the mapview.
>+
>+  See comment "The Mapview Scrollbars" up above.
>+**************************************************************************/
>+void get_scrollbar_sizes(int *scroll_xsize, int *scroll_ysize)
>+{
>+  if (is_isometric) {
>+    /*
>+     * We add on an EXTRA_BOTTOM_ROW just to be safe - note this
>+     * probably isn't correct under a different topology, but it
>+     * shouldn't be much of a problem.  We also have to divide by
>+     * 2 since the iso positions are on a different scale.
>+     */
>+    get_iso_position_range(scroll_xsize, scroll_ysize);
>+    *scroll_xsize = (*scroll_xsize + EXTRA_BOTTOM_ROW) / 2;
>+    *scroll_ysize = (*scroll_ysize + EXTRA_BOTTOM_ROW) / 2;
>+  } else {
>+    /* If not isometric... */
>+    *scroll_xsize = map.xsize;
>+    *scroll_ysize = map.ysize + EXTRA_BOTTOM_ROW;
>+  }
>+}
>+
>+/**************************************************************************
>+  This function centers the mapview given a change in the scrollbar
>+  position of one of the scrollbars.
>+
>+  There's a bit of a problem here: say a user in iso-view scrolls up,
>+  and the resulting topology adjustment causes the horizontal scrollbar
>+  to move.  Getting the GUI to update this information is very tricky.
>+  The current solution is to return TRUE if the mapview origin has
>+  changed at all, in which case we leave it up to the GUI to update
>+  the scrollbars if it deems necessary.
>+
>+  See comment "The Mapview Scrollbars" up above.
>+**************************************************************************/
>+int center_mapview_from_scrollbar(int *map_view_topleft_map_x,
>+                                int *map_view_topleft_map_y,
>+                                int map_view_map_width,
>+                                int map_view_map_height)
>+{
>+  int old_map_view_topleft_map_x = *map_view_topleft_map_x;
>+  int old_map_view_topleft_map_y = *map_view_topleft_map_y;
>+
>+  if(get_client_state() != CLIENT_GAME_RUNNING_STATE) {
>+    return 0;
>+  }
>+
>+  assert(mapview_scrollbars.x >= 0 && mapview_scrollbars.y >= 0);
>+
>+  if (is_isometric) {
>+    int iso_x = mapview_scrollbars.x * 2 + mapview_scrollbars.x_offset;
>+    int iso_y = mapview_scrollbars.y * 2 + mapview_scrollbars.y_offset;
>+    iso_to_map_pos(map_view_topleft_map_x, map_view_topleft_map_y,
>+                 iso_x, iso_y);
>+  } else {
>+    *map_view_topleft_map_x = mapview_scrollbars.x;
>+    *map_view_topleft_map_y = mapview_scrollbars.y;
>+  }
>+
>+  /* Note: the second half of this function could be unified with
>+     base_center_tile_mapcanvas. */
>+
>+  /*
>+   * Account for topology considerations.  This currently hard-codes
>+   * the topology, which is bad.  It also currently only uses
>+   * map_view_map_height, but under different topologies would also use
>+   * map_view_map_width.
>+   */
>+  nearest_real_pos(map_view_topleft_map_x, map_view_topleft_map_y);
>+  *map_view_topleft_map_y =
>+      (*map_view_topleft_map_y > map.ysize + EXTRA_BOTTOM_ROW -
>+                               map_view_map_height) ?
>+      map.ysize + EXTRA_BOTTOM_ROW - map_view_map_height :
>+      *map_view_topleft_map_y;
>+
>+  /* Finally, we update the graphics. */
>+  if (   old_map_view_topleft_map_x != *map_view_topleft_map_x
>+      || old_map_view_topleft_map_y != *map_view_topleft_map_y) {
>+    update_map_canvas_visible();
>+    refresh_overview_viewrect();
>+    return 1;
>+  }
>+  return 0;
> }
>Index: client/mapview_common.h
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/client/mapview_common.h,v
>retrieving revision 1.3
>diff -u -r1.3 mapview_common.h
>--- client/mapview_common.h    2001/12/21 16:53:10     1.3
>+++ client/mapview_common.h    2002/01/16 21:51:41
>@@ -62,4 +62,27 @@
>                               
> struct city *find_city_near_tile(int x, int y);
> 
>+/*
>+ * Scrollbar code - the GUI may call these functions to handle
>+ * scrollbars for either standard or isometric views.
>+ */
>+struct mapview_scrollbar {
>+  int x, y;
>+  int x_offset, y_offset;
>+};
>+
>+extern struct mapview_scrollbar mapview_scrollbars;
>+
>+void set_scrollbar_positions(int map_view_topleft_map_x,
>+                           int map_view_topleft_map_y);
>+void get_scrollbar_slider_sizes(int *scroll_slider_width,
>+                              int *scroll_slider_height,
>+                              int map_view_map_width,
>+                              int map_view_map_height);
>+void get_scrollbar_sizes(int *scroll_xsize, int *scroll_ysize);
>+int center_mapview_from_scrollbar(int *map_view_topleft_map_x,
>+                                 int *map_view_topleft_map_y,
>+                                 int map_view_map_width,
>+                                 int map_view_map_height);
>+
> #endif /* FC__MAPVIEW_COMMON_H */
>Index: client/gui-gtk/mapview.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk/mapview.c,v
>retrieving revision 1.111
>diff -u -r1.111 mapview.c
>--- client/gui-gtk/mapview.c   2001/12/21 19:14:18     1.111
>+++ client/gui-gtk/mapview.c   2002/01/16 21:51:45
>@@ -1724,23 +1724,35 @@
> }
> 
> /**************************************************************************
>-...
>+  This function updates the scrollbars by setting how far each one is
>+  scrolled.
> **************************************************************************/
> void update_map_canvas_scrollbars(void)
> {
>-  gtk_adjustment_set_value(GTK_ADJUSTMENT(map_hadj), map_view_x0);
>-  gtk_adjustment_set_value(GTK_ADJUSTMENT(map_vadj), map_view_y0);
>+  set_scrollbar_positions(map_view_x0, map_view_y0);
>+  gtk_adjustment_set_value(GTK_ADJUSTMENT(map_hadj), mapview_scrollbars.x);
>+  gtk_adjustment_set_value(GTK_ADJUSTMENT(map_vadj), mapview_scrollbars.y);
> }
> 
> /**************************************************************************
>-...
>+  This function sets the size of the scrollbars and sliders, for instance
>+  on startup or when the window is resized.
> **************************************************************************/
> void update_map_canvas_scrollbars_size(void)
> {
>-  map_hadj=gtk_adjustment_new(-1, 0, map.xsize, 1,
>-         map_canvas_store_twidth, map_canvas_store_twidth);
>-  map_vadj=gtk_adjustment_new(-1, 0, map.ysize+EXTRA_BOTTOM_ROW, 1,
>-         map_canvas_store_theight, map_canvas_store_theight);
>+  int scroll_slider_width, scroll_slider_height;
>+  int scroll_xsize, scroll_ysize;
>+
>+  get_scrollbar_slider_sizes(&scroll_slider_width,
>+                           &scroll_slider_height,
>+                           map_canvas_store_twidth,
>+                           map_canvas_store_theight);
>+  get_scrollbar_sizes(&scroll_xsize, &scroll_ysize);
>+
>+  map_hadj=gtk_adjustment_new(-1, 0, scroll_xsize, 1,
>+                            scroll_slider_width, scroll_slider_width);
>+  map_vadj=gtk_adjustment_new(-1, 0, scroll_ysize, 1,
>+                            scroll_slider_height, scroll_slider_height);
>   gtk_range_set_adjustment(GTK_RANGE(map_horizontal_scrollbar),
>       GTK_ADJUSTMENT(map_hadj));
>   gtk_range_set_adjustment(GTK_RANGE(map_vertical_scrollbar),
>@@ -1753,38 +1765,27 @@
> }
> 
> /**************************************************************************
>-...
>+  This callback function is called by GTK when the scrollbar is moved.
> **************************************************************************/
> void scrollbar_jump_callback(GtkAdjustment *adj, gpointer hscrollbar)
> {
>-  int last_map_view_x0;
>-  int last_map_view_y0;
>-
>-  gfloat percent=adj->value;
>-
>-  if(get_client_state()!=CLIENT_GAME_RUNNING_STATE)
>-     return;
>-
>-  last_map_view_x0=map_view_x0;
>-  last_map_view_y0=map_view_y0;
>-
>-  if(hscrollbar)
>-    map_view_x0=percent;
>-  else {
>-    map_view_y0=percent;
>-    map_view_y0=(map_view_y0<0) ? 0 : map_view_y0;
>-    map_view_y0=
>-      (map_view_y0>map.ysize+EXTRA_BOTTOM_ROW-map_canvas_store_theight) ? 
>-      map.ysize+EXTRA_BOTTOM_ROW-map_canvas_store_theight :
>-      map_view_y0;
>+  if (hscrollbar) {
>+    mapview_scrollbars.x = adj->value;
>+  } else {
>+    mapview_scrollbars.y = adj->value;
>   }
> 
>-  if (last_map_view_x0!=map_view_x0 || last_map_view_y0!=map_view_y0) {
>-    update_map_canvas_visible();
>-    refresh_overview_viewrect();
>+  /* This call handles all calculations _and_ updates the graphics. */
>+  if (center_mapview_from_scrollbar(&map_view_x0, &map_view_y0,
>+                              map_canvas_store_twidth,
>+                              map_canvas_store_theight)) {
>+    /* Danger - calling update_map_canvas_scrollbars will trigger
>+       another callback, invoking this function again (twice!).  This
>+       is less than ideal, but since the positions should remain the
>+       same it won't have any real bad effects. */
>+    update_map_canvas_scrollbars();
>   }
> }
>-
>   
> /**************************************************************************
> draw a line from src_x,src_y -> dest_x,dest_y on both map_canvas and
>Index: common/map.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
>retrieving revision 1.103
>diff -u -r1.103 map.c
>--- common/map.c       2001/12/13 19:13:16     1.103
>+++ common/map.c       2002/01/16 21:51:51
>@@ -1330,6 +1330,61 @@
> }
> 
> /**************************************************************************
>+This converts a map position to an "isometric" position.  It is effectively
>+a -pi/4 (clockwise) rotation with an associated sqrt(2) scaling, followed
>+by a shift to make sure all values are positive.
>+
>+The basic transformation is like this:
>+                  1
>+    123          4 2
>+    456  ---->  7 5 3
>+    789          8 6
>+                  9
>+but we also have to shift things because we want (iso_x, iso_y) to be
>+in the range ([0..iso_x_range), [0..iso_y_range)).  So we subtract off
>+the minimum iso x and y values to achieve this.
>+
>+In the above case, you can see that position (7) provides the minimum
>+iso X and (3) provides the maximum.  (1) provides the minimum iso Y and
>+(9) provides the maximum.  For the standard rectangle-shaped map, this
>+means the "minimum iso x and y" from above are (1-map.ysize, 0), and
>+after the shift the iso ranges both become (map.xsize + map.ysize - 1).
>+
>+There are three associated functions for this conversion process:
>+  map_to_iso_pos, iso_to_map_pos, get_iso_position_range.
>+
>+Note, this is the operation an isometric-view client will use to convert
>+from map to canvas coordinates.
>+**************************************************************************/
>+void map_to_iso_pos(int *iso_x, int *iso_y, int map_x, int map_y)
>+{
>+  *iso_x = (map_x - map_y) - (1 - map.ysize);
>+  *iso_y = map_x + map_y;
>+}
>+
>+/**************************************************************************
>+Converts an isometric position to a map position.  See map_to_iso_pos()
>+for a full explanation.
>+
>+Note, using this code for non-map-related iso coordinates is dangerous,
>+since the rounding (in the division by 2 part) may not be done correctly.
>+**************************************************************************/
>+void iso_to_map_pos(int *map_x, int *map_y, int iso_x, int iso_y)
>+{
>+  *map_x = (iso_x + iso_y + (1 - map.ysize)) / 2;
>+  *map_y = (iso_y - iso_x - (1 - map.ysize)) / 2;
>+}
>+
>+/**************************************************************************
>+Defines the range of values for isometric positions.  See map_to_iso_pos()
>+for a full explanation.
>+**************************************************************************/
>+void get_iso_position_range(int *iso_x_range, int *iso_y_range)
>+{
>+  *iso_x_range = *iso_y_range = map.xsize + map.ysize - 1;
>+}
>+
>+/**************************************************************************
> Random neighbouring square.
> **************************************************************************/
> void rand_neighbour(int x0, int y0, int *x, int *y)
>Index: common/map.h
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
>retrieving revision 1.109
>diff -u -r1.109 map.h
>--- common/map.h       2002/01/06 16:43:26     1.109
>+++ common/map.h       2002/01/16 21:51:53
>@@ -305,6 +305,10 @@
> void map_distance_vector(int *dx, int *dy, int x0, int y0, int x1, int y1);
> int map_num_tiles(void);
> 
>+void map_to_iso_pos(int *iso_x, int *iso_y, int map_x, int map_y);
>+void iso_to_map_pos(int *map_x, int *map_y, int iso_x, int iso_y);
>+void get_iso_position_range(int *iso_x_range, int *iso_y_range);
>+
> void rand_neighbour(int x0, int y0, int *x, int *y);
> void rand_map_pos(int *x, int *y);
> 
>