[Freeciv-Dev] Re: Safe paths for triremes etc. (PR#1007)

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

--- Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx> wrote:
>  --- Raahul Kumar <raahul_da_man@xxxxxxxxx> wrote: 
> 
> Thanks a lot for bothering to look at my patch.
> 
> First, I would like to announce that all trouble bit were inherited by my
> patch from previous code.  Well, many of trouble bits...
> 
> [..]
> 
> > number 3. Usage of your goto results has improved readability. You
> > should
> > probably break your patch into 4 smaller sub patches
> 
> I will do it, maybe in a slightly different way, but I will.
> 
> [..]
> 
> > List of Scary Code
> > 
> > find_the_shortest_path
> > Some of the corrected functions.
> 
> sorry
> All I did is to cut-n-paste relevant pieces of find_the_shortest_path
> into these corrected cost functions.  I shall add some comments.  Now I
> know why each component is there, although I am not 100% sure that all
> the needed components are there yet.  I added some checks for FoW
> (is_known_and_seen) but that's it.
> 

What components are not there yet?

> >  c = map_get_tile(punit->x, punit->y)->move_cost[k];
> 
> oops
> that's cut-n-paste for sure.
> ah, actually I didn't touch this part at all (apart from fixing the
> majik)
> I will probably fix it as a first patch, together with other majik
> things.
> 

Good.

> > -  if (unit_profits_of_watchtower(punit)
> > -      && map_get_tile(punit->x, punit->y)->special & S_FORTRESS)
> > -    range =get_watchtower_vision(punit);
> > 
> > You removed all the code that checks for increased unit vision range.
> > Your bad or part of an evil masterplan?
> 
> both :0
> actually the use of range in this function is to estimate how far the
> unit _will_ see when it come to some location (x,y).  So the presence of
> a watchtower at the initial location should not affect this variable.  At
> most we can recalculate range for each final location (x,y) but it is
> costly and sometimes the tile (x,y) is still unknown...
> 

I agree.

> > /* maxcost is maximum distnace of search * SINLGE_MOVE */
> > 
> > int maxcost = pplayer->ai.control ? 2 * THRESHOLD : 3 * SINGLE_MOVE;
> > 
> > I see no reason to care if the player is an ai player or not. Obviously
> > you do, go ahead and explain your reasoning.
> 
> actually this is another "inherited" bit.  I just changed it a little :)
> I think if the human player sees a hut and wants to collect it, he should
> send the explorer directly to it.  In some cases you do not want to try
> the hut for the fear of unleashing barbarians too close to your
> undefended cities.
> 

The fact that explorers do not automatically uncover huts is one of the
reasons I do not like auto-explore mode.

> >       
> > /* init_warmap(punit->x, punit->y, LAND_MOVING); */
> > 
> > I am pretty sure that init_warmap does not actually do anything on x86
> > boxes.
> > Does anyone know any reason to leave it in?
> 
> this particular init_warmap doesn't do anything, it seems.
> I asked the same question, nobody answered...
> 

Well, let's shout it out. Raimar, Thue, anybody at all, does init warmap
have a purpose?

> > return -SINGLE_MOVE
> > 
> > No, no,no. Bad Berkolaiko, Bad! Do not return a negative single move
> > cost, this
> > is part of the reason I used to believe there was a relation between
> > single
> > move cost and the sea move cost.
> > 
> > return SEA_MOVE_COST would be better.
> 
> well, this is similar to the use of SEA_MOVE_COST but in actual fact it
> is completely opposite.  returning negative movecost is to signal that
> routes must not pass through the tile and, at the same time, provide the
> movecost
> for _ending_ the route here.
>

The above should be a comment, and it is still a bad thing to return
-SINGLE_MOVE. Call it something like

EVIL_TWIN_OF_SEA_MOVE_COST

Of course, if you can think of a funnier variable name ...

 
> > is_coast_seen2 What does this marvellous function do that is different
> > enough
> > from is_coast_seen to justify its existence? It should be possible to
> > roll
> > these two functions into one.
> 
> it answers the question: can player be sure (from what he sees) that the
> given location is at most distance 2 from the coast?
> is_coast_seen answers the same question but for distance 1.
> if you can suggest a way to do it in a more elegant way, I will be very
> grateful.
>

Your wish is my command.

int is_coast_seen(int x, int y, int dist, struct player *pplayer)
{
  square_iterate(x, y, dist, x1, y1) {
    enum tile_terrain_type ter = map_get_terrain(x1, y1);
    if (ter != T_OCEAN) {
      /* AI cheats */
      if (pplayer->ai.control) 
      return 1;
      square_iterate(x1, y1, 1, x2, y2) {
        if (map_get_known(x2, y2, pplayer)) {
          return 1;
        }
      } square_iterate_end; /* around x1,y1 */
    }
  } square_iterate_end; /*around x2,y2 */
  return 0;
}

Please, if you need to express your gratitude in more concrete ways, two words

Exotic dancers
 
