[Freeciv-Dev] Re: Remove map_adjust_[xy] invocations from server (PR#1003)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Oct 12, 2001 at 09:49:54PM -0400, Ross W. Wetmore wrote:
> At 05:18 AM 01/10/12 -0400, Jason Dorje Short wrote:
> >Raimar Falke wrote:
> >[snip: uses of is_real_tile, normalize_map_pos, etc.]
> >
> >> I plan to propose something like this:
> >>  - there is a CHECK_MAP_POS(x,y) == assert(is_normal_map_pos(x,y));
> >>  - every "assert(is_real_tile(x,y));normalize_map_pos(x,y)" is
> >>  replaced by CHECK_MAP_POS
> >>  - there is an extra check (if(!is_normal_map_pos(x,y)){y=x/0;})
> >>  introduced in map_inx
> 
> As far as I am aware, is_normal_map_pos() is not the same thing as
> is_real_tile(). It tests for normalized and not realness which are 
> different concepts that you really need to keep straight.

This is correct. These are different things. But in the long term we
want normalized map positions and the code IS ready to 99%. So we
should go IMHO the full way and just say that function arguements have
to be normalized and deal with rest (~1%) in the next weeks and be
done with it.

> Some of Jason's cases are just a check for normalized, and that is what
> his macro is designed to do. This does not mean Raimar should break the
> code everywhere by replacing the is_real_tile(), normalize_map_pos()
> constructs used elsewhere.
> 
> So if you add this macro do it with the right check, and document so
> carefully that idiots cannot misunderstand what it does.
> 
> If you need a second macro, then make it and document it in the same
> way.
>  
> [...]
> >1.  I do not see the logic of forcing a crash when not in debugging
> >mode.  Presumably people playing the game in this stage would appreciate
> >it if we did everything possible to avoid crashes.  For the "extra
> >check", why not have something like this:
> >
> >if(!is_normal_map_pos(x, y)) {
> >  assert(0);
> >  freelog(...);
> >  nearest_real_pos(&x, &y);
> >}
> 
> Please add BIG BAD COMMENTS to explain that this is a GROSS HACK to keep
> playing in the face of a SEVERE PROGRAMMING ERROR that is not yet fixed.
> 
> Note it may seriously damage gameplay, as any illegal game coordinate
> transformation is liable to do.
> 
> But I think you want to use normalize_map_pos() here to safely continue
> because if you were concerned about coordinates that might be unreal
> and hence need some gross hackery, then you should have tested with
> is_real_tile().
> 
> Seriously, are you checking for unreal or just unnormalized coordinates?

Both and there are two sets of error cases. On can be fixed with
calling normalize_map_pos and other can be band-aided with
nearest_real_pos. Both should assert in a normal development
version. We are just argueing what should happen in a release version.

> [...]
> >2.  None of this is a reason not to go ahead with this patch.  This
> >replaces one set of imperfect code with another set, but at least the
>           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ make this clear
> >new set uses the desired code constructs (i.e. normalize_map_pos instead
> >of map_adjust_x()).
> >
> >
> >Either way, I'm willing to come up with a patch that makes the checks we
> >decide on (although fixing the code to use them everywhere will take
> >more work).  Let me know what you want.
> >
> >jason
> 
> Actually, I think you got the right fix a few emails later. It used
> normalize_map_pos() in the test and nearest_real_pos() in the fail clause
> which is at least consistent.
> 
> But this should still be documenteed as a GROS HACK as above :-). 

Jason since do the patch you can also add a comment in
freeciv_hackers_guide.txt.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Like the ad says, at 300 dpi you can tell she's wearing a
  swimsuit. At 600 dpi you can tell it's wet. At 1200 dpi you
  can tell it's painted on. I suppose at 2400 dpi you can tell
  if the paint is giving her a rash."
    -- Joshua R. Poulson