[freeciv-ai] Re: simple historian agent wants to get into cvs

[Raimar Falke <i-freeciv-lists@xxxxxxxxxxxxx>]

On Mon, Mar 29, 2004 at 02:07:10PM -0800, nikodimka wrote:
> 
> Hello,
> 
> I have fixed the Simple Historian Agent initial implementation
> acording to Raimar's comments and would like to present the patch.
> 
> What should I do to get it into the CVS?

> diff -bwruN -X./freeciv-cvs-Feb-27/diff_ignore 
> ./freeciv-cvs-Feb-27/client/agents/sha.c
> ./freeciv-cvs-Feb-27.sha/client/agents/sha.c
> --- ./freeciv-cvs-Feb-27/client/agents/sha.c  Wed Dec 31 17:00:00 1969
> +++ ./freeciv-cvs-Feb-27.sha/client/agents/sha.c      Tue Mar 30 01:50:09 2004
> @@ -0,0 +1,142 @@
> +/********************************************************************** 

> + Freeciv - Copyright (C) 2001 - A. Gorshenev (aka nikodimka)

The year. The aka isn't necessary ;)

> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 2, or (at your option)
> +   any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +***********************************************************************/

> +#include "agents.h"
> +#include "log.h"
> +#include "map.h"
> +#include "sha.h"
> +#include "support.h"

There is an include order defined.

> +/**************************************************************************
> +This is the simple historian agent.
> +It just saves the last states of all cities, tiles and units.
> +The trick is just to call this agent the last of all
> +so it still keeps old values whereas all other agents 
> +allready got the new ones.
> +**************************************************************************/
> +
> +static struct agent simple_historian_agent;
> +
> +static struct tile *previous_tiles = NULL;
> +static struct unit_list previous_units;

> +static void sha_tile_update(int x, int y);
> +
> +static void sha_unit_new(int id);
> +static void sha_unit_change(int id);
> +static void sha_unit_remove(int id);

If you move simple_historian_init after these functions you don't have
to declare them here.

There is commented code and comments about cities but no real
support?!

> +  freelog(LOG_DEBUG, "sha got tile: %d ~= (%d, %d)\n", index, x, y);

freelog calls shouldn't have a trailing "\n".

> +  struct unit * punit, *pold_unit;

> +  pold_unit = create_unit_virtual(get_player(punit->owner), 
> +                               NULL,
> +                               0,
> +                               0);

I'm pretty sure that these doesn't follow the style.

> +static void sha_unit_remove(int id) {
> +  struct unit *pold_unit;
> +
> +  freelog(LOG_DEBUG, "sha got unit: %d\n", id);
> +
> +  pold_unit = unit_list_find(&previous_units, id);

It is encouraged to perform the initial setting with the declaration:

  struct unit *pold_unit = unit_list_find(&previous_units, id);

> +  if (known_changed || tile_changed) {
> +    /* here I assume that tile can *change* only if it was known

> +       and still is known. Otherwise it new or removed 

it _is_

> +       and that it can not go into UNKNOWN after it has been known */
> +    if (known_changed && (ptile->known == TILE_KNOWN)) {
> +      agents_tile_new(packet->x, packet->y);
> +    } else if (known_changed && (ptile->known == TILE_KNOWN_FOGGED)) {
> +      agents_tile_remove(packet->x, packet->y);
> +    } else {
> +      agents_tile_changed(packet->x, packet->y);
> +    }
>    }

> +typedef union {
> +  int null_arg;
> +  int int_arg;
> +  struct xy_arg {
> +    int x;
> +    int y;
> +  } xy_arg;
> +} arguments;

The use of typedefs in freeciv isn't encouraged.

Is the null_arg needed?


>  {
>    int i;
> -
>    freelog(LOG_DEBUG,
>         "A: agents_unit_changed(unit=%d) type=%s pos=(%d,%d) owner=%s",
>         punit->id, unit_types[punit->type].name, punit->x, punit->y,

Noise.

> +/***********************************************************************
> + Called from client/packhand.c. See agents_unit_changed for a generic
> + documentation.

> + Tiles not removed, tiles lost in fog of war.

"Tile got removed because of FOW." But I'm not a native speaker
myself.

> +      /* encoding tile coordinates into single integer */

Wrong comment.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "- Amiga Y2K fixes (a bit late, wouldn't you say?)"
    -- Linus Torvalds about linux 2.4.0 at 4 Jan 2001