[freeciv-ai] Re: simple historian agent
[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
On Wed, Mar 17, 2004 at 02:53:22PM -0800, nikodimka wrote:
>
> Raimar,
>
> could you please review the simple historian agent initial implementation.
> It is a simple agent which remembers the last state of all the units, cities,
> tiles.
>
> Please see attachment for the details.
Lets see.
> /**********************************************************************
> Freeciv - Copyright (C) 2001 - A. Gorshenev (aka nikodimka)
> 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.
> ***********************************************************************/
>
You need to insert a guard here against multiple includes:
#ifndef FOOBAR
#define FOOBAR
> #include <stdlib.h>
Why is that needed?
> #include "agents.h"
Why is that needed?
> /**************************************************************************
> 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.
> **************************************************************************/
If you put this into the .c file it is enough.
> extern struct agent simple_historian_agent;
Why is that needed?
> void simple_historian_init(void);
> void sha_tile_update(int index);
>
> void sha_unit_new(int id);
> void sha_unit_change(int id);
> void sha_unit_remove(int id);
IMHO it isn't needed to have these functions in the header file.
> struct tile* sha_tile_recall(int x, int y);
> struct unit* sha_unit_recall(int id);
> /**************************************************************************
> 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.
> **************************************************************************/
> struct agent simple_historian_agent;
static.
> static struct tile *previous_tiles = NULL;
> static struct unit_list previous_units;
>
> /**************************************************************************
> ...
> **************************************************************************/
> void
> simple_historian_init(void)
Put it one one line.
> {
>
> previous_tiles = malloc(MAX_MAP_INDEX*sizeof(struct tile));
> memset(previous_tiles, 0, MAX_MAP_INDEX*sizeof(struct tile));
Instead of
struct foobar *p=malloc(sizeof(struct foobar));
use
struct foobar *p=malloc(sizeof(*p));
It is safer if you change the type of p.
Also don't use malloc. Use fc_malloc.
> unit_list_init(&previous_units);
>
> memset(&simple_historian_agent, 0, sizeof(simple_historian_agent));
> strcpy(simple_historian_agent.name, "Simple Historian");
strlcpy
> simple_historian_agent.level = 98; /* well which level this should
> be to recieve change
> notification after all others? */
Good question.
> simple_historian_agent.unit_callbacks[CB_REMOVE] = sha_unit_remove;
> simple_historian_agent.unit_callbacks[CB_CHANGE] = sha_unit_change;
> simple_historian_agent.unit_callbacks[CB_NEW] = sha_unit_new;
> /*
> simple_historian_agent.city_callbacks[CB_REMOVE] = sha_city_remove;
> simple_historian_agent.city_callbacks[CB_CHANGE] = sha_city_change;
> simple_historian_agent.city_callbacks[CB_NEW] = sha_city_new;
> */
> simple_historian_agent.tile_callbacks[CB_REMOVE] = sha_tile_update;
> simple_historian_agent.tile_callbacks[CB_CHANGE] = sha_tile_update;
> simple_historian_agent.tile_callbacks[CB_NEW] = sha_tile_update;
> register_agent(&simple_historian_agent);
> }
>
> /**************************************************************************
> ...
> **************************************************************************/
> void
> sha_tile_update(int index)
All of these functions should be static.
> {
> int x; int y;
>
> freelog(LOG_DEBUG, "sha got tile: %d\n", index);
>
> index_to_map_pos(&x, &y, index);
> /* Yes. decode and encode again. I could not find a function :( */
> previous_tiles[index] = *map_get_tile(x, y);
> }
IMHO the agents.c should do the callback with x and y and not with
index.
>
> /**************************************************************************
> ...
> **************************************************************************/
> void
> sha_unit_change(int id)
> {
> struct unit * punit, *pold_unit;
>
> freelog(LOG_DEBUG, "sha got unit: %d\n", id);
>
> punit = find_unit_by_id(id);
> pold_unit = unit_list_find(&previous_units, id);
> if (pold_unit) {
> *pold_unit = *punit;
> } else {
> freelog(LOG_DEBUG, "sha: Something wrong. CHANGEing unit I know
> nothing about: %d\n", id);
I would insert an assert here to catch these cases.
> pold_unit = create_unit_virtual(get_player(punit->owner),
> NULL,
> 0,
> 0);
> *pold_unit = *punit;
> unit_list_insert(&previous_units, pold_unit);
> }
> }
>
> /**************************************************************************
> ...
> **************************************************************************/
> void
> sha_unit_new(int id)
> {
> struct unit * punit, *pold_unit;
>
> freelog(LOG_DEBUG, "sha got unit: %d\n", id);
>
> punit = find_unit_by_id(id);
> pold_unit = create_unit_virtual(get_player(punit->owner),
> NULL,
> 0,
> 0);
> *pold_unit = *punit;
> unit_list_insert(&previous_units, pold_unit);
> }
>
> /**************************************************************************
> ...
> **************************************************************************/
> 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);
> if (pold_unit) {
> unit_list_unlink(&previous_units, pold_unit);
> } else {
> freelog(LOG_DEBUG, "sha: Something wrong. REMOVEing unit I know nothing
> about: %d\n", id);
Same here.
> }
> }
>
> /**************************************************************************
> Public interface
> **************************************************************************/
>
> /**************************************************************************
> ...
> **************************************************************************/
> struct tile*
> sha_tile_recall(int x, int y)
Make the return type const:
const struct tile *sha_tile_recall(int x, int y)
so that no other code changes the struct fields.
> {
> int index = map_pos_to_index(x, y);
> return &previous_tiles[index];
> }
>
> /**************************************************************************
> ...
> **************************************************************************/
> struct unit*
> sha_unit_recall(int id)
Same here.
> {
> return unit_list_find(&previous_units, id);
> }
Raimar
--
email: rf13@xxxxxxxxxxxxxxxxx
"Windows is the one true OS. MS invented the GUI. MS invented
the 32 bit OS. MS is open and standard. MS loves you. We have
always been at war with Oceana."
|
|
