split villagecapture into seperate undo action

this wayits possibel to undo the action exactly in the reverse order
as they are a happening on the original code, even with wml events involved

src/actions/create.cpp

@@ -626,6 +626,7 @@ place_recruit_result place_recruit(unit_ptr u, const map_location &recruit_locat
 	if (full_movement) {
 		u->set_movement(u->total_movement(), true);
 	} else {
+		//TODO: it looks to me like this change of unit stats is not properly undone yet.
 		u->set_movement(0, true);
 		u->set_attacks(0);
 	}
@@ -721,10 +722,11 @@ void recruit_unit(const unit_type & u_type, int side_num, const map_location & l
 
 
 	// Place the recruit.
-	place_recruit_result res = place_recruit(new_unit, loc, from, u_type.cost(), false, map_location::direction::indeterminate, show);
+	resources::undo_stack->add_recruit(new_unit, loc, from);
+	place_recruit_result res
+		= place_recruit(new_unit, loc, from, u_type.cost(), false, map_location::direction::indeterminate, show);
 	resources::controller->statistics().recruit_unit(*new_unit);
 
-	resources::undo_stack->add_recruit(new_unit, loc, from, std::get<1>(res), std::get<2>(res));
 	// Check for information uncovered or randomness used.
 
 	synced_context::block_undo(std::get<0>(res));
@@ -745,15 +747,16 @@ bool recall_unit(const std::string & id, team & current_team,
 	if ( !recall )
 		return false;
 
+	resources::undo_stack->add_recall(recall, loc, from);
 	// Place the recall.
 	// We also check to see if a custom unit level recall has been set if not,
 	// we use the team's recall cost otherwise the unit's.
 	int cost = recall->recall_cost() >= 0 ? recall->recall_cost() : current_team.recall_cost();
+
 	place_recruit_result res = place_recruit(recall, loc, from, cost,
 	                             true, facing, show);
-	resources::controller->statistics().recall_unit(*recall);
 
-	resources::undo_stack->add_recall(recall, loc, from, std::get<1>(res), std::get<2>(res));
+	resources::controller->statistics().recall_unit(*recall);
 	synced_context::block_undo(std::get<0>(res));
 
 
@@ -765,5 +768,4 @@ bool recall_unit(const std::string & id, team & current_team,
 	return true;
 }
 
-
 }//namespace actions

src/actions/move.cpp

@@ -159,7 +159,63 @@ void move_unit_spectator::set_unit(const unit_map::const_iterator &u)
 	unit_ = u;
 }
 
+namespace {
 
+/**
+ * The number of the side that preivously owned the village that the unit stepped on
+ * Note, that recruit/recall actions can also take a village if the unit was recruits/recalled onto a village
+ */
+struct take_village_step : undo_action
+{
+	int original_village_owner;
+	bool take_village_timebonus;
+	map_location loc;
+
+	take_village_step(
+		int orig_village_owner, bool time_bonus, map_location loc)
+		: original_village_owner(orig_village_owner)
+		, take_village_timebonus(time_bonus)
+		, loc(loc)
+	{
+	}
+
+	take_village_step(const config& cfg)
+		: original_village_owner(cfg["village_owner"].to_int())
+		, take_village_timebonus(cfg["village_timebonus"].to_bool())
+		, loc(cfg)
+	{
+	}
+
+	static const char* get_type_impl() { return "take_village"; }
+	virtual const char* get_type() const { return get_type_impl(); }
+
+	virtual ~take_village_step() { }
+
+	/** Writes this into the provided config. */
+	virtual void write(config& cfg) const
+	{
+		undo_action::write(cfg);
+		cfg["village_owner"] = original_village_owner;
+		cfg["village_timebonus"] = take_village_timebonus;
+		loc.write(cfg);
+	}
+
+	/** Undoes this action. */
+	virtual bool undo(int)
+	{
+		team& current_team = resources::controller->current_team();
+		if(resources::gameboard->map().is_village(loc)) {
+			get_village(loc, original_village_owner, nullptr, false);
+			// MP_COUNTDOWN take away capture bonus
+			if(take_village_timebonus) {
+				current_team.set_action_bonus_count(current_team.action_bonus_count() - 1);
+			}
+		}
+		return true;
+	}
+};
+
+}
 game_events::pump_result_t get_village(const map_location& loc, int side, bool *action_timebonus, bool fire_event)
 {
 	std::vector<team> &teams = resources::gameboard->teams();
@@ -201,6 +257,7 @@ game_events::pump_result_t get_village(const map_location& loc, int side, bool *
 		if (display::get_singleton() != nullptr) {
 			display::get_singleton()->invalidate(loc);
 		}
+		resources::undo_stack->add_custom<take_village_step>(old_owner_side, grants_timebonus, loc);
 		return t->get_village(loc, old_owner_side, fire_event ? resources::gamedata : nullptr);
 	}
 
@@ -1183,7 +1240,7 @@ namespace { // Private helpers for move_unit()
 				// MP_COUNTDOWN: added param
 				undo_stack->add_move(
 					move_it_.get_shared_ptr(), begin_, real_end_, orig_moves_,
-					action_time_bonus, orig_village_owner, orig_dir_);
+					orig_dir_);
 			}
 
 			if ( !mover_valid  ||  undo_blocked()  ||

src/actions/shroud_clearing_action.cpp

@@ -22,28 +22,4 @@
 #include "play_controller.hpp"
 
 namespace actions {
-void shroud_clearing_action::return_village()
-{
-	team &current_team = resources::controller->current_team();
-	const map_location back = route.back();
-	if(resources::gameboard->map().is_village(back)) {
-		get_village(back, original_village_owner, nullptr, false);
-		//MP_COUNTDOWN take away capture bonus
-		if(take_village_timebonus) {
-			current_team.set_action_bonus_count(current_team.action_bonus_count() - 1);
-		}
-	}
-}
-void shroud_clearing_action::take_village()
-{
-	team &current_team = resources::controller->current_team();
-	const map_location back = route.back();
-	if(resources::gameboard->map().is_village(back)) {
-		get_village(back, current_team.side(), nullptr, false);
-		//MP_COUNTDOWN restore capture bonus
-		if(take_village_timebonus) {
-			current_team.set_action_bonus_count(1 + current_team.action_bonus_count());
-		}
-	}
-}
 }

src/actions/shroud_clearing_action.hpp

@@ -27,28 +27,22 @@ struct shroud_clearing_action
 	shroud_clearing_action(const config& cfg)
 		: route()
 		, view_info(cfg.child_or_empty("unit"))
-		, original_village_owner(cfg["village_owner"].to_int())
-		, take_village_timebonus(cfg["village_timebonus"].to_bool())
 	{
 		read_locations(cfg, route);
 	}
 
-	shroud_clearing_action(const unit_const_ptr u, const map_location& loc, int village_owner, bool village_bonus)
+	shroud_clearing_action(const unit_const_ptr u, const map_location& loc)
 		: route(1, loc)
 		, view_info(*u)
-		, original_village_owner(village_owner)
-		, take_village_timebonus(village_bonus)
 	{
 
 	}
 
 	typedef std::vector<map_location> route_t;
 
-	shroud_clearing_action(const unit_const_ptr u, const route_t::const_iterator& begin, const route_t::const_iterator& end, int village_owner, bool village_bonus)
+	shroud_clearing_action(const unit_const_ptr u, const route_t::const_iterator& begin, const route_t::const_iterator& end)
 		: route(begin, end)
 		, view_info(*u)
-		, original_village_owner(village_owner)
-		, take_village_timebonus(village_bonus)
 	{
 
 	}
@@ -60,25 +54,11 @@ struct shroud_clearing_action
 	route_t route;
 	/** A record of the affected unit's ability to see. */
 	clearer_info view_info;
-	/**
-	 * The number of the side that preivously owned the village that the unit stepped on
-	 * Note, that recruit/recall actions can also take a village if the unit was recruits/recalled onto a village
-	 */
-	int original_village_owner;
-	/** Whether this actions got a timebonus because it took a village. */
-	bool take_village_timebonus;
-
-	/** Change village owner on undo. */
-	void return_village();
-	/** Change village owner on redo. */
-	void take_village();
 
 	void write(config & cfg) const
 	{
 		write_locations(route, cfg);
 		view_info.write(cfg.add_child("unit"));
-		cfg["village_owner"] = original_village_owner;
-		cfg["village_timebonus"] = take_village_timebonus;
 	}
 
 	virtual ~shroud_clearing_action() {}

src/actions/undo.cpp

@@ -96,28 +96,28 @@ void undo_list::add_dismissal(const unit_const_ptr u)
 void undo_list::add_move(const unit_const_ptr u,
                          const std::vector<map_location>::const_iterator & begin,
                          const std::vector<map_location>::const_iterator & end,
-                         int start_moves, int timebonus, int village_owner,
+                         int start_moves,
                          const map_location::direction dir)
 {
-	add(std::make_unique<undo::move_action>(u, begin, end, start_moves, timebonus, village_owner, dir));
+	add(std::make_unique<undo::move_action>(u, begin, end, start_moves, dir));
 }
 
 /**
  * Adds a recall to the undo stack.
  */
 void undo_list::add_recall(const unit_const_ptr u, const map_location& loc,
-                           const map_location& from, int orig_village_owner, bool time_bonus)
+                           const map_location& from)
 {
-	add(std::make_unique<undo::recall_action>(u, loc, from, orig_village_owner, time_bonus));
+	add(std::make_unique<undo::recall_action>(u, loc, from));
 }
 
 /**
  * Adds a recruit to the undo stack.
  */
 void undo_list::add_recruit(const unit_const_ptr u, const map_location& loc,
-                            const map_location& from, int orig_village_owner, bool time_bonus)
+                            const map_location& from)
 {
-	add(std::make_unique<undo::recruit_action>(u, loc, from, orig_village_owner, time_bonus));
+	add(std::make_unique<undo::recruit_action>(u, loc, from));
 }
 
 

