Complete.Org: Mailing Lists: Archives: freeciv-dev: April 2003:
[Freeciv-Dev] Re: Multiple veteran level system patch
Home

[Freeciv-Dev] Re: Multiple veteran level system patch

[Top] [All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
To: James <james.blewitt@xxxxxxxxxxxxxx>
Cc: freeciv-dev@xxxxxxxxxxx
Subject: [Freeciv-Dev] Re: Multiple veteran level system patch
From: "Per I. Mathisen" <per@xxxxxxxxxxx>
Date: Fri, 18 Apr 2003 12:30:30 +0000 (GMT)

On Mon, 14 Apr 2003, James wrote:
> First version of the combined multiple level veteran patch has been uploaded
> to /pub/freeciv/incoming/multi_level_veteran_patch.diff

Nice. A few complaints, however:

The patch is reversed. Also, the gui-mui part of the patch is different
from all other clients.

You indent a lot of empty lines. This is bad. Remove those from the patch.

Is game.rgame.expanded_veteran_combat really necessary? Can't we just
define the classic veteran levels in terms of the new?

This can't be right:

 int get_virtual_defense_power(Unit_Type_id att_type, Unit_Type_id def_type,
-                             int x, int y, bool fortified, int veteran)
+                             int x, int y, bool fortified, bool veteran)
 {
   int defensepower = unit_types[def_type].defense_strength;
   enum tile_terrain_type t = map_get_terrain(x, y);
@@ -411,7 +402,7 @@
   }
   defensepower *= db;

-  if (veteran == 1) {
+  if (veteran) {
     defensepower = (defensepower * 3) / 2;
   }

What happens if veteran > 1?

expanded_veteran_combat        = 0
expanded_veteran_settlers      = 0
expanded_veteran_triremes      = 0
expanded_veteran_diplomats     = 0
expanded_veteran_caravans      = 0

I don't think these are needed. If there is only one level defined, ie
"veteran_combat_raise_chance = 50, 0, 0, 0, 0, 0", then there will be no
"expanded" veteral levels. This means normal civ1/2 ruleset should have
veteran_power_fact = 50, eg.

num_veteran_levels = 6 is also unnecessary when you can limit the number
of levels with veteran_combat_raise_chance, IMHO.

I am not sure that I am happy about the way diplomats become veteran.
Shouldn't they become veteran by doing successful missions like other
units? I think so.

+      if (myrand(100)<1.5*game.vet_combat_raise_chance[punit->veteran]) {

Style: Too few spaces in some places.

A limit on six veteran levels is okay with me. Don't know what other
people say about that.

Other than this, the patch looks fine. Please look at the above, and post
an updated and gzipped version to rt@xxxxxxxxxxx.

  - Per



[Prev in Thread] Current Thread [Next in Thread]