Merge lp:~nha/widelands/soldier-cleanup into lp:widelands

[Nicolai Hähnle]

Let soldier attributes be computed directly from their respective level. Reduces the number of variables stored in Soldier.

Gameplay is affected in that the max HP of soldiers is no longer random, but always the same (depending obviously on the soldier's level). See http://wl.widelands.org/forum/topic/469/?page=1#post-2525

[SirVer (sirver)]

seems clear enough to me

review: Approve

=== modified file 'src/logic/soldier.cc'
--- src/logic/soldier.cc	2010-04-13 16:47:49 +0000
+++ src/logic/soldier.cc	2010-05-08 19:52:28 +0000
@@ -55,28 +55,7 @@
 {
 	add_attribute(Map_Object::SOLDIER);
 
-	try { //  hitpoints
-		const char * const hp = global_s.get_safe_string("hp");
-		std::vector<std::string> list(split_string(hp, "-"));
-		if (list.size() != 2)
-			throw game_data_error
-				(_("expected %s but found \"%s\""), _("\"min-max\""), hp);
-		container_iterate(std::vector<std::string>, list, i)
-			remove_spaces(*i.current);
-		char * endp;
-		m_min_hp = strtol(list[0].c_str(), &endp, 0);
-		if (*endp or 0 == m_min_hp)
-			throw game_data_error
-				(_("expected %s but found \"%s\""),
-				 _("positive integer"), list[0].c_str());
-		m_max_hp = strtol(list[1].c_str(), &endp, 0);
-		if (*endp or m_max_hp < m_min_hp)
-			throw game_data_error
-				(_("expected positive integer >= %u but found \"%s\""),
-				 m_min_hp, list[1].c_str());
-	} catch (_wexception const & e) {
-		throw game_data_error("hp: %s", e.what());
-	}
+	m_base_hp = global_s.get_safe_positive("hp");
 
 	try { //  parse attack
 		const char * const attack = global_s.get_safe_string("attack");
@@ -343,23 +322,7 @@
 	m_defense_level = 0;
 	m_evade_level   = 0;
 
-	m_hp_max        = 0;
-	m_min_attack    = descr().get_min_attack();
-	m_max_attack    = descr().get_max_attack();
-	m_defense       = descr().get_defense   ();
-	m_evade         = descr().get_evade     ();
-	{
-		const uint32_t min_hp = descr().get_min_hp();
-		assert(min_hp);
-		assert(min_hp <= descr().get_max_hp());
-		m_hp_max =
-			min_hp
-			+
-			ref_cast<Game, Editor_Game_Base>(egbase).logic_rand()
-			%
-			(descr().get_max_hp() - (min_hp - 1));
-	}
-	m_hp_current    = m_hp_max;
+	m_hp_current    = get_max_hitpoints();
 
 	m_combat_walking   = CD_NONE;
 	m_combat_walkstart = 0;
@@ -391,39 +354,25 @@
 	assert(m_hp_level <= hp);
 	assert              (hp <= descr().get_max_hp_level());
 
-	while (m_hp_level < hp) {
-		++m_hp_level;
-		m_hp_max     += descr().get_hp_incr_per_level();
-		m_hp_current += descr().get_hp_incr_per_level();
-	}
+	m_hp_level = hp;
 }
 void Soldier::set_attack_level(const uint32_t attack) {
 	assert(m_attack_level <= attack);
 	assert                  (attack <= descr().get_max_attack_level());
 
-	while (m_attack_level < attack) {
-		++m_attack_level;
-		m_min_attack += descr().get_attack_incr_per_level();
-		m_max_attack += descr().get_attack_incr_per_level();
-	}
+	m_attack_level = attack;
 }
 void Soldier::set_defense_level(const uint32_t defense) {
 	assert(m_defense_level <= defense);
 	assert                   (defense <= descr().get_max_defense_level());
 
-	while (m_defense_level < defense) {
-		++m_defense_level;
-		m_defense += descr().get_defense_incr_per_level();
-	}
+	m_defense_level = defense;
 }
 void Soldier::set_evade_level(const uint32_t evade) {
 	assert(m_evade_level <= evade);
 	assert                 (evade <= descr().get_max_evade_level());
 
-	while (m_evade_level < evade) {
-		++m_evade_level;
-		m_evade += descr().get_evade_incr_per_level();
-	}
+	m_evade_level = evade;
 }
 
 uint32_t Soldier::get_level(tAttribute const at) const {
@@ -453,13 +402,38 @@
 	return Worker::get_tattribute(attr);
 }
 
+uint32_t Soldier::get_max_hitpoints() const
+{
+	return descr().get_base_hp() + m_hp_level*descr().get_hp_incr_per_level();
+}
+
+uint32_t Soldier::get_min_attack() const
+{
+	return descr().get_base_min_attack() + m_attack_level*descr().get_attack_incr_per_level();
+}
+
+uint32_t Soldier::get_max_attack() const
+{
+	return descr().get_base_max_attack() + m_attack_level*descr().get_attack_incr_per_level();
+}
+
+uint32_t Soldier::get_defense() const
+{
+	return descr().get_base_defense() + m_defense_level*descr().get_defense_incr_per_level();
+}
+
+uint32_t Soldier::get_evade() const
+{
+	return descr().get_base_evade() + m_evade_level*descr().get_evade_incr_per_level();
+}
+
 //  Unsignedness ensures that we can only heal, not hurt through this method.
 void Soldier::heal (const uint32_t hp) {
-	molog ("[soldier] healing (%d+)%d/%d\n", hp, m_hp_current, m_hp_max);
+	molog ("[soldier] healing (%d+)%d/%d\n", hp, m_hp_current, get_max_hitpoints());
 	assert(hp);
-	assert(m_hp_current <  m_hp_max);
-	m_hp_current += std::min(hp, m_hp_max - m_hp_current);
-	assert(m_hp_current <= m_hp_max);
+	assert(m_hp_current <  get_max_hitpoints());
+	m_hp_current += std::min(hp, get_max_hitpoints() - m_hp_current);
+	assert(m_hp_current <= get_max_hitpoints());
 }
 
 /**
@@ -469,7 +443,7 @@
 {
 	assert (m_hp_current > 0);
 
-	molog ("[soldier] damage %d(-%d)/%d\n", m_hp_current, value, m_hp_max);
+	molog ("[soldier] damage %d(-%d)/%d\n", m_hp_current, value, get_max_hitpoints());
 	if (m_hp_current < value)
 		m_hp_current = 0;
 	else
@@ -564,8 +538,8 @@
 		Rect r(Point(drawpos.x - w, drawpos.y - h - 7), w * 2, 5);
 		dst.draw_rect(r, HP_FRAMECOLOR);
 		// Draw the actual bar
-		assert(m_hp_max);
-		const float fraction = static_cast<float>(m_hp_current) / m_hp_max;
+		assert(get_max_hitpoints());
+		const float fraction = static_cast<float>(m_hp_current) / get_max_hitpoints();
 		RGBColor color(owner().get_playercolor()[2]);
 		assert(2 <= r.w);
 		assert(2 <= r.h);
@@ -1619,10 +1593,10 @@
 	molog
 		("Levels: %d/%d/%d/%d\n",
 		 m_hp_level, m_attack_level, m_defense_level, m_evade_level);
-	molog ("HitPoints: %d/%d\n", m_hp_current, m_hp_max);
-	molog ("Attack :  %d-%d\n", m_min_attack, m_max_attack);
-	molog ("Defense : %d%%\n", m_defense);
-	molog ("Evade:    %d%%\n", m_evade);
+	molog ("HitPoints: %d/%d\n", m_hp_current, get_max_hitpoints());
+	molog ("Attack :  %d-%d\n", get_min_attack(), get_max_attack());
+	molog ("Defense : %d%%\n", get_defense());
+	molog ("Evade:    %d%%\n", get_evade());
 	molog ("CombatWalkingDir:   %i\n", m_combat_walking);
 	molog ("CombatWalkingStart: %i\n", m_combat_walkstart);
 	molog ("CombatWalkEnd:      %i\n", m_combat_walkend);

=== modified file 'src/logic/soldier.h'
--- src/logic/soldier.h	2010-03-14 16:44:43 +0000
+++ src/logic/soldier.h	2010-05-08 19:52:28 +0000
@@ -54,12 +54,11 @@
 	uint32_t get_max_defense_level     () const {return m_max_defense_level;}
 	uint32_t get_max_evade_level       () const {return m_max_evade_level;}
 
-	uint32_t get_min_hp                () const {return m_min_hp;}
-	uint32_t get_max_hp                () const {return m_max_hp;}
-	uint32_t get_min_attack            () const {return m_min_attack;}
-	uint32_t get_max_attack            () const {return m_max_attack;}
-	uint32_t get_defense               () const {return m_defense;}
-	uint32_t get_evade                 () const {return m_evade;}
+	uint32_t get_base_hp        () const {return m_base_hp;}
+	uint32_t get_base_min_attack() const {return m_min_attack;}
+	uint32_t get_base_max_attack() const {return m_max_attack;}
+	uint32_t get_base_defense   () const {return m_defense;}
+	uint32_t get_base_evade     () const {return m_evade;}
 
 	uint32_t get_hp_incr_per_level     () const {return m_hp_incr;}
 	uint32_t get_attack_incr_per_level () const {return m_attack_incr;}
@@ -90,8 +89,7 @@
 	virtual Bob & create_object() const;
 
 	//  start values
-	uint32_t m_min_hp;
-	uint32_t m_max_hp;
+	uint32_t m_base_hp;
 	uint32_t m_min_attack;
 	uint32_t m_max_attack;
 	uint32_t m_defense;
@@ -110,10 +108,10 @@
 	uint32_t m_max_evade_level;
 
 	//  level pictures
-	std::vector<PictureID>    m_hp_pics;
-	std::vector<PictureID>    m_attack_pics;
-	std::vector<PictureID>    m_evade_pics;
-	std::vector<PictureID>    m_defense_pics;
+	std::vector<PictureID>   m_hp_pics;
+	std::vector<PictureID>   m_attack_pics;
+	std::vector<PictureID>   m_evade_pics;
+	std::vector<PictureID>   m_defense_pics;
 	std::vector<std::string> m_hp_pics_fn;
 	std::vector<std::string> m_attack_pics_fn;
 	std::vector<std::string> m_evade_pics_fn;
@@ -217,11 +215,11 @@
 	}
 
 	uint32_t get_current_hitpoints() const {return m_hp_current;}
-	uint32_t get_max_hitpoints    () const {return m_hp_max;}
-	uint32_t get_min_attack       () const {return m_min_attack;}
-	uint32_t get_max_attack       () const {return m_max_attack;}
-	uint32_t get_defense          () const {return m_defense;}
-	uint32_t get_evade            () const {return m_evade;}
+	uint32_t get_max_hitpoints() const;
+	uint32_t get_min_attack() const;
+	uint32_t get_max_attack() const;
+	uint32_t get_defense() const;
+	uint32_t get_evade() const;
 
 	PictureID get_hp_level_pic     () const {
 		return descr().get_hp_level_pic     (m_hp_level);
@@ -286,12 +284,6 @@
 
 private:
 	uint32_t m_hp_current;
-	uint32_t m_hp_max;
-	uint32_t m_min_attack;
-	uint32_t m_max_attack;
-	uint32_t m_defense;
-	uint32_t m_evade;
-
 	uint32_t m_hp_level;
 	uint32_t m_attack_level;
 	uint32_t m_defense_level;

=== modified file 'src/map_io/widelands_map_bobdata_data_packet.cc'
--- src/map_io/widelands_map_bobdata_data_packet.cc	2010-05-01 13:54:47 +0000
+++ src/map_io/widelands_map_bobdata_data_packet.cc	2010-05-08 19:52:28 +0000
@@ -51,7 +51,7 @@
 #define WORKER_BOB_PACKET_VERSION 1
 
 // Worker subtype versions
-#define SOLDIER_WORKER_BOB_PACKET_VERSION 6
+#define SOLDIER_WORKER_BOB_PACKET_VERSION 7
 #define CARRIER_WORKER_BOB_PACKET_VERSION 1
 
 
@@ -432,29 +432,16 @@
 					{
 						Soldier_Descr const & descr = soldier->descr();
 
-						uint32_t min_hp = descr.get_min_hp();
-						assert(min_hp);
 						soldier->m_hp_current = fr.Unsigned32();
-						soldier->m_hp_max = fr.Unsigned32();
-						// This has been commented because now exists a 'die' task,
-						// so a soldier can have 0 hitpoints if it's dying.
-						//if (not soldier->m_hp_current)
-						// throw game_data_error("no hitpoints (should be dead)");
-						if (soldier->m_hp_max < soldier->m_hp_current)
-							throw game_data_error
-								("hp_max (%u) < hp_current (%u)",
-								 soldier->m_hp_max, soldier->m_hp_current);
-
-						soldier->m_min_attack    = fr.Unsigned32();
-						soldier->m_max_attack    = fr.Unsigned32();
-						if (soldier->m_max_attack < soldier->m_min_attack)
-							throw game_data_error
-								("max_attack = %u but must be at least %u",
-								 soldier->m_max_attack, soldier->m_min_attack);
-
-						soldier->m_defense       = fr.Unsigned32();
-
-						soldier->m_evade         = fr.Unsigned32();
+
+						if (soldier_worker_bob_packet_version <= 6) {
+							// no longer used values
+							fr.Unsigned32(); // max hp
+							fr.Unsigned32(); // min attack
+							fr.Unsigned32(); // max attack
+							fr.Unsigned32(); // defense
+							fr.Unsigned32(); // evade
+						}
 
 #define READLEVEL(variable, pn, maxfunction)                                  \
    soldier->variable = fr.Unsigned32();                                       \
@@ -478,148 +465,8 @@
 						READLEVEL(m_defense_level, "defense", get_max_defense_level);
 						READLEVEL(m_evade_level,   "evade",   get_max_evade_level);
 
-						{ //  validate hp values
-							uint32_t const level_increase =
-								descr.get_hp_incr_per_level() * soldier->m_hp_level;
-							min_hp += level_increase;
-							uint32_t const max = descr.get_max_hp() + level_increase;
-							if (soldier->m_hp_max < min_hp) {
-								//  The soldier's type's definition may have changed,
-								//  so that max_hp must be larger. Adjust it and scale
-								//  up the current amount of hitpoints proportionally.
-								uint32_t const new_current =
-									soldier->m_hp_current * min_hp / soldier->m_hp_max;
-								log
-									("WARNING: %s %s (%u) of player %u has hp_max = %u "
-									 "but it must be at least %u (= %u + %u * %u), "
-									 "changing it to that value and increasing current "
-									 "hp from %u to %u\n",
-									 descr.tribe().name().c_str(),
-									 descr.descname().c_str(), soldier->serial(),
-									 soldier->owner().player_number(),
-									 soldier->m_hp_max, min_hp,
-									 descr.get_min_hp(),
-									 descr.get_hp_incr_per_level(), soldier->m_hp_level,
-									 soldier->m_hp_current, new_current);
-								soldier->m_hp_current = new_current;
-								soldier->m_hp_max = min_hp;
-							} else if (max < soldier->m_hp_max) {
-								//  The soldier's type's definition may have changed,
-								//  so that max_hp must be smaller. Adjust it and scale
-								//  down the current amount of hitpoints
-								//  proportionally. Round to the soldier's favour and
-								//  make sure that the scaling does not kill the
-								//  soldier.
-								uint32_t const new_current =
-									1
-									+
-									(soldier->m_hp_current * max - 1)
-									/
-									soldier->m_hp_max;
-								assert(new_current);
-								assert(new_current <= soldier->m_hp_max);
-								log
-									("WARNING: %s %s (%u) of player %u has hp_max = %u "
-									 "but it can be at most %u (= %u + %u * %u), "
-									 "changing it to that value and decreasing current "
-									 "hp from %u to %u\n",
-									 descr.tribe().name().c_str(),
-									 descr.descname().c_str(), soldier->serial(),
-									 soldier->owner().player_number(),
-									 soldier->m_hp_max, max,
-									 descr.get_max_hp(),
-									 descr.get_hp_incr_per_level(), soldier->m_hp_level,
-									 soldier->m_hp_current, new_current);
-								soldier->m_hp_current = new_current;
-								soldier->m_hp_max = max;
-							}
-						}
-
-						{ //  validate attack values
-							uint32_t const level_increase =
-								descr.get_attack_incr_per_level()
-								*
-								soldier->m_attack_level;
-							{
-								uint32_t const min =
-									descr.get_min_attack() + level_increase;
-								if (soldier->m_min_attack < min) {
-									log
-										("WARNING: %s %s (%u) of player %u has "
-										 "min_attack = %u but it must be at least %u (= "
-										 "%u + %u * %u), changing it to that value\n",
-										 descr.tribe().name().c_str(),
-										 descr.descname().c_str(), soldier->serial(),
-										 soldier->owner().player_number(),
-										 soldier->m_min_attack, min,
-										 descr.get_min_attack(),
-										 descr.get_attack_incr_per_level(),
-										 soldier->m_attack_level);
-									soldier->m_min_attack = min;
-									if (soldier->m_max_attack < min) {
-										log
-											(" (and changing max_attack from %u to the "
-											 "same value)",
-											 soldier->m_max_attack);
-										soldier->m_max_attack = min;
-									}
-									log("\n");
-								}
-							}
-							{
-								uint32_t const max =
-									descr.get_max_attack() + level_increase;
-								if (max < soldier->m_max_attack) {
-									log
-										("WARNING: %s %s (%u) of player %u has "
-										 "max_attack = %u but it can be at most %u (= "
-										 "%u + %u * %u), changing it to that value",
-										 descr.tribe().name().c_str(),
-										 descr.descname().c_str(), soldier->serial(),
-										 soldier->owner().player_number(),
-										 soldier->m_max_attack, max,
-										 descr.get_max_attack(),
-										 descr.get_attack_incr_per_level(),
-										 soldier->m_attack_level);
-									soldier->m_max_attack = max;
-									if (max < soldier->m_min_attack) {
-										log
-											(" (and changing min_attack from %u to the "
-											 "same value)",
-											 soldier->m_min_attack);
-										soldier->m_min_attack = max;
-									}
-									log("\n");
-								}
-							}
-							assert(soldier->m_min_attack <= soldier->m_max_attack);
-						}
-
-#define VALIDATE_VALUE(pn, valuefunct, incrfunct, level_variable, variable)   \
-   {                                                                          \
-      uint32_t const value =                                                  \
-         descr.valuefunct() + descr.incrfunct() * soldier->level_variable;    \
-      if (value != soldier->variable) {                                       \
-         log                                                                  \
-            ("WARNING: %s %s (%u) of player %u has "                          \
-             pn                                                               \
-             " = %u but it must be %u (= %u + %u * %u), changing it to that " \
-             "value\n",                                                       \
-             descr.tribe().name().c_str(), descr.descname().c_str(),          \
-             soldier->serial(), soldier->owner().player_number(),             \
-             soldier->variable, value,                                        \
-             descr.valuefunct(), descr.incrfunct(), soldier->level_variable); \
-         soldier->variable = value;                                           \
-      }                                                                       \
-   }                                                                          \
-
-						VALIDATE_VALUE
-							("defense", get_defense, get_defense_incr_per_level,
-							 m_defense_level, m_defense);
-
-						VALIDATE_VALUE
-							("evade",   get_evade,   get_evade_incr_per_level,
-							 m_evade_level,   m_evade);
+						if (soldier->m_hp_current > soldier->get_max_hitpoints())
+							soldier->m_hp_current = soldier->get_max_hitpoints();
 
 						if (Serial const battle = fr.Unsigned32())
 							soldier->m_battle = &mol.get<Battle>(battle);
@@ -882,11 +729,6 @@
 	if (upcast(Soldier const, soldier, &worker)) {
 		fw.Unsigned16(SOLDIER_WORKER_BOB_PACKET_VERSION);
 		fw.Unsigned32(soldier->m_hp_current);
-		fw.Unsigned32(soldier->m_hp_max);
-		fw.Unsigned32(soldier->m_min_attack);
-		fw.Unsigned32(soldier->m_max_attack);
-		fw.Unsigned32(soldier->m_defense);
-		fw.Unsigned32(soldier->m_evade);
 		fw.Unsigned32(soldier->m_hp_level);
 		fw.Unsigned32(soldier->m_attack_level);
 		fw.Unsigned32(soldier->m_defense_level);

=== modified file 'src/writeHTML.cc'
--- src/writeHTML.cc	2010-04-01 10:59:34 +0000
+++ src/writeHTML.cc	2010-05-08 19:52:28 +0000
@@ -592,9 +592,9 @@
 	snprintf
 		(buffer, sizeof(buffer),
 		 _
-		 	("Hitpoints is between %u and %u, plus %u for each level above 0 "
+		 	("Hitpoints is %u, plus %u for each level above 0 "
 		 	 "(maximum level is %u)."),
-		 m_min_hp,      m_max_hp,      m_hp_incr,      m_max_hp_level);
+		 m_base_hp,      m_hp_incr,      m_max_hp_level);
 	fw.Text(buffer);
 	fw.Text
 		("</p>\n"

=== modified file 'tribes/atlanteans/soldier/conf'
--- tribes/atlanteans/soldier/conf	2009-12-14 11:04:15 +0000
+++ tribes/atlanteans/soldier/conf	2010-05-08 19:52:28 +0000
@@ -5,7 +5,7 @@
 max_evade_level=2
 
 # initial values and per level increasements
-hp=120-150
+hp=135
 hp_incr_per_level=40
 attack=12-16
 attack_incr_per_level=8

=== modified file 'tribes/barbarians/soldier/conf'
--- tribes/barbarians/soldier/conf	2009-12-14 11:04:15 +0000
+++ tribes/barbarians/soldier/conf	2010-05-08 19:52:28 +0000
@@ -7,7 +7,7 @@
 max_evade_level=2
 
 # initial values and per level increasements
-hp=110-150
+hp=130
 hp_incr_per_level=28
 attack=12-16
 attack_incr_per_level=7

=== modified file 'tribes/empire/soldier/conf'
--- tribes/empire/soldier/conf	2009-12-14 11:04:15 +0000
+++ tribes/empire/soldier/conf	2010-05-08 19:52:28 +0000
@@ -8,7 +8,7 @@
 max_evade_level=2
 
 # initial values and per level increasements
-hp=110-150
+hp=130
 hp_incr_per_level=21
 attack=13-15
 attack_incr_per_level=8