[freeciv-ai] Re: (PR#9610) AI movemap

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=9610 >

Per I. Mathisen wrote:
> <URL: http://rt.freeciv.org/Ticket/Display.html?id=9610 >
> 
> Here is the promised movemap structure. Despite the warnings in the code
> and the rampant use of pf, I did't measure more than 5% slowdown for
> autogames I tested. And that is when this is in addition to the old code
> and replaces nothing.
> 
> It is accessed using two iterators, movemap_iterate_one_turn(x, y, punit)
> and movemap_iterate_two_turn(x, y, punit). These gives you the units that
> can reach us within one turn and two turns, respectively. However, the two
> turn one does not also contain the one turn ones.
> 
> We honour the game.slow_invasions parameter, which is vital for
> correctly evaluating ferry dangers.
> 
> It handles ferries, although this part of the code is not very elegant. I
> tried to beat pf into submission, but gave up. As far as I could see,
> there was no way to do it properly.
> 
> TODO: More checks to ensure that we don't add eg submarines as overlapping
> sea units on land tiles.
> 
> Also, I two turn range is probably not enough. A third list might be
> necessary, which does not look at ferries, but only pure land and sea
> attackers, perhaps filtered to save CPU and get only relevant hits
> (eg horsemen and ironclads).

I have no real comments on the design.  But...

> +  movemap = fc_calloc(sizeof(struct movemap_type), map.xsize * map.ysize);

should be sizeof(*movemap)

and use MAX_MAP_INDEX.

> +/**************************************************************************
> +  Check where passengers onboard ferries can go.  turns is the number of
> +  turns the passenger can walk on land in a two turn horizon, ie either 
> +  one or two.
> +
> +  We have to check that we don't accidentially insert a unit into a list
> +  where it already exists. This is cumbersome but I see no other way.
> +**************************************************************************/
> +static void movemap_check_ferry(int x, int y, int id, int turns)
> +#define INSERT(list, unit) {                              \
> +  bool has_already = FALSE;                               \
> +  movemap_list_iterate(list, check) {                     \
> +    if (*check == unit->id) { has_already = TRUE; break; }\
> +  } movemap_list_iterate_end;                             \
> +  if (!has_already) {                                     \
> +    movemap_list_insert(&list, &unit->id);                \
> +  }                                                       \
> +}

IMO INSERT should just be a function.  Also you shouldn't break style, 
even inside macros.

> +      pfmap = pf_create_map(&parameter);
> +      pf_iterator(pfmap, pos) {
> +        if (pos.total_MC > moverate * 2) {
> +          /* This is too far */
> +          break;
> +        } else if (pos.total_MC <= moverate) {
> +          movemap_list_insert(&MOVEMAP(pos.x, pos.y).one_turn, &punit->id);
> +        } else {
> +          movemap_list_insert(&MOVEMAP(pos.x, pos.y).two_turn, &punit->id);
> +        }

I think you want to use pos.turn here.  Otherwise initial MP have a 
large effect.

> +  /* Now do ferries. This is ugly, but correct. I gave up on beating
> +   * pf into submission. It is also probably very very slow. */

Get Greg's advice.

> +  case AIR_MOVING:
> +  case HELI_MOVING:
> +    parameter->get_MC = single_airmove; /* very crude */
> +    break;    

There's already a patch for air PF.

jason