src/actions/undo.hpp

@@ -54,14 +54,14 @@ public:
 	void add_move(const unit_const_ptr u,
 	              const std::vector<map_location>::const_iterator & begin,
 	              const std::vector<map_location>::const_iterator & end,
-	              int start_moves, int timebonus=0, int village_owner=-1,
+	              int start_moves,
 	              const map_location::direction dir=map_location::direction::indeterminate);
 	/** Adds a recall to the undo stack. */
 	void add_recall(const unit_const_ptr u, const map_location& loc,
-	                const map_location& from, int orig_village_owner, bool time_bonus);
+	                const map_location& from);
 	/** Adds a recruit to the undo stack. */
 	void add_recruit(const unit_const_ptr u, const map_location& loc,
-	                 const map_location& from, int orig_village_owner, bool time_bonus);
+	                 const map_location& from);
 
 	template<class T, class... Args>
 	void add_custom(Args&&... args)

src/actions/undo_move_action.cpp

@@ -32,9 +32,9 @@ namespace actions::undo
 move_action::move_action(const unit_const_ptr moved,
 			const std::vector<map_location>::const_iterator & begin,
 			const std::vector<map_location>::const_iterator & end,
-			int sm, int timebonus, int orig, const map_location::direction dir)
+			int sm, const map_location::direction dir)
 	: undo_action()
