[Freeciv-Dev] Re: different iso drawing algorithms

[Raimar Falke <rf13@xxxxxxxxxxxxxxxxx>]

On Fri, Dec 13, 2002 at 03:02:23PM +0100, Rafa³ Bursig wrote:
> Dnia 2002.12.13 10:35 Raimar Falke napisa³(a):
> >On Fri, Dec 13, 2002 at 02:28:14AM -0500, Jason Short wrote:
> >> On Thu, 2002-12-12 at 15:14, Raimar Falke wrote:
> >>
> >> > I agree that the code has to to more work for the iso case. But
> >this
> >> > be a factor around 3-5 and not 30. So did anybody profiled it? I
> >> > did. Data attached. As usual profiling distort the timing results.
> >But
> >> > is clearly visible that:
> >> >   - get_drawn is called almost the double amount in iso
> >> >   - get_drawn* create a lot of normalize_map_pos calls (Jason are
> >they
> >> >   really necessary?)
> >> >   - fill_tile_sprite_array* are called by the same amount
> >> >   - the amount of calls of pixmap_put_tile_iso and pixmap_put_tile
> >are also
> >> >   equal. Also the execution times are roughly equal.
> >>
> 
> All draw loop come from no-iso code and basicalyu look like :
> for( x=...; x < ... +  ... ; x++)
> this wotk preaty good to no-iso maps but with iso maps each tile must 
> be checked that is correct.

What is your point here? What must be checked?

> How many memory you have ?

We are here speaking about 3 pixmaps each about 800*600*3 bytes. These
are 4MB total. This isn't a problem _if_ it make the drawing faster.

> >> On my computer, dividing a signed integer by 2 takes 8 instructions.
> >
> >gcc-2.96, gcc-3.2:
> >
> >        movl    %ebx, %edx
> >        shrl    $31, %edx
> >        addl    %edx, %ebx
> >        sarl    %ebx
> >
> >Intel 6.0, 7.0:
> >
> >        addl      $-2147483648, %esi                            #3.9
> >        adcl      $-2147483648, %esi                            #3.9
> >        sarl      $1, %esi                                      #3.9
> >
> >You have an odd compiler.
> >
> >So dividing by 2 is a non-issue.
> >
> But still must be done

It is noise in the ocean of the other code.

> >> > > Before I start...
> >> > > Flush funtctions need pixel cordinate not map cordinate.
> >> > > Better we "save" pixel cordinate than "save" map cordinate and
> >> > > recalculate them with flush.
> >> >
> >> > The flush function doesn't take any parameter.
> >>
> >> We have the possibility for several different layers of flushing.
> >You
> >> guys keep saying "we need xxx", but you should also say *where* this
> >> code should go: in GUI or common.
> >
> >As much as possible common. Maintainability.
> >
> Rather in GUI becouse diferent client makes it in different way.

Why should clients make it in a different way?

> >> The GUI code should provide at least one function:
> >>
> >>   flush_map_canvas(canvas_x, canvas_y, pixel_width, pixel_height);
> >>
> and flush_map_canvas( 0, 0, 0, 0 );
> should fresh entire screen.
> 
> >> it may be possible to increase efficiency for some clients by
> >> providing
> >> flush_map_canvas_visible() or flush_rects() (but I would like to
> >> have this explained before providing it).  These should all work
> >> on canvas positions.
> those funcions should fresh  array/list redrawed rect.
> 
> draw_one(...);/* draw to buffer */
> save_rect(...);(or mapcanvas_dirty_rectangle(...) )
> 
> draw_one(...);/* draw to buffer */
> save_rect(...);(or mapcanvas_dirty_rectangle(...) )
> 
> draw_one(...);/* draw to buffer */
> save_rect(...);(or mapcanvas_dirty_rectangle(...) )
> 
> /* update screen */
> fresh_rects();(or flush_map_canvas_visible() )
> 
> >>
> >> The common code may wish to track the dirty-ness of individual
> >> tiles. That way instead of calling
> >> update_map_canvas(x, y, 1, 1, FALSE) we just
> >> mark position (x, y) for being redrawn later.  Then when another
> >> function is called we redraw this function and all others.  To make
> >> things clear we probably shouldn't call this "flushing" but should
> >> come up with some other name.
> >
> >I disagree. There should only be one flush function which flushes all
> >objects which are dirtied before. No flush_map_canvas(canvas_x,
> >canvas_y, pixel_width, pixel_height). The flush function can calculate
> >all this by itself.
> >
> Yes and No
> YES , we need only one flush system and it should work on pixel 
> (canvas) cordinates but this system should have 3 functions :
> 
> 1) flush_map_canvas( x, y, w, h ); ( special case to flush_map_canvas( 
> 0, 0, 0, 0 ) it should make flush entire screen and clean any "saved" 
> rect. in list/array )
> 
> 2) save_map_canvas_rect( x, y, w, h );( or mark_rect_dirty(...) ) - 
> store redrawed(dirty) rect.
> 3) refresh_rects(void) ( or mapcanvas_dirty_flush() ) - flush all 
> stored ( dirty/redrawed ) rectangles.
> 
> 2) and 3) should have mehanism that allow flush fullscreen if numbers 
> of redrawed rect. are high.
> This mehanism should be used in functions like mapcanvas_dirty_all()
> 
> NO, We should draw this tile ( not only mark ) with remembering ( save 
> ) draw area and flush should be make with all redrawed tiles in one 
> call flush function.
> 
> ...
> draw_tile( ... );
> if ( write_to_srreen )
>   flush_map_canvas( x, y, w, h );/* flush at once */
> else
>   save_map_canvas_rect( x, y, w, h ); /* save rect. and flush with 
> others */
> ...

