Fix research sync toggle and classic medikit crash

src/Battlescape/BattlescapeState.cpp

@@ -2325,78 +2325,55 @@ void BattlescapeState::coopHealing(int actor_id, int type, int part, std::string
 	if (!unit)
 		return;
 
-	// coop (PRD-P1 rule, applied here by PRD-P6): replay on a stack-local action
-	// carrying the HEALER's own medikit. The classic branch below runs on
-	// _currentAction, whose weapon is whatever the receiving player happens to be
-	// holding - in parallel mode that is the other player's business entirely,
-	// and medikitUse() dereferences it.
-	if (connectionTCP::parallelTurnActive())
+	// Replay every peer medikit use on a stack-local action carrying the HEALER's
+	// own medikit.  The old classic-turn path reused this machine's currentAction
+	// and set actor/type/time but never weapon; medikitUse() then dereferenced a
+	// null (or, worse, unrelated local) weapon.  Modern packets identify both the
+	// healer and item, so classic and parallel turns can share the safe path.
+	BattleUnit* healer = healer_id == -1 ? unit : nullptr;
+	if (healer_id != -1)
 	{
-		BattleUnit* healer = unit;
-		if (healer_id != -1)
+		for (auto u : *_save->getUnits())
 		{
-			for (auto u : *_save->getUnits())
+			if (u->getId() == healer_id)
 			{
-				if (u->getId() == healer_id)
-				{
-					healer = u;
-					break;
-				}
+				healer = u;
+				break;
 			}
 		}
+	}
 
-		BattleAction action = BattlescapeGame::makeReplayAction(healer);
-		action.type = (BattleActionType)type;
-		action.Time = time;
-		action.weapon = BattlescapeGame::coopResolveWeapon(_save, healer, weapon_id, weapon_type, hand);
-		if (!action.weapon)
-		{
-			Log(LOG_INFO) << "coop: medkit replay skipped, healer " << healer->getId()
-						  << " has no '" << weapon_type << "' (id " << weapon_id << ")";
-			return;
-		}
-
-		BattleMediKitAction mode = medkit_state == "stimulant" ? BMA_STIMULANT
-								 : medkit_state == "painkiller" ? BMA_PAINKILLER
-																: BMA_HEAL;
-		UnitBodyPart bodyPart = mode == BMA_HEAL ? (UnitBodyPart)part : BODYPART_TORSO;
-		_battleGame->getTileEngine()->medikitUse(&action, unit, mode, bodyPart);
-		// both machines now hold the same charge counts, so this reaches the same
-		// answer on both and the item census stays equal.
-		_battleGame->getTileEngine()->medikitRemoveIfEmpty(&action);
-		updateSoldierInfo();
+	if (!healer)
+	{
+		Log(LOG_WARNING) << "coop: medkit replay skipped, healer " << healer_id << " was not found";
 		return;
 	}
 
-	_save->setSelectedUnit(unit);
-	_battleGame->getCurrentAction()->actor = unit;
-
-	_battleGame->getCurrentAction()->type = (BattleActionType)type;
-
-	_battleGame->getCurrentAction()->Time = time;
-
-		if (medkit_state == "heal")
-		{
-
-			_battleGame->getTileEngine()->medikitUse(_battleGame->getCurrentAction(), unit, BMA_HEAL, (UnitBodyPart)part);
-
-		}
-
-		if (medkit_state == "stimulant")
-		{
-
-			_battleGame->getTileEngine()->medikitUse(_battleGame->getCurrentAction(), unit, BMA_STIMULANT, BODYPART_TORSO);
-
-		}
-
-		if (medkit_state == "painkiller")
-		{
-
-			_battleGame->getTileEngine()->medikitUse(_battleGame->getCurrentAction(), unit, BMA_PAINKILLER, BODYPART_TORSO);
-
-		}
+	BattleAction action = BattlescapeGame::makeReplayAction(healer);
+	action.type = (BattleActionType)type;
+	action.Time = time;
+	action.weapon = BattlescapeGame::coopResolveWeapon(_save, healer, weapon_id, weapon_type, hand);
+	if (!action.weapon || !action.weapon->getRules()
+		|| action.weapon->getRules()->getBattleType() != BT_MEDIKIT)
+	{
+		Log(LOG_WARNING) << "coop: medkit replay skipped, healer " << healer->getId()
+						 << " has no '" << weapon_type << "' (id " << weapon_id << ")";
+		return;
+	}
 
-
+	BattleMediKitAction mode = medkit_state == "stimulant" ? BMA_STIMULANT
+							 : medkit_state == "painkiller" ? BMA_PAINKILLER
+													: BMA_HEAL;
+	UnitBodyPart bodyPart = BODYPART_TORSO;
+	if (mode == BMA_HEAL && part >= 0 && part < BODYPART_MAX)
+	{
+		bodyPart = (UnitBodyPart)part;
+	}
+	_battleGame->getTileEngine()->medikitUse(&action, unit, mode, bodyPart);
+	// Both machines now hold the same charge counts, so this reaches the same
+	// answer on both and the item census stays equal.
+	_battleGame->getTileEngine()->medikitRemoveIfEmpty(&action);
+	updateSoldierInfo();
 }
 
 void BattlescapeState::coopActiveGranade(int actor_id, int type, std::string hand, int fusetimer, int item_id)

