Re: [Freeciv-Dev] New game.ruleset to support custom calendars, etc. (PR#45)

[David Pfitzner <dwp@xxxxxxxxxxxxxx>]

Bobby D. Bryant wrote:

> bdbryant@xxxxxxxxxxxxxxx wrote:
> 
> > This is a new file, game.ruleset.  ...
> 
> Jitterbug does not seem to allow me to append anything to the above (id = 
> 45), so

True, one can't send replies via the web interface as a guest
user, which is a bit annoying.  (I think Freeciv is using Jitterbug
in a slightly different way than the Jitterbug developer originally
intended, which makes some things about Jitterbug seem a bit unnatural.)

> the actual patch files are attached below.  Here's hoping someone can/will 
> attach
> them properly...

Too late: you already did the right thing! :-)

Sending email to the bug system with a bug identifier tag
in the subject (eg "(PR#45)") adds it to that bug report
as a followup.  Just replying to the report when it appears
on freeciv-dev (and keeping the CC to the bug system) will
achieve this nicely.

[ BTW, Please see http://www.freeciv.org/contribute.html about
the prefered way to do diffs.  In particular, its easiest if 
you provide a single diff file, either by concatenating
the individual diffs or using cvs or separate directories
to generate a single big diff in one go. ]

> +++ server/gamehand.c Mon Jul  5 08:33:59 1999
> @@ -42,7 +42,7 @@
>  extern int iRandJ, iRandK, iRandX; 
>  extern int rand_init;
>  
> -#define SAVEFILE_OPTIONS "1.7 startoptions unirandom spacerace2 rulesets"
> +#define SAVEFILE_OPTIONS "1.8.2 startoptions unirandom spacerace2 rulesets"

Changing 1.7 to 1.8.2 here is not really desirable; the point of 
options/capabilities is to _not_ tie things to specific version
numbers.  (And the actual and accurate version number is already 
in the file elsewhere if it is needed.)  The 1.7 is really a relic, 
though maybe a use could be found for it sometime.  In any case, 
I don't think it should be changed without some coherent reason 
for doing so.  Think of it as meaning: "this savefile format is the
same as the 1.7 series, with the following additions:".

> @@ -256,9 +256,15 @@
>    if (game.skill_level==0)
>      game.skill_level = GAME_OLD_DEFAULT_SKILL_LEVEL;
>    game.timeout       = secfile_lookup_int(file, "game.timeout");
> +  for( i=0; i<MAX_EPOCHS; i++) {
> +   game.epoch[i].start_year = secfile_lookup_int(file,
> +                                              "game.epoch%d.start_year", i);
> +   game.epoch[i].turn_increment = secfile_lookup_int(file,
> +                                                  
> "game.epoch%d.turn_increment", i);
> +  };

Does this mean the game.ruleset information is duplicated into 
the savefile?  In some ways this is reasonable, but on the other
hand it can potentially (especially for the larger rulesets) 
create alot of unnecessary bloat in the savefiles, considering
that the same information goes in every autosave file, when it
is already available in the ruleset file, accessable via the
"game" server option value (which _is_ in the savefile).

>    if (has_capability("rulesets",savefile_options)) {
> +    strcpy(game.ruleset.game,
> +        secfile_lookup_str(file, "game.ruleset.game"));

Use secfile_lookup_str_default() here, to allow reading old 
savefiles.  (And then _check_ that it does work with old
savefiles :-)  Eg, after loading a savefile you should then
load the ruleset file it uses, which should default to the
default ruleset file for old savegames.

> +++ server/ruleset.c  Mon Jul  5 07:45:22 1999
> @@ -14,6 +14,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <ctype.h>
> +#include <values.h>

I assume values.h is for MAXINT?  But is this a standard header?
I have a vague feeling limits.h would be better?  (More standard.)

Though even then I'm not sure if its a good idea: it potentially
allows platform dependent ruleset files and savefiles, if someone 
uses an int value which is in range on one platform but not on others. 

> +++ common/shared.h   Mon Jul  5 06:51:05 1999
> @@ -25,6 +25,7 @@
>  #define MAX_PLAYERS 14
>  #define MAX_LENGTH_NAME 32
>  #define MAX_LENGTH_ADDRESS 32
> +#define MAX_EPOCHS 10

As others, I wonder if hardcoding this is necessary.

> +++ server/stdinhand.c        Mon Jul  5 07:38:08 1999
> @@ -263,6 +263,15 @@
>      "Number of initial advances per player", "" },
>  
> - * ("endyear" is here, and not RULES_FLEXIBLE, because it doesn't
> - * affect what happens in the game, it just determines when the
> - * players stop playing and look at the score.)
>   */
> -  { "endyear", &game.end_year,
> -    SSET_META, SSET_TO_CLIENT,
> -    GAME_MIN_END_YEAR, GAME_MAX_END_YEAR, GAME_DEFAULT_END_YEAR,
> -    "Year the game ends", "" },
> -

I'm not too happy about endyear being completely removed
as a server option.  Because the server option is useful
_during_ games, not just at the start, which is functionality
which would now be lost.

I would prefer to keep it as a server option, even at the cost 
of making things a bit more complicated, such as not being able
to set endyear until after the game.ruleset file has
been loaded.  

Regards,
-- David