> > igter_move_unit(int x, int y, int dir, int x1, int y1)
> > +{
> > +  if (map_get_terrain(x1, y1) == T_OCEAN) {
> > +    if (ground_unit_transporter_capacity(x1, y1,
> > unit_owner(warmap.warunit)) >
> > 0)
> > +      return SINGLE_MOVE;
> > +    else
> > +      return MAXCOST;
> > +  } else {
> > +    return (map_get_tile(x, y)->move_cost[dir] ? MOVE_COST_ROAD : 0);
> > +  } 
> > 
> > I believe this implies that warmap generation no longer believes the
> > move cost
> > for igter units is 3 instead of 1. I don't remember seeing this change.
> 
> I didn't see any reason for igter move to cost 3 and it is 1 (as I think
> it should be) in client/goto.c and nobody complains -- so I changed it.
>

Love this change. Finally, something new to Freeciv, logic ;).
 
> > What normal_move_city does ? No idea. Comment it out and watch what
> > happens. I
> > can think of a function that calculates how to attack a unit within 1-2
> > moves
> > of the city. Obvious use to the AI.
> 
> yes, something like this.
>

Regardless of what it does, it is very badly named. Junk it.
 
> the cost function normal_move_city is actually never used (yet).
> 
> > maxcost= THRESHOLD * 6 + 2
> > 
> > Why add 2? 
>

t is only logical to believe that the writer of the
patch knows what he is writing. Thue mentioned some of these issues before in
the email below. I was hoping for clarification.

On Wed, 08 Nov 2000 05:51:07 Raahul Kumar wrote:
> > > This patch updates the AI and movement code(it was patched against
> freeciv-cvs-Oct31, replacing a lot of those
> magic numbers
> based on 3 with constants defined below. This should make it easier to
> support
> more civ variants and make the AI code a little bit easier to follow. This
> was mostly based on Artur Biesiadowski's patch for 1.8.1.

A worthy cause.

> Will someone who understands the AI code please rename those variables? It
> is difficult to understand where those numbers come from in particular:
>
> -3 for sea movement cost appears to be a special number(Does it depend on
> the movement cost of 1 square or not?)

It is just a convention; a movecost of -3 means a boat can move between tiles
in one move.

> maxcost =  72 ( apparently this is computed as a result of threshold * 6)
> > How was this number derived?

This constant is arbitrary. It exists to limit the amount of calculations the
AI does.

> In general, whoever is currently maintaining the AI code

aheh. Who is that? :)

> needs to insert a lot of comments.

He truly does!

> It would be nice if the AI was moved away from relying on so many hardcoded
numbers.
> > Missing:
> > some funcs in server/gotohand.c
> common/map.c:tile_move_cost_ai
> some funcs in ai/advmilitary.c
> some funcs in ai/advunit.c

No way those in the patch plus these are all that need to be changed.

> I've added a few additonal  /* MOVEFIXME */ comments in places where I
> wasn't sure of
> meaning of code (3 or 4 places) - somebody should review those places
> closely.

I think I can do that.

> The plan is to to use 30 instead of 3 as a base - 1/3 for road/river, 1/10
> for rail. It would also allow C:CTP costs (1/2 for river, 1/3 for road,
> 1/5 for rail, 1/10 for maglev). It should also make it easier for SMAC and
> Civ III. There is some additional code currently commented out that allows
> for
> a fixed railroad movement - i.e if you stuck a howitzer or a mech infantry
> on a railroad they would both move the same distance.

Note that you will also have to convert the warmap from char to short to avoid
it overflowing. (or else it would only allow warmaps with routes of ~25 moves).
Some places the number 255 is used; this should be replaced with MAXCOST,
defined in server/gotohand.c currently.

> For now it is defined in unit.h 
> #define SINGLE_MOVE 3
> Also there are definitions for
> #define MOVE_COST_RIVER 1
> #define MOVE_COST_ROAD 1
> #define MOVE_COST_RAIL 0 
> > They are
> used only in once place, so it should be easy for grep for them and
> change (or even replace with
> #define MOVE_COST_RIVER game.river_movement_cost
> or anything).
>
> Somebody has to work hard on goto code - there is way too much places
> where magic numbers are used, which probably are dependent on 3. 

Worthy goal, as I said. But not close to finished. The entire code base will
have to be audited. I don't have the time to do that now, but I am willing to
apply the incomplete patch, as it makes the modified code more readable.
 
> you ask me?
> I got it from really_generate_warmap so my warmap would be compatible.
> 
> Thanks again for the comments.  I am sure that with your help we can
> improve the patch and eventually get it through.
> 
> Best,
> G.
> 
> 
> 
> ____________________________________________________________
> Do You Yahoo!?
> Get your free @yahoo.co.uk address at http://mail.yahoo.co.uk
> or your free @yahoo.ie address at http://mail.yahoo.ie


__________________________________________________
Do You Yahoo!?
Make a great connection at Yahoo! Personals.
http://personals.yahoo.com