wb: refactor out get_unit() calls

there have been reported many assertion failures due to get_unit()
returning nullptr, so we try not to rely on get_unit() whenever
possible.

src/whiteboard/attack.cpp

@@ -57,7 +57,7 @@ attack::attack(std::size_t team_index, bool hidden, unit& u, const map_location&
 	: move(team_index, hidden, u, route, arrow, std::move(fake_unit)),
 	target_hex_(target_hex),
 	weapon_choice_(weapon_choice),
-	attack_movement_cost_(get_unit()->attacks()[weapon_choice_].movement_used()),
+	attack_movement_cost_(u.attacks()[weapon_choice_].movement_used()),
 	temp_movement_subtracted_(0)
 {
 }
@@ -118,7 +118,7 @@ void attack::execute(bool& success, bool& complete)
 
 	//check that attacking unit is still alive, if not, consider the attack a failure
 	unit_map::const_iterator survivor = resources::gameboard->units().find(get_dest_hex());
-	if(!survivor.valid() || survivor->id() != unit_id_)
+	if(!survivor.valid() || (!unit_id_.empty() && (survivor->id() != unit_id_)))
 	{
 		success = false;
 	}

src/whiteboard/move.cpp

@@ -147,18 +147,14 @@ move::move(const config& cfg, bool hidden)
 	this->init();
 }
 
-void move::init()
+void move::init(unit* u)
 {
-	// If a unit is invalid, return immediately to avoid crashes such as trying to plan a move for a planned recruit.
-	// As per Bug #18637, this should be fixed so that planning moves on planned recruits work properly.
-	// The alternative is to disable movement on planned recruits altogether,
-	// possibly in select_or_action() where the fake unit is selected in the first place.
-	if (get_unit() == nullptr)
-		return;
-
-	assert(get_unit());
-	unit_id_ = get_unit()->id();
-
+	if(get_unit()) {
+		unit_id_ = get_unit()->id();
+	}
+	else if(u) {
+		unit_id_ = u->id();
+	}
 	//This action defines the future position of the unit, make its fake unit more visible
 	//than previous actions' fake units
 	if (fake_unit_)
@@ -166,7 +162,7 @@ void move::init()
 		fake_unit_->anim_comp().set_ghosted(true);
 	}
 	side_actions_ptr side_actions = resources::gameboard->teams().at(team_index()).get_side_actions();
-	side_actions::iterator action = side_actions->find_last_action_of(*(get_unit()));
+	side_actions::iterator action = side_actions->find_last_action_of(unit_underlying_id_);
 	if (action != side_actions->end())
 	{
 		if (move_ptr move = std::dynamic_pointer_cast<class move>(*action))
@@ -176,8 +172,6 @@ void move::init()
 		}
 	}
 
-	this->calculate_move_cost();
-
 	// Initialize arrow_brightness_ and arrow_texture_ using arrow_->style_
 	std::string arrow_style = arrow_->get_style();
 	if(arrow_style == arrow::STYLE_STANDARD)
@@ -250,7 +244,7 @@ void move::execute(bool& success, bool& complete)
 		success = false;
 		complete = true;
 	}
-	else if ( unit_it == resources::gameboard->units().end()  ||  unit_it->id() != unit_id_ )
+	else if ( unit_it == resources::gameboard->units().end()  ||  (unit_id_.empty() && ( unit_it->id() != unit_id_ )))
 	{
 		WRN_WB << "Unit disappeared from map during move execution." << std::endl;
 		success = false;
@@ -313,7 +307,6 @@ map_location move::get_dest_hex() const
 void move::set_route(const pathfind::marked_route& route)
 {
 	route_.reset(new pathfind::marked_route(route));
-	this->calculate_move_cost();
 	arrow_->set_path(route_->steps);
 }
 
@@ -327,7 +320,6 @@ bool move::calculate_new_route(const map_location& source_hex, const map_locatio
 						dest_hex, 10000, path_calc, resources::gameboard->map().w(), resources::gameboard->map().h());
 	if (new_plain_route.move_cost >= path_calc.getNoPathValue()) return false;
 	route_.reset(new pathfind::marked_route(pathfind::mark_route(new_plain_route)));
-	calculate_move_cost();
 	return true;
 }
 
@@ -354,12 +346,11 @@ void move::apply_temp_modifier(unit_map& unit_map)
 	//Modify movement points
 	DBG_WB <<"Move: Changing movement points for unit " << unit->name() << " [" << unit->id()
 			<< "] from " << unit->movement_left() << " to "
-			<< unit->movement_left() - movement_cost_ << ".\n";
+			<< calculate_moves_left(*unit) << ".\n";
 	// Move the unit
 	DBG_WB << "Move: Temporarily moving unit " << unit->name() << " [" << unit->id()
 			<< "] from (" << get_source_hex() << ") to (" << get_dest_hex() <<")\n";
-	mover_.reset(new temporary_unit_mover(unit_map, get_source_hex(), get_dest_hex(),
-	                                      unit->movement_left() - movement_cost_));
+	mover_.reset(new temporary_unit_mover(unit_map, get_source_hex(), get_dest_hex(), calculate_moves_left(*unit)));
 
 	//Update status of fake unit (not undone by remove_temp_modifiers)
 	//@todo this contradicts the name "temp_modifiers"
@@ -382,7 +373,7 @@ void move::remove_temp_modifier(unit_map&)
 		}
 		DBG_WB << "Move: Movement points for unit " << unit->name() << " [" << unit->id()
 					<< "] should get changed from " << unit->movement_left() << " to "
-					<< unit->movement_left() + movement_cost_ << ".\n";
+					<< calculate_moves_left(*unit) << ".\n";
 	}
 
 	// Restore the unit to its original position and movement.
