Consolidated and improved the validation code for attack and move.

Fixes bug #19613.

src/whiteboard/attack.cpp

@@ -110,7 +110,9 @@ void attack::invalidate()
 void attack::execute(bool& success, bool& complete)
 {
 	if (!valid_) {
-		complete = false;
+		success = false;
+		//Setting complete to true signifies to side_actions to delete the planned action: nothing more to do with it.
+		complete = true;
 		return;
 	}
 
@@ -122,7 +124,8 @@ void attack::execute(bool& success, bool& complete)
 		move::execute(m_success,m_complete);
 		if(!m_success) {
 			//Move failed for some reason, so don't attack.
-			complete = false;
+			success = false;
+			complete = true;
 			return;
 		}
 	}

src/whiteboard/manager.cpp

@@ -950,9 +950,9 @@ bool manager::execute_all_actions()
 
 	while (sa->turn_begin(0) != sa->turn_end(0))
 	{
-		bool action_completed;
+		bool action_successful;
 		try {
-			action_completed = sa->execute(sa->begin());
+			action_successful = sa->execute(sa->begin());
 		} catch (end_level_exception&) { //satisfy the gods of WML
 			executing_all_actions_ = false;
 			throw;
@@ -968,7 +968,7 @@ bool manager::execute_all_actions()
 			return false;
 		}
 		// Interrupt on incomplete action
-		if (!action_completed)
+		if (!action_successful)
 		{
 			executing_all_actions_ = false;
 			return false;

src/whiteboard/move.cpp

@@ -194,7 +194,9 @@ void move::accept(visitor& v)
 void move::execute(bool& success, bool& complete)
 {
 	if (!valid_) {
-		success = complete = false;
+		success = false;
+		//Setting complete to true signifies to side_actions to delete the planned action.
+		complete = true;
 		return;
 	}
 
@@ -204,12 +206,6 @@ void move::execute(bool& success, bool& complete)
 		return;
 	}
 
-	//Ensure destination hex is free
-	if (get_visible_unit(get_dest_hex(),resources::teams->at(viewer_team())) != NULL) {
-		success = complete = false;
-		return;
-	}
-
 	LOG_WB << "Executing: " << shared_from_this() << "\n";
 
 	set_arrow_brightness(ARROW_BRIGHTNESS_HIGHLIGHTED);
@@ -242,7 +238,8 @@ void move::execute(bool& success, bool& complete)
 	if (final_location == route_->steps.front())
 	{
 		LOG_WB << "Move execution resulted in zero movement.\n";
-		success = complete = false;
+		success = false;
+		complete = true;
 	}
 	else if (final_location.valid() &&
 			(unit_it = resources::units->find(final_location)) != resources::units->end()
@@ -266,6 +263,8 @@ void move::execute(bool& success, bool& complete)
 		}
 		else // Move was interrupted, probably by enemy unit sighted
 		{
+			success = false;
+
 			LOG_WB << "Move finished at (" << final_location << ") instead of at (" << get_dest_hex() << "), analyzing\n";
 			std::vector<map_location>::iterator start_new_path;
 			bool found = false;
@@ -285,19 +284,20 @@ void move::execute(bool& success, bool& complete)
 				//FIXME: probably better to use the new calculate_new_route instead of doing this
 				route_->steps = new_path;
 				arrow_->set_path(new_path);
-				success = complete = false;
+				complete = false;
 			}
 			else //Unit ended up in location outside path, likely due to a WML event
 			{
 				WRN_WB << "Unit ended up in location outside path during move execution.\n";
-				success = complete = true;
+				complete = true;
 			}
 		}
 	}
 	else //Unit disappeared from the map, likely due to a WML event
 	{
 		WRN_WB << "Unit disappeared from map during move execution.\n";
-		success = complete = true;
+		success = false;
+		complete = true;
 	}
 
 	if(!complete)

src/whiteboard/side_actions.cpp

@@ -158,9 +158,10 @@ bool side_actions::execute(side_actions::iterator position)
 	}
 
 	bool action_successful;
-	bool should_erase;
+	// Determines whether action should be deleted. Interrupted moves return action_complete == false.
+	bool action_complete;
 	try	{
-		 action->execute(action_successful,should_erase);
+		 action->execute(action_successful,action_complete);
 	} catch (end_turn_exception&) {
 		synced_erase(position);
 		LOG_WB << "End turn exception caught during execution, deleting action. " << *this << "\n";
@@ -174,7 +175,7 @@ bool side_actions::execute(side_actions::iterator position)
 
 	std::stringstream ss;
 	ss << "After " << (action_successful? "successful": "failed") << " execution ";
-	if(should_erase)
+	if(action_complete)
 	{
 		ss << "with deletion, ";
 		synced_erase(position);

src/whiteboard/validate_visitor.cpp

@@ -93,6 +93,11 @@ validate_visitor::VALIDITY validate_visitor::evaluate_move_validity(move_ptr m_p
 	if (m.unit_id_ != unit_it->id() || m.unit_underlying_id_ != unit_it->underlying_id())
 		return WORTHLESS;
 
+	//If the path has at least two hexes (it can have less with the attack subclass), ensure destination hex is free
+	if (m.get_route().steps.size() >= 2 && get_visible_unit(m.get_dest_hex(),resources::teams->at(viewer_team())) != NULL) {
+		return WORTHLESS;
+	}
+
 	//check that the path is good
 	if (m.get_source_hex() != m.get_dest_hex()) //skip zero-hex move used by attack subclass
 	{
@@ -160,26 +165,35 @@ void validate_visitor::visit(attack_ptr attack)
 	resources::screen->invalidate(attack->get_dest_hex());
 	resources::screen->invalidate(attack->target_hex_);
 
-	//Verify that the target hex is still valid
-	//and Verify that the target hex isn't empty
-	if (!attack->target_hex_.valid()
-			|| resources::units->find(attack->target_hex_) == resources::units->end())
+	if  (
+			// Verify that the unit that planned this attack exists
+			attack->get_unit()
+			// Verify that the target hex is still valid
+			&& attack->target_hex_.valid()
+			// Verify that the target hex isn't empty
+			&& resources::units->find(attack->target_hex_) != resources::units->end()
+			// Verify that the attacking unit has attacks left
+			&& attack->get_unit()->attacks_left() > 0
+			// Verify that the attacker and target are enemies
+			&& (*resources::teams)[attack->get_unit()->side() - 1].is_enemy(resources::units->find(attack->target_hex_)->side())
+
+			//@todo: (maybe) verify that the target hex contains the same unit that before,
+			// comparing for example the unit ID
+		)
 	{
-		if(viewer_team() == attack->team_index()) //< Don't mess with any other team's queue -- only our own
+		//All checks pass, so call the visitor on the superclass
+		visit(boost::static_pointer_cast<move>(attack));
+	}
+	else
+	{
+		attack->set_valid(false);
+
+		if (viewer_team() == attack->team_index()) //< Don't mess with any other team's queue -- only our own
 		{
 			LOG_WB << "Worthless invalid attack detected, adding to actions_to_erase_.\n";
 			actions_to_erase_.insert(attack);
 		}
 	}
-	else //All checks pass, so call the visitor on the superclass
-	{
-		//@todo: verify that the target hex contains the same unit that before,
-		// comparing for example the unit ID
-
-		//@todo: Verify that the target unit is our enemy
-
-		visit(boost::static_pointer_cast<move>(attack));
-	}
 }
 
 void validate_visitor::visit(recruit_ptr recruit)