Why do you insisted on a flush function which take a rectangle? It is
unneeded. We can built the whole system on these two functions:

  void mapcanvas_dirty_canvas_rectangle(canvas_rectangle);
  void mapcanvas_dirty_flush(void);

Some wrapper functions for dirting map positions may be nice and
required. So the users (packhand.c, control.c, ...) only have to dirty
certain parts. Than million cycles later we found out that we got all
data from the network. At this point we redraw all the regions we have
dirtied before (call mapcanvas_dirty_flush). mapcanvas_dirty_flush
can't and won't take a region of a canvas. This also means that no
function will take a write_to_screen parameter anymore. The flow is
like this:

  packhand.c:handle_tile_info calls
  mapview_common.c: map_dirty_tile (this function can't calculate the
                    canvas position since the map will be moved later,
                    so it will save the map position in some list)

later
 packhand.c:handle_freeze_hint calls
 mapview_common.c:mapcanvas_dirty_flush which goes through the dirty
                  lists (one for map positions and one for canvas
                  rectangles) and creates a unique list of map tiles
                  which need updating. Than it calls draw_tile
                  (something like pixmap_put_tile or put_one_tile) for
                  the dirty tiles. At the end it calls
                  mapcanvas_update_screen (which is something like
      get_canvas_xy(x, y, &canvas_x, &canvas_y);
      gdk_draw_pixmap(map_canvas->window, civ_gc, map_canvas_store,
                      canvas_x, canvas_y,
                      canvas_x, canvas_y,
                      width*NORMAL_TILE_WIDTH,
                      height*NORMAL_TILE_HEIGHT);

For better efficiency mapcanvas_dirty_flush should maybe operate on
tiles rectangles and not single tiles. This allows certain speed ups
in the draw_tiles functions.

Mhh thinking about this also leads to the conclusion that you _can't_
pass canvas coordinates to the dirty functions since the canvas may be
recentered. Only the map positions stay invariant/fixed. So the

  void mapcanvas_dirty_canvas_rectangle(canvas_rectangle);

from above needs to be changed to:

  void mapcanvas_dirty_tiles(int x, int y, int width_in_tiles, int 
height_in_tiles);

> >> One implementation question is at what point it is faster to discard
> >> the list of rects that need flushing and when we should just flush 
> >the
> >> whole mapcanvas.  One such choice would be:
> >
> >Yes that is in interesting question.
> >
> void update_map_canvas_visible(void) should flush entire screen.
> 
> read above about flush entire screen mehanism in 2) and 3).
> 
> >>
> >>   static int x0 = -1, x1 = -1, y0 = -1, y1 = -1;
> >>
> >>   void mark_rect_dirty(int canvas_x, int canvas_y,
> >>                        int width, int height)
> >>   {
> >>     if (x0 == -1) {
> >>       x0 = canvas_x;
> >>       x1 = canvas_x + width;
> >>       y0 = canvas_y;
> >>       y1 = canvas_y + height;
> >>     } else {
> >>       x0 = MIN(x0, canvas_x);
> >>       x1 = MAX(x1, canvas_x + width);
> >>       y0 = MIN(y0, canvas_y);
> >>       y1 = MAX(y1, canvas_y);
> >>     }
> >>   }
> >>
> >> thus we keep no "list" of rects that are dirty.  An alternative is
> >> to keep a full list.  This is a tradeoff; both have cases where 
> >they do
> >> a lot of unnecessary redrawing.
> >>
> Each client should have own implementation of those functions. ( 1), 
> 2), 3) )

I'm very sure that won't be the case. The implementation of
mapcanvas_dirty_canvas_rectangle is GUI independent. So is a certain
part (managing the list if dirtied rectangles) of
mapcanvas_dirty_flush.