@@ -471,7 +462,7 @@ action::error move::check_validity() const
 
 	//check if the unit in the source hex has the same unit id as before,
 	//i.e. that it's the same unit
-	if(unit_id_ != unit_it->id() || unit_underlying_id_ != unit_it->underlying_id()) {
+	if((!unit_id_.empty() && unit_id_ != unit_it->id()) || unit_underlying_id_ != unit_it->underlying_id()) {
 		return UNIT_CHANGED;
 	}
 
@@ -533,15 +524,14 @@ config move::to_config() const
 	return final_cfg;
 }
 
-void move::calculate_move_cost()
+int move::calculate_moves_left(unit& u)
 {
-	assert(get_unit());
 	assert(route_);
 	if (get_source_hex().valid() && get_dest_hex().valid() && get_source_hex() != get_dest_hex())
 	{
 
 		// @todo: find a better treatment of movement points when defining moves out-of-turn
-		if(get_unit()->movement_left() - route_->move_cost < 0
+		if(u.movement_left() - route_->move_cost < 0
 				&& resources::controller->current_side() == display::get_singleton()->viewing_side()) {
 			WRN_WB << "Move defined with insufficient movement left." << std::endl;
 		}
@@ -549,13 +539,14 @@ void move::calculate_move_cost()
 		// If unit finishes move in a village it captures, set the move cost to unit's movement_left()
 		 if (route_->marks[get_dest_hex()].capture)
 		 {
-			 movement_cost_ = get_unit()->movement_left();
+			 return 0;
 		 }
 		 else
 		 {
-			 movement_cost_ = route_->move_cost;
+			 return u.movement_left() - route_->move_cost;
 		 }
 	}
+	return 0;
 }
 
 void move::redraw()

src/whiteboard/move.hpp

@@ -21,6 +21,7 @@
 #include "action.hpp"
 
 struct temporary_unit_mover;
+class unit;
 
 namespace wb {
 
@@ -95,7 +96,7 @@ protected:
 		return std::static_pointer_cast<move>(action::shared_from_this());
 	}
 
-	void calculate_move_cost();
+	int calculate_moves_left(unit& u);
 
 	std::size_t unit_underlying_id_;
 	std::string unit_id_;
@@ -117,7 +118,7 @@ private:
 	void hide_fake_unit();
 	void show_fake_unit();
 
-	void init();
+	void init(unit* u = nullptr);
 	void update_arrow_style();
 	std::unique_ptr<temporary_unit_mover> mover_;
 	bool fake_unit_hidden_;

src/whiteboard/side_actions.cpp

@@ -570,6 +570,36 @@ side_actions::iterator side_actions::find_first_action_at(map_location hex)
 	return find_first_action_of(actions_.get<container::by_hex>().equal_range(hex), begin(), std::less<iterator>());
 }
 
+side_actions::iterator side_actions::find_first_action_of(size_t unit_id, side_actions::iterator start_position)
+{
+	return find_first_action_of(actions_.get<container::by_unit>().equal_range(unit_id), start_position, std::less<iterator>());
+}
+
+side_actions::const_iterator side_actions::find_last_action_of(size_t unit_id, side_actions::const_iterator start_position) const {
+	return find_first_action_of(actions_.get<container::by_unit>().equal_range(unit_id), start_position, std::greater<iterator>());
+}
+
+side_actions::iterator side_actions::find_last_action_of(size_t unit_id, side_actions::iterator start_position)
+{
+	return find_first_action_of(actions_.get<container::by_unit>().equal_range(unit_id), start_position, std::greater<iterator>());
+}
+
+side_actions::const_iterator side_actions::find_last_action_of(size_t unit_id) const
+{
+	if(end() == begin()) {
+		return end();
+	}
+	return find_last_action_of(unit_id, end() - 1);
+}
+
+side_actions::iterator side_actions::find_last_action_of(size_t unit_id)
+{
+	if(end() == begin()) {
+		return end();
+	}
+	return find_last_action_of(unit_id, end() - 1);
+}
+
 side_actions::iterator side_actions::find_first_action_of(const unit& unit, side_actions::iterator start_position)
 {
 	return find_first_action_of(actions_.get<container::by_unit>().equal_range(unit.underlying_id()), start_position, std::less<iterator>());

src/whiteboard/side_actions.hpp

@@ -465,20 +465,26 @@ public:
 	 * @return The position, or end() if not found.
 	 */
 	iterator find_first_action_of(const unit& unit, iterator start_position);
+	iterator find_first_action_of(size_t unit_id, iterator start_position);
 	/** Variant of this method that always start searching at the beginning of the queue */
 	iterator find_first_action_of(const unit& unit){ return find_first_action_of(unit, begin()); }
+	iterator find_first_action_of(size_t unit_id){ return find_first_action_of(unit_id, begin()); }
 
 	/**
 	 * Finds the last action that belongs to this unit, starting the search backwards from the specified position.
 	 * @return The position, or end() if not found.
 	 */
 	iterator find_last_action_of(const unit& unit, iterator start_position);
+	iterator find_last_action_of(size_t unit_id, iterator start_position);
 	/** const variant of the previous function */
 	const_iterator find_last_action_of(const unit& unit, const_iterator start_position) const;
+	const_iterator find_last_action_of(size_t unit_id, iterator start_position) const;
 	/** Variant of the previous method that always start searching at the end of the queue */
 	iterator find_last_action_of(const unit& unit);
+	iterator find_last_action_of(size_t unit_id);
 	/** const variant of the previous function */
 	const_iterator find_last_action_of(const unit& unit) const;
+	const_iterator find_last_action_of(size_t unit_id) const;
 
 	bool unit_has_actions(const unit& unit);
 	std::size_t count_actions_of(const unit& unit);