src/Battlescape/ExplosionBState.cpp

@@ -556,7 +556,7 @@ void ExplosionBState::think()
 		// terrain leaked onto the pacing path again.
 		if (_explosionCounter > 0) ++g_coopTerrainPacingConsumes;
 		_coopTaskCompleted = false;
-		_parent->getCoopMod()->_coopPacingWait = false;
+		_parent->getCoopMod()->_coopPacingWait = false; 
 		_parent->popState();
 		return;
 	}

src/Battlescape/TileEngine.cpp

@@ -33,6 +33,7 @@
 #include "../Savegame/HitLog.h"
 #include "../Engine/RNG.h"
 #include "../Engine/GraphSubset.h"
+#include "../Engine/Logger.h"
 #include "BattlescapeState.h"
 #include "../Mod/MapDataSet.h"
 #include "../Mod/Unit.h"
@@ -5526,6 +5527,12 @@ void TileEngine::medikitRemoveIfEmpty(BattleAction *action)
 
 bool TileEngine::medikitUse(BattleAction *action, BattleUnit *target, BattleMediKitAction originalMedikitAction, UnitBodyPart bodyPart)
 {
+	if (!action || !target || !action->weapon || !action->weapon->getRules())
+	{
+		Log(LOG_WARNING) << "medikitUse: rejected action with missing target, weapon or rules";
+		return false;
+	}
+
 	if (_save->isPreview())
 	{
 		return false;

src/CoopMod/OptionsMultiplayer.cpp

@@ -26,6 +26,7 @@
 #include "../Interface/Window.h"
 #include "../Mod/Mod.h"
 #include "../Mod/RuleInterface.h"
+#include "../CoopMod/connectionTCP.h"
 #include "../CoopMod/OptionsMultiplayer.h"
 #include <algorithm>
 #include <sstream>
@@ -421,8 +422,34 @@ void OptionsMultiplayerState::lstOptionsClick(Action* action)
 	if (setting->type() == OPTION_BOOL)
 	{
 		bool* b = setting->asBool();
+		// Research sync is host-authoritative while connected.  The client sees
+		// the negotiated value but must not silently override it locally.
+		if (b == &Options::EnableResearchSync
+			&& _game->getCoopMod()->getCoopStatic()
+			&& !_game->getCoopMod()->getServerOwner())
+		{
+			return;
+		}
 		*b = !*b;
 		settingText = *b ? tr("STR_YES") : tr("STR_NO");
+		if (b == &Options::EnableResearchSync && _game->getCoopMod())
+		{
+			// Make an in-session change effective immediately on this peer.  The
+			// handshake already distributes the host's initial value to the client.
+			_game->getCoopMod()->_enable_research_sync = *b;
+			if (!*b)
+			{
+				_game->getCoopMod()->waitedResearch.clear();
+			}
+			if (_game->getCoopMod()->getCoopStatic()
+				&& _game->getCoopMod()->getServerOwner())
+			{
+				Json::Value root;
+				root["state"] = "research_sync_option";
+				root["enabled"] = *b;
+				_game->getCoopMod()->sendTCPPacketData(root.toStyledString());
+			}
+		}
 		if (b == &Options::lazyLoadResources && !*b)
 		{
 			Options::reload = true; // reload when turning lazy loading off

src/CoopMod/connectionTCP.cpp

@@ -7362,9 +7362,24 @@ void connectionTCP::onTCPMessage(std::string stateString, Json::Value obj)
 
 	if (stateString == "research")
 	{
+		// The host decides this once during the COOP_READY handshake.  Do not
+		// retain disabled packets: otherwise switching the option back on could
+		// unexpectedly apply an old completion.
+		if (_enable_research_sync)
+		{
+			waitedResearch.append(obj);
+		}
 
-		waitedResearch.append(obj);
+	}
 
+	if (stateString == "research_sync_option" && !getServerOwner())
+	{
+		_enable_research_sync = obj.get("enabled", false).asBool();
+		Options::EnableResearchSync = _enable_research_sync;
+		if (!_enable_research_sync)
+		{
+			waitedResearch.clear();
+		}
 	}
 
 	if (stateString == "add_coop_item")
@@ -12558,11 +12573,17 @@ void connectionTCP::onTCPMessage(std::string stateString, Json::Value obj)
 		connectionTCP::_enable_time_sync = Options::EnableTimeSync;
 		root["enable_time_sync"] = connectionTCP::_enable_time_sync;
 
-		// reaction shoot option (PVP)
+		// Reaction-fire suppression is a PvP-only option.  Always reset the
+		// session value for non-PvP games so a disabled value from an earlier PvP
+		// session cannot leak into a later co-op campaign.
 		if (getCoopGamemode() == 2 || getCoopGamemode() == 3)
 		{
 			connectionTCP::_enable_reaction_shoot = Options::EnableReactionFirePvp;
 		}
+		else
+		{
+			connectionTCP::_enable_reaction_shoot = true;
+		}
 
 		// reaction shoot
 		root["enable_reaction_shoot"] = connectionTCP::_enable_reaction_shoot;
@@ -12698,6 +12719,10 @@ void connectionTCP::onTCPMessage(std::string stateString, Json::Value obj)
 		// research option
 		bool enable_research_sync = obj["enable_research_sync"].asBool();
 		_enable_research_sync = enable_research_sync;
+		// Keep the multiplayer options page in sync with the host-authoritative
+		// session value.  Previously it continued to display the client's local
+		// default (YES), even when the host had sent NO.
+		Options::EnableResearchSync = enable_research_sync;
 
 		// time option
 		bool enable_time_sync = obj["enable_time_sync"].asBool();

src/Engine/Options.cpp

@@ -574,7 +574,7 @@ void createAdvancedOptionsOTHER()
 	// coop
 	_info.push_back(OptionInfo(OPTION_OTHER, "EnableTimeSync", &EnableTimeSync, true, "Enable Time Sync", "STR_GEOSCAPE"));
 	_info.push_back(OptionInfo(OPTION_OTHER, "EnableHostOnlyTimeSpeed", &EnableHostOnlyTimeSpeed, false, "Only Host Can Change Time Speed", "STR_GEOSCAPE"));
-	_info.push_back(OptionInfo(OPTION_OTHER, "EnableResearchSync", &EnableResearchSync, true, "Enable Research Sync", "STR_BASESCAPE"));
+	_info.push_back(OptionInfo(OPTION_OTHER, "EnableResearchSync", &EnableResearchSync, true, "Enable Research Sync (Separate)", "STR_BASESCAPE"));
 	_info.push_back(OptionInfo(OPTION_OTHER, "UnbalancedCraftSoldiersLimit", &UnbalancedCraftSoldiersLimit, false, "Unbalanced Craft Soldiers Limit", "STR_BASESCAPE"));
 	_info.push_back(OptionInfo(OPTION_OTHER, "EnableReactionFirePvp", &EnableReactionFirePvp, true, "Enable Reaction Fire in PvP", "STR_BATTLESCAPE"));
 	_info.push_back(OptionInfo(OPTION_OTHER, "EnableOtherPlayerFootsteps", &EnableOtherPlayerFootsteps, true,"Enable Other Player Footstep Sounds", "STR_BATTLESCAPE"));

src/Geoscape/GeoscapeState.cpp

@@ -1357,7 +1357,9 @@ void GeoscapeState::think()
 
 	// coop
 	// research
-	if (_game->getCoopMod()->getCoopStatic() && !_game->getCoopMod()->waitedResearch.empty())
+	if (_game->getCoopMod()->getCoopStatic()
+		&& _game->getCoopMod()->_enable_research_sync
+		&& !_game->getCoopMod()->waitedResearch.empty())
 	{
 
 		if (_game->getSavedGame()->getSelectedBase())
@@ -1423,6 +1425,13 @@ void GeoscapeState::think()
 			}
 		}
 	}
+	else if (!_game->getCoopMod()->_enable_research_sync
+		&& !_game->getCoopMod()->waitedResearch.empty())
+	{
+		// Never let completions received before/while research sync was disabled
+		// become active later if the option changes.
+		_game->getCoopMod()->waitedResearch.clear();
+	}
 
 	// coop
 	_game->getCoopMod()->setCoopCampaign(true);

src/Geoscape/ResearchCompleteState.cpp

@@ -94,7 +94,8 @@ ResearchCompleteState::ResearchCompleteState(const RuleResearch* newResearch, co
 	// here. (This also avoids the newResearch->getName() null-deref below when the
 	// host completes an already-seen lookup.)
 	if (_game->getCoopMod()->getCoopStatic() == true && _coop == false
-		&& !_game->getCoopMod()->isSharedCampaign())
+		&& !_game->getCoopMod()->isSharedCampaign()
+		&& _game->getCoopMod()->_enable_research_sync)
 	{
 
 		Json::Value root;
@@ -117,7 +118,7 @@ ResearchCompleteState::ResearchCompleteState(const RuleResearch* newResearch, co
 		}
 
 		root["base_lat"] = base->getLatitude();
-		root["base_lot"] = base->getLongitude();
+		root["base_lon"] = base->getLongitude();
 
 		_game->getCoopMod()->sendTCPPacketData(root.toStyledString());