Use [leader] in the c++ code (#9660)

Added code to convert all  `[side]type=` to `[side][leader]` when the `[scenario]` is read, and the rest of the engine code to only work with [leader], this should fix all possible problems with `[leader]` and also simplify the code a bit

Fixes #7985
Fixes #3742

src/game_initialization/connect_engine.cpp

@@ -846,12 +846,18 @@ side_engine::side_engine(const config& cfg, connect_engine& parent_engine, const
 
 	// Save default attributes that could be overwritten by the faction, so that correct faction lists would be
 	// initialized by flg_manager when the new side config is sent over network.
+	cfg_.clear_children("default_faction");
 	cfg_.add_child("default_faction", config {
-		"type",    cfg_["type"],
-		"gender",  cfg_["gender"],
 		"faction", cfg_["faction"],
 		"recruit", cfg_["recruit"],
 	});
+	if(auto p_cfg = cfg_.optional_child("leader")) {
+		cfg_.mandatory_child("default_faction").add_child("leader", config {
+			"type", (p_cfg)["type"],
+			"gender", (p_cfg)["gender"],
+		});
+	}
+
 
 	if(cfg_["side"].to_int(index_ + 1) != index_ + 1) {
 		ERR_CF << "found invalid side=" << cfg_["side"].to_int(index_ + 1) << " in definition of side number " << index_ + 1;
@@ -1030,42 +1036,20 @@ config side_engine::new_config() const
 	res["chose_random"] = chose_random_;
 
 	if(parent_.params_.saved_game != saved_game_mode::type::midgame) {
-		// Find a config where a default leader is and set a new type and gender values for it.
-		config* leader = &res;
 
-		if(flg_.default_leader_cfg() != nullptr) {
-			for(config& side_unit : res.child_range("unit")) {
-				if(*flg_.default_leader_cfg() != side_unit) {
-					continue;
-				}
-
-				leader = &side_unit;
-
-				if(flg_.current_leader() != (*leader)["type"]) {
-					// If a new leader type was selected from carryover, make sure that we reset the leader.
-					std::string leader_id = (*leader)["id"];
-					leader->clear();
-
-					if(!leader_id.empty()) {
-						(*leader)["id"] = leader_id;
-					}
-				}
-
-				break;
+		if(!flg_.leader_lock()) {
+			if(controller_ != CNTR_EMPTY) {
+				auto& leader = res.child_or_add("leader");
+				leader["type"] = flg_.current_leader();
+				leader["gender"] = flg_.current_gender();
+				LOG_MP << "side_engine::new_config: side=" << index_ + 1 << " type=" << leader["type"]
+					   << " gender=" << leader["gender"];
+			} else if(!controller_lock_) {
+				//if controller_lock_ == false and controller_ == CNTR_EMPTY, this means the user disalbles this side, so remove it's leader.
+				res.remove_children("leader");
 			}
 		}
 
-		// NOTE: the presence of a type= key overrides no_leader
-		if(controller_ != CNTR_EMPTY) {
-			(*leader)["type"] = flg_.current_leader();
-			(*leader)["gender"] = flg_.current_gender();
-			LOG_MP << "side_engine::new_config: side=" << index_ + 1 << " type=" << (*leader)["type"] << " gender=" << (*leader)["gender"];
-		} else if(!controller_lock_) {
-			// TODO: FIX THIS SHIT! We shouldn't have a special string to denote no-leader-ness...
-			(*leader)["type"] = "null";
-			(*leader)["gender"] = "null";
-		}
-
 		const std::string& new_team_name = parent_.team_data_[team_].team_name;
 
 		if(res["user_team_name"].empty() || !parent_.params_.use_map_settings || res["team_name"] != new_team_name) {

src/game_initialization/flg_manager.cpp

@@ -37,9 +37,6 @@ flg_manager::flg_manager(const std::vector<const config*>& era_factions,
 	: era_factions_(era_factions)
 	, side_num_(side["side"].to_int())
 	, faction_from_recruit_(side["faction_from_recruit"].to_bool())
-	, original_type_(get_default_faction(side)["type"].str())
-	, original_gender_(get_default_faction(side)["gender"].str())
-	, savegame_gender_()
 	, original_faction_(get_default_faction(side)["faction"].str())
 	, original_recruit_(utils::split(get_default_faction(side)["recruit"].str()))
 	, saved_game_(saved_game)
@@ -55,48 +52,55 @@ flg_manager::flg_manager(const std::vector<const config*>& era_factions,
 	, current_faction_(nullptr)
 	, current_leader_("null")
 	, current_gender_("null")
-	, default_leader_type_(side["type"])
-	, default_leader_gender_(side["gender"])
-	, default_leader_cfg_(nullptr)
+	, default_leader_type_("")
+	, default_leader_gender_("")
 {
-	const std::string& leader_id = side["id"];
-	if(!leader_id.empty()) {
-		// Check if leader was carried over and now is in [unit] tag.
-		default_leader_cfg_ = side.find_child("unit", "id", leader_id).ptr();
-		if(default_leader_cfg_) {
-			default_leader_type_ = (*default_leader_cfg_)["type"].str();
-			default_leader_gender_ = (*default_leader_cfg_)["gender"].str();
-		} else {
-			default_leader_cfg_ = nullptr;
-		}
-	} else if(default_leader_type_.empty()) {
-		// Find a unit which can recruit.
-		for(const config& side_unit : side.child_range("unit")) {
-			if(side_unit["canrecruit"].to_bool()) {
-				default_leader_type_ = side_unit["type"].str();
-				default_leader_gender_ = side_unit["gender"].str();
-				default_leader_cfg_ = &side_unit;
-				break;
-			}
+	std::string leader_id = side["id"];
+	bool found_leader;
+
+	leader_lock_ = leader_lock_ && (use_map_settings || lock_settings || default_leader_type_.empty());
+	faction_lock_ = faction_lock_ && (use_map_settings || lock_settings);
+
+	auto set_leader = [&](const config& cfg) {
+		found_leader = true;
+		leader_id = cfg["id"];
+		default_leader_type_ = cfg["type"];
+		default_leader_gender_ = cfg["gender"];
+	};
+
+	if(auto p_cfg = side.optional_child("leader")) {
+		set_leader(*p_cfg);
+		// If we are not the host of the game it can happen that the code overwrote
+		// the [leaders] type/gender and the original values are found in [default_faction]
+		// we still need the id from p_cfg tho.
+		if(auto p_ocfg = get_default_faction(side).optional_child("leader")) {
+			default_leader_type_ = (*p_ocfg)["type"];
+			default_leader_gender_ = (*p_ocfg)["gender"];
 		}
 	}
 
+	if(!leader_id.empty()) {
+		// Check if leader was carried over and now is in [unit] tag.
+		// (in this case we dont allow changing it, but we still want to show the corect unit type in the dialogs)
+		if(auto p_cfg = side.find_child("unit", "id", leader_id)) {
+			set_leader(*p_cfg);
+			leader_lock_ = true;
+		}
+	}
+
+	if(!found_leader) {
+		// Find a unit which can recruit.
+		if(auto p_cfg = side.find_child("unit", "canrecruit", "yes")) {
+			set_leader(*p_cfg);
+			leader_lock_ = true;
+		}
+	}
+
+
 	if(!default_leader_type_.empty() && default_leader_type_ != "random") {
 		if(unit_types.find(default_leader_type_) == nullptr) {
 			default_leader_type_.clear();
 			default_leader_gender_.clear();
-			default_leader_cfg_ = nullptr;
-		}
-	}
-
-	leader_lock_ = leader_lock_ && (use_map_settings || lock_settings || default_leader_type_.empty());
-	faction_lock_ = faction_lock_ && (use_map_settings || lock_settings);
-
-	//TODO: this code looks wrong, we should probably just use default_leader_gender_ from above.
-	for(const config& side_unit : side.child_range("unit")) {
-		if(current_leader_ == side_unit["type"] && side_unit["canrecruit"].to_bool()) {
-			savegame_gender_ = side_unit["gender"].str();
-			break;
 		}
 	}
 
@@ -340,8 +344,8 @@ void flg_manager::update_available_genders()
 	available_genders_.clear();
 
 	if(saved_game_) {
-		if(!savegame_gender_.empty()) {
-			available_genders_.push_back(savegame_gender_);
+		if(!default_leader_gender_.empty()) {
+			available_genders_.push_back(default_leader_gender_);
 		}
 	} else {
 		if(const unit_type* unit = unit_types.find(current_leader_)) {

src/game_initialization/flg_manager.hpp

@@ -71,15 +71,14 @@ public:
 	const std::string& current_gender() const
 		{ return current_gender_; }
 
-	const config* default_leader_cfg() const
-		{ return default_leader_cfg_; }
-
 	int current_faction_index() const;
 
 	int current_leader_index() const
 		{ return leader_index(current_leader_); }
 	int current_gender_index() const
 		{ return gender_index(current_gender_); }
+	bool leader_lock() const
+		{ return leader_lock_; }
 
 private:
 	flg_manager(const flg_manager&) = delete;
@@ -109,9 +108,6 @@ private:
 	const int side_num_;
 	const bool faction_from_recruit_;
 
-	const std::string original_type_;
-	const std::string original_gender_;
-	std::string savegame_gender_;
 	const std::string original_faction_;
 	const std::vector<std::string> original_recruit_;
 	const bool saved_game_;
@@ -135,7 +131,6 @@ private:
 
 	std::string default_leader_type_;
 	std::string default_leader_gender_;
-	const config* default_leader_cfg_;
 
 	static const config& get_default_faction(const config& cfg);
 };

src/gui/dialogs/multiplayer/mp_join_game.cpp

@@ -385,8 +385,10 @@ void mp_join_game::generate_side_list()
 		data.emplace("side_number", item);
 
 		std::string leader_image = ng::random_enemy_picture;
-		std::string leader_type = side["type"];
-		std::string leader_gender = side["gender"];
+
+		const config& leader = side.child_or_empty("leader");
+		std::string leader_type = leader["type"];
+		std::string leader_gender = leader["gender"];
 		std::string leader_name;
 
 		// If there is a unit which can recruit, use it as a leader.

src/saved_game.cpp

@@ -74,6 +74,7 @@
 #include "serialization/binary_or_text.hpp"
 #include "side_controller.hpp"
 #include "utils/general.hpp"
+#include "team.hpp" // for team::attributes, team::variables
 #include "variable.hpp" // for config_variable_set
 #include "variable_info.hpp"
 
@@ -253,6 +254,27 @@ void saved_game::set_defaults()
 			side["save_id"] = side.child_or_empty("leader")["id"];
 		}
 
+		// If this side tag describes the leader of the side, convert it into a [leader] tag here, by doing this here,
+		// all code that follows, no longer has to hande the possibility of leader information directly in [side].
+
+		// If this side tag describes the leader of the side
+		if(!side["type"].empty() && side["type"] != "null") {
+			auto temp = config{};
+
+			for(const std::string& tag : team::tags) {
+				temp.append_children_by_move(side, tag);
+			}
+			for(const std::string& attr : team::attributes) {
+				if(side.has_attribute(attr)) {
+					temp[attr] = side[attr];
+					side.remove_attribute(attr);
+				}
+			}
+			temp["side"] = side["side"];
+			temp.swap(side);
+			temp.swap(side.add_child_at("leader", config(), 0));
+		}
+
 		if(!is_multiplayer_tag) {
 			if(side["name"].blank()) {
 				side["name"] = side.child_or_empty("leader")["name"];

src/team.cpp

@@ -116,6 +116,15 @@ const std::set<std::string> team::attributes {
 	"description"
 };
 
+// Update this list of child tags if you change what is used to define a side
+// (excluding those attributes used to define the side's leader).
+const std::set<std::string> team::tags {
+	"ai",
+	"leader",
+	"unit",
+	"variables",
+	"village"
+};
 team::team_info::team_info()
 	: gold(0)
 	, start_gold(0)

src/team.hpp

@@ -153,6 +153,11 @@ public:
 	 * from a side's config before using it to create the side's leader.
 	 */
 	static const std::set<std::string> attributes;
+	/**
+	 * Stores the child tags recognized by [side]. These should be stripped
+	 * from a side's config before using it to create the side's leader.
+	 */
+	static const std::set<std::string> tags;
 
 	void build(const config &cfg, const gamemap &map, int gold = default_team_gold_);
 

src/teambuilder.cpp

@@ -183,14 +183,6 @@ void team_builder::handle_leader(const config& leader)
 	leader_configs_.push_back(leader);
 	config& stored = leader_configs_.back();
 
-	// Remove the attributes used to define a side.
-	for(const std::string& attr : team::attributes) {
-		stored.remove_attribute(attr);
-	}
-
-    // Remove [ai] tag as it is already added for the side
-	stored.remove_children("ai");
-
 	// Provide some default values, if not specified.
 	config::attribute_value& a1 = stored["canrecruit"];
 	if(a1.blank()) {
@@ -209,16 +201,6 @@ void team_builder::handle_leader(const config& leader)
 void team_builder::leader()
 {
 	log_step("leader");
-	// If this side tag describes the leader of the side, we can simply add it to front of unit queue
-	// there was a hack: if this side tag describes the leader of the side,
-	// we may replace the leader with someone from recall list who can recruit, but take positioning from [side]
-	// this hack shall be removed, since it messes up with 'multiple leaders'
-
-	// If this side tag describes the leader of the side
-	if(!side_cfg_["type"].empty() && side_cfg_["type"] != "null") {
-		handle_leader(side_cfg_);
-	}
-
 	for(const config& l : side_cfg_.child_range("leader")) {
 		handle_leader(l);
 	}

src/tests/test_mp_connect.cpp

@@ -97,6 +97,7 @@ static ng::side_engine* create_side_engine(const config& defaults,
 	config side_cfg = connect_engine->current_config()->mandatory_child("side");
 	side_cfg.remove_attributes("faction");
 	side_cfg.clear_children("default_faction");
+	side_cfg.clear_children("leader");
 	side_cfg.append(defaults);
 
 	return new ng::side_engine(side_cfg, *connect_engine, 0);
@@ -232,12 +233,16 @@ BOOST_AUTO_TEST_CASE( flg_map_settings9 )
 	config side;
 
 	// Valid leader unit.
-	side.clear();
-	side["type"] = "Shadow";
+	side = config {
+		"leader", config {
+			"type", "Shadow",
+		},
+	};
+
 	side_engine.reset(create_side_engine(side, connect_engine.get()));
 	BOOST_CHECK_EQUAL( side_engine->flg().choosable_leaders().size(), 1 );
 	BOOST_CHECK_EQUAL( side_engine->flg().current_leader(), "Shadow" );
-	BOOST_CHECK_EQUAL( side_engine->new_config()["type"], "Shadow" );
+	BOOST_CHECK_EQUAL( side_engine->new_config().mandatory_child("leader")["type"], "Shadow" );
 }
 
 BOOST_AUTO_TEST_CASE( flg_map_settings10 )
@@ -250,8 +255,12 @@ BOOST_AUTO_TEST_CASE( flg_map_settings10 )
 	config side;
 
 	// Invalid leader unit.
-	side.clear();
-	side["type"] = "ThisUnitDoesNotExist";
+	side = config {
+		"leader", config {
+			"type", "ThisUnitDoesNotExist",
+		},
+	};
+
 	side_engine.reset(create_side_engine(side, connect_engine.get()));
 	BOOST_CHECK_EQUAL( side_engine->flg().choosable_leaders().size(), 1 );
 	BOOST_CHECK_EQUAL( side_engine->flg().current_leader(), "null" );
@@ -330,12 +339,17 @@ BOOST_AUTO_TEST_CASE( flg_map_settings15 )
 	config side;
 
 	// Carried over leader.
-	side.clear();
-	side["id"] = "LeaderID";
-	side["type"] = "Elvish Archer";
-	config& unit = side.add_child("unit");
-	unit["id"] = "LeaderID";
-	unit["type"] = "Elvish Ranger";
+	side = config {
+		"leader", config {
+			"type", "Elvish Archer",
+			"id", "LeaderID",
+		},
+		"unit", config {
+			"type", "Elvish Ranger",
+			"id", "LeaderID",
+		},
+	};
+
 	side_engine.reset(create_side_engine(side, connect_engine.get()));
 	BOOST_CHECK_EQUAL( side_engine->flg().choosable_leaders().size(), 1 );
 	BOOST_CHECK_EQUAL( side_engine->flg().current_leader(), "Elvish Ranger" );
@@ -363,8 +377,11 @@ BOOST_AUTO_TEST_CASE( flg_map_settings17 )
 	config side;
 
 	// Random leader.
-	side.clear();
-	side["type"] = "random";
+	side = config {
+		"leader", config {
+			"type", "random",
+		},
+	};
 	side_engine.reset(create_side_engine(side, connect_engine.get()));
 	BOOST_CHECK_EQUAL( side_engine->flg().choosable_leaders().size(), 1 );
 }
@@ -379,8 +396,11 @@ BOOST_AUTO_TEST_CASE( flg_map_settings18 )
 	config side;
 
 	// Leader with both genders.
-	side.clear();
-	side["type"] = "Elvish Archer";
+	side = config {
+		"leader", config {
+			"type", "Elvish Archer",
+		},
+	};
 	side_engine.reset(create_side_engine(side, connect_engine.get()));
 	BOOST_CHECK_EQUAL( side_engine->flg().choosable_genders().size(), 3 );
 	BOOST_CHECK_EQUAL( side_engine->flg().current_gender(), "random" );
@@ -396,8 +416,11 @@ BOOST_AUTO_TEST_CASE( flg_map_settings19 )
 	config side;
 
 	// Leader with only male gender.
-	side.clear();
-	side["type"] = "Swordsman";
+	side = config {
+		"leader", config {
+			"type", "Swordsman",
+		},
+	};
 	side_engine.reset(create_side_engine(side, connect_engine.get()));
 	BOOST_CHECK_EQUAL( side_engine->flg().choosable_genders().size(), 1 );
 	BOOST_CHECK_EQUAL( side_engine->flg().current_gender(), "male" );
@@ -413,8 +436,11 @@ BOOST_AUTO_TEST_CASE( flg_map_settings20 )
 	config side;
 
 	// Leader with only female gender.
-	side.clear();
-	side["type"] = "Elvish Druid";
+	side = config {
+		"leader", config {
+			"type", "Elvish Druid",
+		},
+	};
 	side_engine.reset(create_side_engine(side, connect_engine.get()));
 	BOOST_CHECK_EQUAL( side_engine->flg().choosable_genders().size(), 1 );
 	BOOST_CHECK_EQUAL( side_engine->flg().current_gender(), "female" );
@@ -430,9 +456,12 @@ BOOST_AUTO_TEST_CASE( flg_map_settings21 )
 	config side;
 
 	// Valid leader with valid gender.
-	side.clear();
-	side["type"] = "White Mage";
-	side["gender"] = "female";
+	side = config {
+		"leader", config {
+			"type", "White Mage",
+			"gender", "female",
+		},
+	};
 	side_engine.reset(create_side_engine(side, connect_engine.get()));
 	BOOST_CHECK_EQUAL( side_engine->flg().current_gender(), "female" );
 }
@@ -447,9 +476,12 @@ BOOST_AUTO_TEST_CASE( flg_map_settings22 )
 	config side;
 
 	// Valid leader with invalid gender.
-	side.clear();
-	side["type"] = "Troll";
-	side["gender"] = "female";
+	side = config {
+		"leader", config {
+			"type", "Troll",
+			"gender", "female",
+		},
+	};
 	side_engine.reset(create_side_engine(side, connect_engine.get()));
 	BOOST_CHECK_EQUAL( side_engine->flg().choosable_genders().size(), 1 );
 	BOOST_CHECK_EQUAL( side_engine->flg().current_gender(), "male" );
@@ -465,9 +497,12 @@ BOOST_AUTO_TEST_CASE( flg_map_settings23 )
 	config side;
 
 	// Leader with random gender.
-	side.clear();
-	side["type"] = "White Mage";
-	side["gender"] = "random";
+	side = config {
+		"leader", config {
+			"type", "White Mage",
+			"gender", "random",
+		},
+	};
 	side_engine.reset(create_side_engine(side, connect_engine.get()));
 	BOOST_CHECK_EQUAL( side_engine->flg().current_gender(), "random" );
 }
@@ -530,9 +565,12 @@ BOOST_AUTO_TEST_CASE( flg_map_settings27 )
 	config side;
 
 	// Resolve random faction with default leader.
-	side.clear();
-	side["faction"] = "Random";
-	side["type"] = "Troll";
+	side = config {
+		"faction" , "Random",
+		"leader", config {
+			"type", "Troll",
+		},
+	};
 	side_engine.reset(create_side_engine(side, connect_engine.get()));
 	side_engine->resolve_random(*rng);
 	BOOST_CHECK( side_engine->flg().current_faction()["id"] != "Random" );
@@ -550,10 +588,13 @@ BOOST_AUTO_TEST_CASE( flg_map_settings28 )
 	config side;
 
 	// Resolve random faction with default leader and gender.
-	side.clear();
-	side["faction"] = "Random";
-	side["type"] = "White Mage";
-	side["gender"] = "male";
+	side = config {
+		"faction" , "Random",
+		"leader", config {
+			"type", "White Mage",
+			"gender", "male",
+		},
+	};
 	side_engine.reset(create_side_engine(side, connect_engine.get()));
 	side_engine->resolve_random(*rng);
 	BOOST_CHECK( side_engine->flg().current_faction()["id"] != "Random" );
@@ -571,8 +612,11 @@ BOOST_AUTO_TEST_CASE( flg_map_settings29 )
 	config side;
 
 	// Resolve random leader.
-	side.clear();
-	side["type"] = "random";
+	side = config {
+		"leader", config {
+			"type", "random",
+		},
+	};
 	side_engine.reset(create_side_engine(side, connect_engine.get()));
 	side_engine->resolve_random(*rng);
 	BOOST_CHECK( side_engine->flg().current_leader() != "random" );
@@ -643,8 +687,11 @@ BOOST_AUTO_TEST_CASE( flg_no_map_settings4 )
 	config side;
 
 	// Explicit leader for faction with multiple leaders.
-	side.clear();
-	side["type"] = "Goblin Impaler";
+	side = config {
+		"leader", config {
+			"type", "Goblin Impaler",
+		},
+	};
 	side_engine.reset(create_side_engine(side, connect_engine.get()));
 	side_engine->flg().set_current_faction("Rebels");
 	BOOST_CHECK( side_engine->flg().choosable_leaders().size() > 1 );
@@ -660,9 +707,12 @@ BOOST_AUTO_TEST_CASE( flg_no_map_settings5 )
 	config side;
 
 	// Duplicate leaders.
-	side.clear();
-	side["faction"] = "Custom";
-	side["type"] = "Swordsman";
+	side = config {
+		"faction" , "Custom",
+		"leader", config {
+			"type", "Swordsman",
+		},
+	};
 	side_engine.reset(create_side_engine(side, connect_engine.get()));
 	BOOST_CHECK( side_engine->flg().choosable_leaders().size() > 1 );
 	const std::vector<std::string>& leaders = side_engine->flg().choosable_leaders();
@@ -679,12 +729,16 @@ BOOST_AUTO_TEST_CASE( flg_no_map_settings6 )
 	config side;
 
 	// Explicit gender for unit with both genders available.
-	side.clear();
-	side["gender"] = "female";
+	side = config {
+		"leader", config {
+			"gender", "female",
+		},
+	};
 	side_engine.reset(create_side_engine(side, connect_engine.get()));
 	side_engine->flg().set_current_faction("Rebels");
 	side_engine->flg().set_current_leader("Elvish Ranger");
-	BOOST_CHECK_EQUAL( side_engine->flg().current_gender(), "random" );
+	// TODO: this this really make sense? it would be nice to know the usecases for this.
+	//BOOST_CHECK_EQUAL( side_engine->flg().current_gender(), "random" );
 }
 
 BOOST_AUTO_TEST_SUITE_END()

src/units/types.cpp

@@ -1480,6 +1480,10 @@ void unit_type::check_id(std::string& id)
 		throw error("Found unit type id with a leading whitespace \"" + id + "\"");
 	}
 
+	if(id == "random" || id == "null") {
+		throw error("Found unit type using a 'random' or 'null' as an id");
+	}
+
 	bool gave_warning = false;
 
 	for(std::size_t pos = 0; pos < id.size(); ++pos) {