Don't assume unit is on the map when testing ability active

This refactors ability tests to use a passed-in unit instead of
searching for the unit on the map based on its location.

While it does fix #2247, I am not confident that this won't also
have some undesirable side-effects.

src/actions/attack.cpp

@@ -122,10 +122,10 @@ battle_context_unit_stats::battle_context_unit_stats(const unit& u,
 	}
 
 	// Get the weapon characteristics as appropriate.
-	weapon->set_specials_context(u_loc, opp_loc, attacking, opp_weapon);
+	weapon->set_specials_context(u, opp, u_loc, opp_loc, attacking, opp_weapon);
 
 	if(opp_weapon) {
-		opp_weapon->set_specials_context(opp_loc, u_loc, !attacking, weapon);
+		opp_weapon->set_specials_context(u, opp, opp_loc, u_loc, !attacking, weapon);
 	}
 
 	slows = weapon->get_special_bool("slow");
@@ -278,10 +278,10 @@ battle_context_unit_stats::battle_context_unit_stats(const unit_type* u_type,
 	}
 
 	// Get the weapon characteristics as appropriate.
-	weapon->set_specials_context(map_location::null_location(), attacking);
+	weapon->set_specials_context(u_type, map_location::null_location(), attacking);
 
 	if(opp_weapon) {
-		opp_weapon->set_specials_context(map_location::null_location(), !attacking);
+		opp_weapon->set_specials_context(u_type, map_location::null_location(), !attacking);
 	}
 
 	slows = weapon->get_special_bool("slow");

src/gui/dialogs/attack_predictions.cpp