-	, shroud_clearing_action(moved, begin, end, orig, timebonus != 0)
+	, shroud_clearing_action(moved, begin, end)
 	, starting_moves(sm)
 	, starting_dir(dir == map_location::direction::indeterminate ? moved->facing() : dir)
 	, goto_hex(moved->get_goto())
@@ -91,12 +91,11 @@ bool move_action::undo(int)
 	// Check units.
 	unit_map::iterator u = units.find(rev_route.front());
 	const unit_map::iterator u_end = units.find(rev_route.back());
-	if ( u == units.end()  ||  u_end != units.end() ) {
-		//this can actually happen if the scenario designer has abused the [allow_undo] command
+	if(u == units.end() || u_end != units.end()) {
+		// this can actually happen if the scenario designer has abused the [allow_undo] command
 		ERR_NG << "Illegal 'undo' found. Possible abuse of [allow_undo]?";
 		return false;
 	}
-	this->return_village();
 
 	// Record the unit's current state so it can be redone.
 	starting_moves = u->movement_left();

src/actions/undo_move_action.hpp

@@ -30,7 +30,7 @@ struct move_action : undo_action, shroud_clearing_action
 	move_action(const unit_const_ptr moved,
 	            const std::vector<map_location>::const_iterator & begin,
 	            const std::vector<map_location>::const_iterator & end,
-	            int sm, int timebonus, int orig, const map_location::direction dir);
+	            int sm, const map_location::direction dir);
 	move_action(const config & cfg)
 		: undo_action()
 		, shroud_clearing_action(cfg)

