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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

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}

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

> > > +/**************************************************************************
> > > +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.

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

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Python 2.0 beta 1 is now available [...]. There is a long list of new 
  features since Python 1.6, released earlier today. We don't plan on 
  any new releases in the next 24 hours."
    -- Jeremy Hylton at Slashdot