> >> > > >> > void mapcanvas_dirty_rectangle(map_x,map_y,width,height);
> >> > > >> if this function work like  "save_rect( int x , int y , int w
> >, int
> >> > > >> h )" then map_x , map_y should be pixel tile positon on
> >screen.
> >> > > >> ( canvas_x , canvas_y )
> >> > > >
> >> > > >Since mapcanvas_dirty_tile works on map positions and not
> >canvas
> >> > > >positions mapcanvas_dirty_rectangle should also be. However we
> >can
> >> > > >also add a
> >> > >
> >> > > >   void
> >> > > 
> >>mapcanvas_dirty_canvas_rectangle(canvas_x,canvas_y,pixel_width,pixel_height);
> >> > > >
> >> > > >Note that I don't know how often these functions will used. It
> >is
> >> > > >quite possible that one function isn't needed at all.
> >> > > >
> >> > >
> >> > > That all depend where you want call those functuions.
> >> > > exp.
> >> > >
> >> > > int draw_tile( int map_x , int map_y )
> >> > > {
> >> > >   put_one_tile( map_x , map_y ); /* here you calculate canvas_x
> >,
> >> > > canvas_y */
> >> > >   mapcanvas_dirty_tile(map_x,map_y);/* here you make secound
> >> > > calculation canvas_x , canvas_y or make it with flush*/
> >> > > }
> >> > >
> >> > > better is
> >> > >
> >> > > void put_one_tile( map_x , map_y );
> >> > > {
> >> > >   int canvas_x , canvas_y;
> >> > >   ...
> >> > >     mapcanvas_dirty_rectangle(canvas_x,canvas_y,
> >> > >          NORMAL_TILE_WIDTH,NORMAL_TILE_HEIGHT);
> >> > > }
> >> >
> >> > > IMO we need both :)
> >>
> >> IMO put_one_tile should be passed in canvas_x, canvas_y.  But it is
> >not
> >> this function that should mark the rect as dirty - it is
> >> update_map_canvas.
> >>
> one question: where in iso code you call update_map_canvas in other 
> case than draw one tile and redraw whole screen. ( I know of city map )
> 
> update_map_canvas is no-iso legacy.
> We should have dedicated functions to do this no one universal.
> exp.
> 
> #define Inc_Col( Col )                        \
> do {                                  \
>   if (++Col >= map.xsize) {           \
>     Col = 0;                          \
>   }                                   \
> } while(0)
> 
> #define Inc_Row( Row )                \
> do {                          \
>     if ( ++Row >= map.ysize ) \
>               Row = 0;        \
> } while(0)
> 
> #define Dec_Col( Col )                \
> do {                          \
>      if ( --Col < 0 )         \
>       Col = map.xsize - 1;    \
> } while(0)
> 
> #define Dec_Row( Row )                \
> do {                          \
>     if ( --Row < 0 )          \
>       Row = map.ysize - 1;    \
> } while(0)
> 
> static void upate_one_tile( int x , int y , bool write_to_screen )
> {
>   int map_view_x0, map_view_y0 , canvas_x , canvas_y ;
>   int map_win_width, map_win_height;
>        
>   get_mapview_dimensions(&map_view_x0, &map_view_y0,
>                        &map_win_width, &map_win_height);
> 
>   if ( !map_pos_to_canvas_pos( x, y , &canvas_x, &canvas_y,
>                              map_view_x0, map_view_y0,
>                              map_win_width, map_win_height) )
>  {
>       return;  }
>   Dec_Col( x );
>   put_one_tile_iso( x , y, canvas_x - NORMAL_TILE_WIDTH/2,
>                           canvas_y - NORMAL_TILE_HEIGHT/2, D_B_R );
>   Dec_Row( y );
>  Inc_Col( x );
>   put_one_tile_iso( x , y, canvas_x + NORMAL_TILE_WIDTH/2,
>                           canvas_y - NORMAL_TILE_HEIGHT/2, D_B_L );
>   Inc_Row( y );
>   put_one_tile_iso( x, y, canvas_x, canvas_y, D_FULL );
>   Inc_Row( y );
>   put_one_tile_iso( x , y, canvas_x - NORMAL_TILE_WIDTH/2,
>                           canvas_y + NORMAL_TILE_HEIGHT/2, D_M_R );
>   Dec_Row( y );
>  Inc_Col( x );
>   put_one_tile_iso( x , y, canvas_x + NORMAL_TILE_WIDTH/2,
>                           canvas_y + NORMAL_TILE_HEIGHT/2, D_M_L );
>   if (write_to_screen)
>  {
>    flush_mapcanvas( canvas_x , canvas_y - NORMAL_TILE_HEIGHT/2 ,
>        NORMAL_TILE_WIDTH , NORMAL_TILE_HEIGHT + NORMAL_TILE_HEIGHT/2 
> );  }
> /* else
> {
>    save_map_canvas_rect( canvas_x, canvas_y - NORMAL_TILE_HEIGHT/2,
>        NORMAL_TILE_WIDTH , NORMAL_TILE_HEIGHT + NORMAL_TILE_HEIGHT/2 
> );
> }
> */ }

I don't understand this. Maybe I don't want to understand it.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "From what I am reading Win98 and NT5.0 will be getting rid of all that
  crap anyway. Seems that Microsoft has invented something called TCP/IP and
  another really revolutionary concept called DNS that eliminates the
  netbios crap too. All that arping from browsers is going to go away.
  I also hear rumors that they are on the verge of breakthrough discoveries
  called NFS, and LPD too. Given enough time and money, they might
  eventually invent Unix."
    -- George Bonser in linux-kernel