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

[jdorje@xxxxxxxxxxxxxxxxxxxxx]

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.

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

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

In any case, a new patch will follow.

jason