@@ -127,7 +127,7 @@ void attack_predictions::set_data(window& window, const combatant_data& attacker
 
 	// Set specials context (for safety, it should not have changed normally).
 	const_attack_ptr weapon = attacker.stats_.weapon;
-	weapon->set_specials_context(attacker.unit_.get_location(), defender.unit_.get_location(), attacker.stats_.is_attacker, defender.stats_.weapon);
+	weapon->set_specials_context(attacker.unit_, defender.unit_, attacker.unit_.get_location(), defender.unit_.get_location(), attacker.stats_.is_attacker, defender.stats_.weapon);
 
 	// Get damage modifiers.
 	unit_ability_list dmg_specials = weapon->get_specials("damage");

src/reports.cpp

@@ -658,7 +658,7 @@ static int attack_info(reports::context & rc, const attack_type &at, config &res
 {
 	std::ostringstream str, tooltip;
 
-	at.set_specials_context(displayed_unit_hex, u.side() == rc.screen().playing_side());
+	at.set_specials_context(u, displayed_unit_hex, u.side() == rc.screen().playing_side());
 	int base_damage = at.damage();
 	int specials_damage = at.modified_damage(false);
 	int damage_multiplier = 100;

src/units/abilities.cpp

@@ -46,19 +46,19 @@ namespace {
 	class temporary_facing
 	{
 		map_location::DIRECTION save_dir_;
-		unit_map::const_iterator u_;
+		const unit* u_;
 	public:
-		temporary_facing(unit_map::const_iterator u, map_location::DIRECTION new_dir)
-			: save_dir_(u.valid() ? u->facing() : map_location::NDIRECTIONS)
+		temporary_facing(const unit* u, map_location::DIRECTION new_dir)
+			: save_dir_(u ? u->facing() : map_location::NDIRECTIONS)
 			, u_(u)
 		{
-			if (u_.valid()) {
+			if (u_) {
 				u_->set_facing(new_dir);
 			}
 		}
 		~temporary_facing()
 		{
-			if (u_.valid()) {
+			if (u_) {
 				u_->set_facing(save_dir_);
 			}
 		}
@@ -696,16 +696,22 @@ std::string attack_type::weapon_specials(bool only_active, bool is_backstab) con
 /**
  * Sets the context under which specials will be checked for being active.
  * This version is appropriate if both units in a combat are known.
+ * @param[in]  self          A reference to the unit with this weapon.
+ * @param[in]  other         A reference to the other unit in the combat.
  * @param[in]  unit_loc      The location of the unit with this weapon.
  * @param[in]  other_loc     The location of the other unit in the combat.
  * @param[in]  attacking     Whether or not the unit with this weapon is the attacker.
  * @param[in]  other_attack  The attack used by the other unit.
  */
-void attack_type::set_specials_context(const map_location& unit_loc,
+void attack_type::set_specials_context(const unit& self,
+                                       const unit& other,
+                                       const map_location& unit_loc,
                                        const map_location& other_loc,
                                        bool attacking,
                                        const_attack_ptr other_attack) const
 {
+	self_ = &self;
+	other_ = &other;
 	self_loc_ = unit_loc;
 	other_loc_ = other_loc;
 	is_attacker_ = attacking;
@@ -716,11 +722,33 @@ void attack_type::set_specials_context(const map_location& unit_loc,
 /**
  * Sets the context under which specials will be checked for being active.
  * This version is appropriate if there is no specific combat being considered.
+ * @param[in]  self          A reference to the unit with this weapon.
  * @param[in]  loc           The location of the unit with this weapon.
  * @param[in]  attacking     Whether or not the unit with this weapon is the attacker.
  */
-void attack_type::set_specials_context(const map_location& loc, bool attacking) const
+void attack_type::set_specials_context(const unit& self, const map_location& loc, bool attacking) const
 {
+	self_ = &self;
+	other_ = nullptr;
+	self_loc_ = loc;
+	other_loc_ = map_location::null_location();
+	is_attacker_ = attacking;
+	other_attack_ = nullptr;
+	is_for_listing_ = false;
+}
+
+/**
+ * Sets the context under which specials will be checked for being active.
+ * This version is appropriate for theoretical units of a particular type.
+ * @param[in]  self_type     A reference to the type of the unit with this weapon.
+ * @param[in]  loc           The location of the unit with this weapon.
+ * @param[in]  attacking     Whether or not the unit with this weapon is the attacker.
+ */
+void attack_type::set_specials_context(const unit_type* self_type, const map_location& loc, bool attacking) const
+{
+	UNUSED(self_type);
+	self_ = nullptr;
+	other_ = nullptr;
 	self_loc_ = loc;
 	other_loc_ = map_location::null_location();
 	is_attacker_ = attacking;
@@ -830,8 +858,8 @@ namespace { // Helpers for attack_type::special_active()
 	 * @param[in]  filter     The filter containing the child filter to use.
 	 * @param[in]  child_tag  The tag of the child filter to use.
 	 */
-	static bool special_unit_matches(const unit_map::const_iterator & un_it,
-									 const unit_map::const_iterator & u2,
+	static bool special_unit_matches(const unit* & u,
+		                             const unit* & u2,
 		                             const map_location & loc,
 		                             const_attack_ptr weapon,
 		                             const config & filter,
@@ -852,19 +880,19 @@ namespace { // Helpers for attack_type::special_active()
 			return true;
 
 		// If the primary unit doesn't exist, there's nothing to match
-		if (!un_it.valid()) {
+		if (!u) {
 			return false;
 		}
 
 		unit_filter ufilt{vconfig(filter_child)};
 
 		// If the other unit doesn't exist, try matching without it
-		if (!u2.valid()) {
-			return ufilt.matches(*un_it, loc);
+		if (!u2) {
+			return ufilt.matches(*u, loc);
 		}
 
 		// Check for a unit match.
-		if (!ufilt.matches(*un_it, loc, *u2)) {
+		if (!ufilt.matches(*u, loc, *u2)) {
 			return false;
 		}
 
@@ -922,16 +950,29 @@ bool attack_type::special_active(const config& special, AFFECTS whom,
 	assert(display::get_singleton());
 	const unit_map& units = display::get_singleton()->get_units();
 
-	unit_map::const_iterator self  = units.find(self_loc_);
-	unit_map::const_iterator other = units.find(other_loc_);
+	const unit* self = self_;
+	const unit* other = other_;
+
+	if(self == nullptr) {
+		unit_map::const_iterator it = units.find(self_loc_);
+		if(it.valid()) {
+			self = it.get_shared_ptr().get();
+		}
+	}
+	if(other == nullptr) {
+		unit_map::const_iterator it = units.find(other_loc_);
+		if(it.valid()) {
+			other = it.get_shared_ptr().get();
+		}
+	}
 
 	// Make sure they're facing each other.
 	temporary_facing self_facing(self, self_loc_.get_relative_dir(other_loc_));
 	temporary_facing other_facing(other, other_loc_.get_relative_dir(self_loc_));
 
 	// Translate our context into terms of "attacker" and "defender".
-	unit_map::const_iterator & att = is_attacker_ ? self : other;
-	unit_map::const_iterator & def = is_attacker_ ? other : self;
+	const unit* & att = is_attacker_ ? self : other;
+	const unit* & def = is_attacker_ ? other : self;
 	const map_location & att_loc   = is_attacker_ ? self_loc_ : other_loc_;
 	const map_location & def_loc   = is_attacker_ ? other_loc_ : self_loc_;
 	const_attack_ptr att_weapon = is_attacker_ ? shared_from_this() : other_attack_;

src/units/attack_type.hpp

@@ -28,6 +28,7 @@
 #include "units/ptr.hpp" // for attack_ptr
 
 class unit_ability_list;
+class unit_type;
 
 //the 'attack type' is the type of attack, how many times it strikes,
 //and how much damage it does.
@@ -72,9 +73,11 @@ class attack_type : public std::enable_shared_from_this<attack_type>
 	unit_ability_list get_specials(const std::string& special) const;
 	std::vector<std::pair<t_string, t_string>> special_tooltips(boost::dynamic_bitset<>* active_list = nullptr) const;
 	std::string weapon_specials(bool only_active=false, bool is_backstab=false) const;
-	void set_specials_context(const map_location& unit_loc, const map_location& other_loc,
+	void set_specials_context(const unit& self, const unit& other,
+	                          const map_location& unit_loc, const map_location& other_loc,
 	                          bool attacking, const_attack_ptr other_attack) const;
-	void set_specials_context(const map_location& loc, bool attacking = true) const;
+	void set_specials_context(const unit& self, const map_location& loc, bool attacking = true) const;
+	void set_specials_context(const unit_type* self_type, const map_location& loc, bool attacking = true) const;
 	void set_specials_context_for_listing() const;
 
 	/// Calculates the number of attacks this weapon has, considering specials.
@@ -106,6 +109,8 @@ class attack_type : public std::enable_shared_from_this<attack_type>
 	// Used via set_specials_context() to control which specials are
 	// considered active.
 	mutable map_location self_loc_, other_loc_;
+	mutable const unit* self_;
+	mutable const unit* other_;
 	mutable bool is_attacker_;
 	mutable const_attack_ptr other_attack_;
 	mutable bool is_for_listing_ = false;