src/actions/undo_recall_action.cpp

@@ -33,9 +33,9 @@ static lg::log_domain log_engine("engine");
 namespace actions::undo
 {
 recall_action::recall_action(const unit_const_ptr recalled, const map_location& loc,
-			  const map_location& from, int orig_village_owner, bool time_bonus)
+			  const map_location& from)
 	: undo_action()
-	, shroud_clearing_action(recalled, loc, orig_village_owner, time_bonus)
+	, shroud_clearing_action(recalled, loc)
 	, id(recalled->id())
 	, recall_from(from)
 {}
@@ -97,7 +97,6 @@ bool recall_action::undo(int side)
 	units.erase(recall_loc);
 	resources::whiteboard->on_kill_unit();
 	un->anim_comp().clear_haloes();
-	this->return_village();
 	return true;
 }
 static auto reg_undo_recall = undo_action_container::subaction_factory<recall_action>();

src/actions/undo_recall_action.hpp

@@ -27,7 +27,7 @@ struct recall_action : undo_action, shroud_clearing_action
 
 
 	recall_action(const unit_const_ptr recalled, const map_location& loc,
-				  const map_location& from, int orig_village_owner, bool time_bonus);
+				  const map_location& from);
 	recall_action(const config & cfg);
 
 	static const char* get_type_impl() { return "recall"; }

src/actions/undo_recruit_action.cpp

@@ -32,9 +32,9 @@ static lg::log_domain log_engine("engine");
 namespace actions::undo
 {
 recruit_action::recruit_action(const unit_const_ptr recruited, const map_location& loc,
-			   const map_location& from, int orig_village_owner, bool time_bonus)
+			   const map_location& from)
 	: undo_action()
-	, shroud_clearing_action(recruited, loc, orig_village_owner, time_bonus)
+	, shroud_clearing_action(recruited, loc)
 	, u_type(recruited->type())
 	, recruit_from(from)
 {}
@@ -98,7 +98,6 @@ bool recruit_action::undo(int side)
 	// to also do the overlapped hexes
 	gui.invalidate(recruit_loc);
 	units.erase(recruit_loc);
-	this->return_village();
 	return true;
 }
 static auto reg_undo_recruit = undo_action_container::subaction_factory<recruit_action>();

src/actions/undo_recruit_action.hpp

@@ -31,7 +31,7 @@ struct recruit_action : undo_action, shroud_clearing_action
 
 
 	recruit_action(const unit_const_ptr recruited, const map_location& loc,
-	               const map_location& from, int orig_village_owner, bool time_bonus);
+	               const map_location& from);
 	recruit_action(const config & cfg);
 
 	static const char* get_type_impl() { return "recruit"; }