[Freeciv-Dev] Re: PATCH: remove map_adjuxt_[xy] invocations

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Oct 09, 2001 at 01:48:34AM -0400, Ross W. Wetmore wrote:
> At 02:57 AM 01/10/05 -0400, Jason Dorje Short wrote:
> >"Ross W. Wetmore" wrote:
> >> 
> >> I would not clutter map_inx(), which was supposed to be changed to
> >> map_index(), with a lot of extra checks and stuff.
> >
> >OK, fine by me.
> >
> >Raimar suggested map_pos_to_index for this macro (with a corresponding
> >index_to_map_pos for the reverse).  How do you feel about this
> >nomenclature?
> >
> >> Make a map_index_checked() for this purpose, and put it where it
> >> is needed.
> >> 
> 
> I sometimes worry about absorbing too much German influence, but I
> actually like adding the "_checked" or "_unchecked" suffixes to 
> distinguish nearly identical macros/functions. I actually prefer to
> use "_unchecked" for the dangerous one, because most people are lazy
> enough in typing and if they find the short default, they don't do
> serious damage, just performance.

Ack. However I think that we don't need two forms. The one form
(currently map_inx) just needs a check.

> The map_pos_to_index/index_to_map_pos is one of Raimar's better 
> efforts. There is a niggling map_pos discussion that might need to be
> resolved before this gets total concensus in some quarters.

It turns out that index_to_map_pos isn't needed.

> >> Some minor nits below.
> >
> >No doubt you've seen my later patch that addressed some of this.  I'll
> >reply anyway...I just got this mail from you, but maybe it was sent some
> >time ago.  Who knows.
> >
> >> At 01:03 AM 01/10/03 -0400, Jason Dorje Short wrote:
> >
> ><snip all the redundancy with is_real_tile>
> >
> >Damn, I didn't think to check for that.  I'll review it all.
> >
> >> >     for(x=punit->x-2; x<punit->x+3; ++x) {
> >> >       unit_list_iterate(map_get_tile(x, y)->units, pu)
> >> >       if(unit_flag(pu, F_PARTIAL_INVIS)) {
> >> >-        refresh_tile_mapcanvas(map_adjust_x(pu->x), y, 1);
> >> >+        /* Why not just use (x, y) instead of (pu->x, pu->y)?  --JDS */
> >> >+        assert(is_normal_map_pos(pu->x, pu->y));
> >> >+        refresh_tile_mapcanvas(pu->x, pu->y, 1);
> >> >       }
> >> >       unit_list_iterate_end
> >> >     }
> >> >   }
> >
> >I don't think the assertion is "incorrect".  It is unneeded since the
> >unit should by definition have normal map positions - if a check is to
> >be done it should be in refresh_tile_mapcanvas.  Thus I removed it in
> >the later patch.
> 
> I think this is part of a larger square_iterate that should take care
> of much of this, i.e. checking map_get_tile(x,y). I suspect all
> the grunge inside the unit_list_iterate is to fix bugs with a bogus
> map location like pointing at a void_tile in the days when everyone 
> was confused and hacking. The map_adjust_x is totally bogus for the
> same reasons.
> 
> >> >
> >> >Index: server/mapgen.c
> [...]
> >> Your use of adjacent_iterate means the /10 is incorrect. You should
> >> probably accumulate the count of tiles actually collected.
> >
> >Correct.  The newer patch uses nearest_real_tile to achieve the
> >identical behavior to current code.  I do think current code is
> >incorrect, but the correctness of the cleanup patch is much easier to
> >verify if it leaves the behavior identical.
> 
> But the current code actually hardcoded the adjacent_iterate, no. So it
> did loop over 2*center + 8*adjacent points, even if some of them were
> bogus. You have changed the count by going to the adjacent_iterate.
> 
> >The biggest problem I have with current code is not that it does the
> >feedback thing (the smoothing is a pretty flexible process anyway), but
> >that the feedback is dependent on whole_map_iterate.  This violates the
> >idea behind whole_map_iterate's use IMO.  But, I'm not prepared to fix
> >it at this time.
> 
> Look at the new mapgen. The smooth_map and adjust_map algorithms are
> quite different, but they will show you a way to fix the whole_map
> problem. In fact, if you look at the "asteroid rain" sections you will
> find some comments and places where this effect is much more subtle
> (but it is really endemic :-).

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Only one human captain has ever survived battle with the Minbari
  fleet. He is behind me. You are in front of me. If you value your 
  lives, be somewhere else."
    -- Ambassador Delenn, "Severed Dreams," Babylon 5