[Freeciv-Dev] Re: get_canvas_xy unification (PR#1054)

[jdorje@xxxxxxxxxxxxxxxxxxxxx]

Raimar Falke wrote:
> 
> On Sun, Nov 11, 2001 at 08:21:04PM -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
> > Raimar Falke wrote:
> > >
> > > On Wed, Nov 07, 2001 at 12:07:45PM -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx 
> > > wrote:
> > > > Andreas Kemnade wrote:
> > > > >
> > >
> > > > +/**************************************************************************
> > > > +Finds the pixel coordinates of a tile.  Beside setting the results in
> > > > +canvas_x,canvas_y it returns whether the tile is inside the visible 
> > > > map.
> > > > +**************************************************************************/
> > > > +int base_get_canvas_xy(int map_x, int map_y,
> > > > +                    int *canvas_x, int *canvas_y,
> > >
> > > > +                    int map_view_x0, int map_view_y0,
> > > > +                    int map_win_width, int map_win_height)
> > >
> > > It looks like map_view_* is a map position and map_win_* are
> > > pixel. Correct? Can this be clarified in the variable names?
> >
> > Yes, but how?  I really haven't been able to think of a good naming
> > system.
> 
> map_view_topleft_map_pos_[xy]
> map_view_pixel_{width,height}

A bit lengthy, but it could work.  We also want
map_view_tile_{width,height}, which can be easily determined from
map_view_pixel_{...}, but sometimes we don't want to go to the trouble. 
Most GUI's track both values anyway.

> Maybe we can also pass "map_win_width/NORMAL_TILE_WIDTH" instead of
> "map_win_width".

isometric-view get_canvas_xy (map_pos_to_canvas_pos) needs to know more
than this to determine if the tile is on-screen, since tiles are spaced
only NORMAL_TILE_WIDTH/2 apart.

> > > > +/**************************************************************************
> > > > +Finds the map coordinates corresponding to pixel coordinates.
> > > > +**************************************************************************/
> > > > +void base_get_map_xy(int canvas_x, int canvas_y,
> > > > +                  int *map_x, int *map_y,
> > > > +                  int map_view_x0, int map_view_y0)
> > >
> > > Since base_get_map_xy isn't a general name IMHO what about
> > > base_canvas_pos_to_map_pos and base_map_pos_to_canvas_pos?
> >
> > In that case, how about just canvas_pos_to_map_pos and
> > map_pos_to_canvas_pos?
> 
> I thought the non-base_ version is for the gui-*/* files.

Yeah, but they're consistently named get_canvas_xy and get_map_xy.  I
thought we could leave those names intact and just give the "correct"
naming to the backend GUI function.

> > > > +void base_center_tile_mapcanvas(int map_x, int map_y,
> > > > +                             int *map_view_x0, int *map_view_y0,
> > > > +                             int map_view_width, int map_view_height)
> > > > +{
> > > > +  if (is_isometric) {
> > >
> > > > +    map_x -= map_view_width/2;
> > > > +    map_y += map_view_width/2;
> > > > +    map_x -= map_view_height/2;
> > > > +    map_y -= map_view_height/2;
> > >
> > > This looks weird.
> >
> > Yeah.  Actually, there are a lot of things that could be cleaned up, but
> > to make it easier to judge correctness I've left the code intact as much
> > as possible.
> >
> > Another example of a cleanup would be changing the very complicated math
> > in base_get_canvas_xy, isometric view to use the very simple
> > flat-coordinates-to-isometric-coordinates conversion.  The same should
> > be done in city_get_canvas_xy (as part of a unification of that
> > function).
> >
> 
> > Should I make these cleanups in this patch?
> 
> No. Please make two patches. Or create the second if the first is in
> the CVS.

OK.  The first patch will remain relatively unchanged, then (except for
the names, as above).  A new patch will follow.

jason