Re: [Freeciv-Dev] Races ruleset - patch

[David Pfitzner <dwp@xxxxxxxxxxxxxx>]

Jerzy Klek wrote:

> This my approach to have races ruleset file which specifies
> race names, leader name, some AI hints and city names.
> The patch is against CVS from 21 August. It actually benefits
> a bit from government rulesets (init_race_goals() function).
> This is my first patch so please excuse any problems...

Cool, thanks.  Finally I take time to write some comments :-)

(These comments are actually based on a slightly different
patch sent later; please send latest patch to freeciv-dev
and/or upload to incoming.)

> Description:
> 
> The races.ruleset file contains what is needed to fill struct
> player race, default race leader names and cities. It is
> almost a copy of what wasinitialized in city.c and player.c.
> The rest is like any other rulesets: load and send to the client.
> I also had to update a bit selecting races, because rulesets
> were sent after that, so some information (races names and leaders)
> have to be sent in 'select race' request.
> There can still be only 14 races, but you can replace the existing 
> ones with whatever you like.

As previously commently, this restriction should be removed
in near future, but is ok for now.  When it is removed, I
would suggest that the number of _players_ stays at 14, but
the number of nations to choose from can be bigger.  

(If we do want to increase the number of players, which I 
personally think we don't need to, but some might, that 
should also be done separately.)

Just a note on one complication when increasing the number
of nations to choose from: scenarios providing starting
positions currently provide 14 starts.  I think probably 
the first 14 nations start from their correct starts, and 
if higher values are used, they select randomly from the 
remaining starts.  Well, this could be done better but the 
above is a minimal change which would work.

> I have changed the russian city names, now there are only cities
> from Russia, no cities from former Soviet Union, that are now 
> in independent states like Ukraine.

Well yes, we should probably work out whether Russia is Russia
or USSR and be consistent :-/  But after the nations ruleset 
machinery is in place would be appropriate to do this.

> Short description of what needs more work:
> 
> 1. It does not send flags

As discussed privately, the way to do this is via tilespec
tag strings; tilespec is now in CVS freeciv.

If the client doesn't have the specified tag (prefered or
alternate), the client has the usual options: die; show nothing; 
show something generic.  The best (but more complicated) option 
would be for the client to simulate a single-color flag with the 
default background color for that player (in which case tilespec 
should include information on a default flag size within the 
tilesize).

> 4. I provide only one default races.ruleset file, but if this thing
> is accepted, I can produce one for Civ1 and also a couple of new
> races to choose.

Since races/nations is really game "flavour", and not game rules, 
I think just one file is appropriate, for the basic game.   Once
we allow multiple races I expect lots of people will want to add
their own of favourite country, and having just one file will be 
easier.  (Modpacks which change lots of things may of course 
provide something different.)

Other comments:

In player.c: init_race_goals(), you changed some LOG_VERBOSE
to LOG_FATAL and exit(1).  The reason for the LOG_VERBOSE was
to allow the same data to be relatively independent of other
rulesets.  Now that races[] itself is from ruleset file, one 
could argue that this can be changed, and the user should use
a consistent set of ruleset files.  But I disagree: as above,
it makes sense and is convenient to have just one races file
which works across default/civ1/civ2 rulesets, and since this
is just AI/default goals, and not game rules, its quite ok if 
a few don't match the rulesets.  So I would leave this as before.

In the ruleset file you have:

+tech0="The Republic"
+tech1="The Wheel"
...
+tech9=""

I would have this as a variable length vector in the file:

tech_goals = "The Republic", "The Wheel", ...

Internally it may be easiest to keep a fixed limit, and complain
if the ruleset exceeds it, but its better to not impose restrictions
as part of the file format.  Then the internals could be made more
flexible without changing the files.

Also, as above I would prefer a single nations.ruleset for 
civ1/civ2/default, in which case should povide alternative 
strings when the tech names differs (eg Theology/Religion), 
as currently in player.c races[].

Regarding city names: 

- It shouldn't be necessary to send city names for nations
other than the one the player is playing?

- Its not even really necesary to send those: I have a patch
(will send separately) where the names are only kept on the
server and the client queries the server for names to suggest.
This would mean less to send at the start, and maybe simpler.

- For completeness and ease of adding/changing, maybe the 
misc_city_names[] should also be in the ruleset files?

Otherwise, looks good :-)

Regards,
-- David