multithreaded collision fixes

code/model/model.h

@@ -1282,6 +1282,12 @@ typedef struct mc_info {
 	float   radius = 0;                 // If MC_CHECK_THICK is set, checks a sphere moving with the radius.
 	int     lod = 0;                    // Which detail level of the submodel to check instead
 
+	// Optional per-submodel collision_checked override array (indexed by submodel number).
+	// When non-null, model_collide uses this instead of pmi->submodel[].collision_checked,
+	// allowing callers to control which submodels are skipped without writing to shared state.
+	// This is essential for thread-safe ship-ship collision detection.
+	const char *collision_checked_override = nullptr;
+
 	// Return values
 	int     num_hits = 0;               // How many collisions were found
 	float   hit_dist = 0.0f;            // The distance from p0 to hitpoint

code/model/modelcollide.cpp

@@ -1098,7 +1098,7 @@ void mc_check_subobj( int mn )
 			vm_vec_add2(&instance_offset, &csmi->canonical_offset);
 
 			blown_off = csmi->blown_off;
-			collision_checked = csmi->collision_checked;
+			collision_checked = Mc->collision_checked_override ? Mc->collision_checked_override[i] : csmi->collision_checked;
 		}
 
 		// Don't check it or its children if it is destroyed

code/object/collideshipship.cpp

@@ -259,29 +259,34 @@ int ship_ship_check_collision(collision_info_struct *ship_ship_hit_info)
 			SCP_vector<int> submodel_vector;
 			model_get_moving_submodel_list(submodel_vector, heavy_obj);
 
+			// Build a local collision_checked override array so we don't write to shared pmi state.
+			// This is essential for thread safety when multiple pairs share the same heavy ship.
+			SCP_vector<char> collision_checked_local(pm->n_models, 0);
+			for (int j = 0; j < pm->n_models; j++) {
+				collision_checked_local[j] = pmi->submodel[j].collision_checked;
+			}
+
 			// turn off all moving submodels, collide against only 1 at a time.
-			// turn off collision detection for all moving submodels
 			for (auto submodel : submodel_vector) {
-				pmi->submodel[submodel].collision_checked = true;
+				collision_checked_local[submodel] = true;
 			}
 
 			// Only check single submodel now, since children of moving submodels are handled as moving as well
 			mc.flags = orig_flags | MC_SUBMODEL;
+			mc.collision_checked_override = collision_checked_local.data();
 
 			if (heavy_sip->collision_lod > -1) {
 				mc.lod = heavy_sip->collision_lod;
 			}
 
 			// check each submodel in turn
 			for (auto submodel : submodel_vector) {
-				auto smi = &pmi->submodel[submodel];
-
 				// turn on just one submodel for collision test
-				smi->collision_checked = false;
+				collision_checked_local[submodel] = false;
 
-				if (smi->blown_off)
+				if (pmi->submodel[submodel].blown_off)
 				{
-					smi->collision_checked = true;
+					collision_checked_local[submodel] = true;
 					continue;
 				}
 
@@ -315,8 +320,13 @@ int ship_ship_check_collision(collision_info_struct *ship_ship_hit_info)
 						model_instance_local_to_global_point(&ship_ship_hit_info->light_collision_cm_pos, &int_light_pos, pm, pmi, mc.hit_submodel, &heavy_obj->orient, &zero);
 					}
 				}
+
+				// re-disable this submodel before enabling the next one
+				collision_checked_local[submodel] = true;
 			}
 
+			// Clear the override before subsequent model_collide calls
+			mc.collision_checked_override = nullptr;
 		}
 
 		// Now complete base model collision checks that do not take into account rotating submodels.

code/object/collideshipweapon.cpp

@@ -29,8 +29,16 @@
 #include "ship/shiphit.h"
 #include "weapon/weapon.h"
 
-//mc, notify_ai_shield_down, shield_collision, quadrant_num, shield_tri_hit, shield_hitpoint
-using ship_weapon_collision_data = std::tuple<std::optional<mc_info>, int, bool, int, int, vec3d>;
+struct ship_weapon_collision_data {
+	std::optional<mc_info> mc;
+	int notify_ai_shield_down;
+	bool shield_collision;
+	int quadrant_num;
+	int shield_tri_hit;
+	vec3d shield_hitpos;
+	bool should_update_danger_weapon;	// deferred to main thread for thread safety
+	bool should_detonate;				// deferred to main thread for thread safety
+};
 
 extern int Game_skill_level;
 extern float ai_endangered_time(const object *ship_objp, const object *weapon_objp);
@@ -206,14 +214,13 @@ static std::tuple<bool, bool, ship_weapon_collision_data> ship_weapon_check_coll
 	Assert( shipp->objnum == OBJ_INDEX(ship_objp));
 
 	// Make ships that are warping in not get collision detection done
-	if ( shipp->is_arriving() ) return {false, true, {std::nullopt, -1, false, -1, -1, ZERO_VECTOR}};
+	if ( shipp->is_arriving() ) return {false, true, {std::nullopt, -1, false, -1, -1, ZERO_VECTOR, false, false}};
 
 	//	Return information for AI to detect incoming fire.
 	//	Could perhaps be done elsewhere at lower cost --MK, 11/7/97
 	float	dist = vm_vec_dist_quick(&ship_objp->pos, &weapon_objp->pos);
-	if (dist < weapon_objp->phys_info.speed) {
-		update_danger_weapon(ship_objp, weapon_objp);
-	}
+	bool should_update_danger_weapon = (dist < weapon_objp->phys_info.speed);
+	bool should_detonate = false;
 
 	int	valid_hit_occurred = 0;				// If this is set, then hitpos is set
 	int	quadrant_num = -1;
@@ -503,12 +510,13 @@ static std::tuple<bool, bool, ship_weapon_collision_data> ship_weapon_check_coll
 		*next_hit = (int) (1000.0f * (mc->hit_dist*(flFrametime + time_limit) - flFrametime) );
 		if (*next_hit > 0)
 			// if hit occurs outside of this frame, do not do damage
-			return { false, false, {std::nullopt, -1, false, -1, -1, ZERO_VECTOR} }; //No hit, but continue checking
+			return { false, false, {std::nullopt, -1, false, -1, -1, ZERO_VECTOR, false, false} }; //No hit, but continue checking
 	}
 
 	bool postproc = valid_hit_occurred || notify_ai_shield_down >= 0;
 	ship_weapon_collision_data collision_data {
-		valid_hit_occurred ? std::optional(*mc) : std::nullopt, notify_ai_shield_down, postproc, quadrant_num, shield_tri_hit, shield_hitpos
+		valid_hit_occurred ? std::optional(*mc) : std::nullopt, notify_ai_shield_down, postproc, quadrant_num, shield_tri_hit, shield_hitpos,
+		should_update_danger_weapon, false
 	};
 
 	// when the $Fixed Missile Detonation: flag is active, skip this whole block, as it's redundant to a similar check in weapon_home()
@@ -521,8 +529,8 @@ static std::tuple<bool, bool, ship_weapon_collision_data> ship_weapon_check_coll
 			if (vm_vec_dot(&vec_to_ship, &weapon_objp->orient.vec.fvec) < 0.0f) {
 				// check if we're colliding against "invisible" ship
 				if (!(shipp->flags[Ship::Ship_Flags::Dont_collide_invis])) {
-					wp->lifeleft = 0.001f;
-					wp->weapon_flags.set(Weapon::Weapon_Flags::Begun_detonation);
+					// Detonation writes are deferred to the main thread for thread safety
+					should_detonate = true;
 
 					if (ship_objp == Player_obj)
 						nprintf(("Jim", "Frame %i: Weapon %d set to detonate, dist = %7.3f.\n", Framecount, OBJ_INDEX(weapon_objp), dist));
@@ -532,7 +540,8 @@ static std::tuple<bool, bool, ship_weapon_collision_data> ship_weapon_check_coll
 		}
 	}
 
-	return { postproc, !static_cast<bool>(valid_hit_occurred), collision_data} ;
+	collision_data.should_detonate = should_detonate;
+	return { postproc || should_detonate || should_update_danger_weapon, !static_cast<bool>(valid_hit_occurred), collision_data} ;
 }
 
 static void ship_weapon_process_collision(obj_pair* pair, const ship_weapon_collision_data& collision_data) {
@@ -543,15 +552,23 @@ static void ship_weapon_process_collision(obj_pair* pair, const ship_weapon_coll
 	const weapon_info* wip = &Weapon_info[wp->weapon_info_index];
 	const ship_info* sip = &Ship_info[shipp->ship_info_index];
 
-	const auto& [mc_opt, notify_ai_shield_down, shield_collision, quadrant_num, shield_tri_hit, shield_hitpos] = collision_data;
-	bool valid_hit_occurred = mc_opt.has_value();
-	auto mc = valid_hit_occurred ? &(*mc_opt) : nullptr;
+	bool valid_hit_occurred = collision_data.mc.has_value();
+	auto mc = valid_hit_occurred ? &(*collision_data.mc) : nullptr;
+
+	// Perform deferred writes that are unsafe to do from worker threads
+	if (collision_data.should_update_danger_weapon)
+		update_danger_weapon(ship_objp, weapon_objp);
+
+	if (collision_data.should_detonate) {
+		wp->lifeleft = 0.001f;
+		wp->weapon_flags.set(Weapon::Weapon_Flags::Begun_detonation);
+	}
 
-	if (notify_ai_shield_down >= 0)
-		Ai_info[Ships[ship_objp->instance].ai_index].danger_shield_quadrant = notify_ai_shield_down;
+	if (collision_data.notify_ai_shield_down >= 0)
+		Ai_info[Ships[ship_objp->instance].ai_index].danger_shield_quadrant = collision_data.notify_ai_shield_down;
 
-	if (shield_tri_hit >= 0)
-		add_shield_point(OBJ_INDEX(ship_objp), shield_tri_hit, &shield_hitpos, wip->shield_impact_effect_radius);
+	if (collision_data.shield_tri_hit >= 0)
+		add_shield_point(OBJ_INDEX(ship_objp), collision_data.shield_tri_hit, &collision_data.shield_hitpos, wip->shield_impact_effect_radius);
 
 	if ( valid_hit_occurred )
 	{
@@ -583,12 +600,12 @@ static void ship_weapon_process_collision(obj_pair* pair, const ship_weapon_coll
 		}
 
 		if(!ship_override && !weapon_override) {
-			if (shield_collision && quadrant_num >= 0) {
+			if (collision_data.shield_collision && collision_data.quadrant_num >= 0) {
 				if ((sip->shield_impact_explosion_anim.isValid()) && (wip->shield_impact_explosion_radius > 0)) {
 					shield_impact_explosion(mc->hit_point, mc->hit_normal, ship_objp, weapon_objp, wip->shield_impact_explosion_radius, sip->shield_impact_explosion_anim);
 				}
 			}
-			ship_weapon_do_hit_stuff(ship_objp, weapon_objp, &mc->hit_point_world, &mc->hit_point, quadrant_num, mc->hit_submodel, &mc->hit_normal);
+			ship_weapon_do_hit_stuff(ship_objp, weapon_objp, &mc->hit_point_world, &mc->hit_point, collision_data.quadrant_num, mc->hit_submodel, &mc->hit_normal);
 		}
 
 		if (scripting::hooks::OnWeaponCollision->isActive() && !(weapon_override && !ship_override)) {
@@ -682,7 +699,7 @@ static std::tuple<bool, bool, ship_weapon_collision_data> prop_weapon_check_coll
 			bool postproc = true;
 			bool recheck = false;
 			// most of this data is irrevelant for props but let's match it to make it easy
-			ship_weapon_collision_data cd{mc, -1, postproc, -1, -1, ZERO_VECTOR};
+			ship_weapon_collision_data cd{mc, -1, postproc, -1, -1, ZERO_VECTOR, false, false};
 			return {postproc, recheck, cd};
 		}
 	}
@@ -692,7 +709,7 @@ static std::tuple<bool, bool, ship_weapon_collision_data> prop_weapon_check_coll
 	bool postproc = (valid_hit_occurred != 0);
 	bool recheck = (valid_hit_occurred == 0);
 	// most of this data is irrevelant for props but let's match it to make it easy
-	ship_weapon_collision_data cd{(valid_hit_occurred ? mc : mc_info{}), -1, postproc, -1, -1, ZERO_VECTOR};
+	ship_weapon_collision_data cd{(valid_hit_occurred ? mc : mc_info{}), -1, postproc, -1, -1, ZERO_VECTOR, false, false};
 
 	return{postproc, recheck, cd};
 }
@@ -702,11 +719,10 @@ static void prop_weapon_process_collision(obj_pair* pair, const ship_weapon_coll
 	object* prop_objp = pair->a;
 	object* weapon_objp = pair->b;
 
-	auto mc_opt = std::get<0>(cd);
-	if (!mc_opt) {
+	if (!cd.mc) {
 		return;
 	}
-	const mc_info& mc = *mc_opt;
+	const mc_info& mc = *cd.mc;
 
 	weapon* wp = &Weapons[weapon_objp->instance];
 	wp->collisionInfo = new mc_info(mc);

code/object/objcollide.cpp

@@ -762,6 +762,7 @@ void spin_up_mp_collision() {
 void spin_down_mp_collision() {
 	threading::spin_down_threaded_task();
 	collision_processing_done.store(true);
+	threading::spin_down_wait_complete();
 }
 
 void queue_mp_collision(uint ctype, const obj_pair& colliding) {
@@ -1210,11 +1211,11 @@ void collide_mp_worker_thread(size_t threadIdx) {
 						UNREACHABLE("Got non MP-compatible collision type!");
 				}
 
-				auto&& [check_again, collision_data_maybe, collision_fnc] = check_collision(&collision_check.objs);
+				auto&& [never_check_again, collision_data_maybe, collision_fnc] = check_collision(&collision_check.objs);
 
 				{
 					std::scoped_lock lock{thread.result_mutex};
-					thread.queue_results->emplace_back(collision_thread_data::collision_queue_result{collision_check.objs, check_again, collision_data_maybe, collision_fnc});
+					thread.queue_results->emplace_back(collision_thread_data::collision_queue_result{collision_check.objs, never_check_again, collision_data_maybe, collision_fnc});
 				}
 				thread.result_length.fetch_add(1, std::memory_order_release);
 				thread.queue_length.fetch_sub(1, std::memory_order_release);

code/utils/threading.cpp

@@ -20,6 +20,7 @@ namespace threading {
 	static std::condition_variable wait_for_task;
 	static std::mutex wait_for_task_mutex;
 	static bool wait_for_task_condition;
+	static std::atomic_uint32_t wait_for_spindown_tasks;
 	static std::atomic<WorkerThreadTask> worker_task;
 
 	static SCP_vector<std::thread> worker_threads;
@@ -28,12 +29,15 @@ namespace threading {
 	static void mp_worker_thread_main(size_t threadIdx) {
 		while(true) {
 			{
+				//We're waiting for a new task, so spindown was successful
+				wait_for_spindown_tasks.fetch_add(1, std::memory_order_release);
 				std::unique_lock<std::mutex> lk(wait_for_task_mutex);
 				wait_for_task.wait(lk, []() { return wait_for_task_condition; });
 			}
 
 			switch (worker_task.load(std::memory_order_acquire)) {
 				case WorkerThreadTask::EXIT:
+					//We're done and will quit, so ensure we report this.
 					return;
 				case WorkerThreadTask::COLLISION:
 					collide_mp_worker_thread(threadIdx);
@@ -144,6 +148,7 @@ namespace threading {
 	//External Functions
 
 	void spin_up_threaded_task(WorkerThreadTask task) {
+		wait_for_spindown_tasks.store(0, std::memory_order_release);
 		worker_task.store(task);
 		{
 			std::scoped_lock lock {wait_for_task_mutex};
@@ -157,6 +162,11 @@ namespace threading {
 		wait_for_task_condition = false;
 	}
 
+	void spin_down_wait_complete() {
+		//Technically, spindowns should only occur when the actual code is confirmed to be complete. So busy-waiting here is not an issue.
+		while (wait_for_spindown_tasks.load(std::memory_order_acquire) < num_threads);
+	}
+
 	void init_task_pool() {
 		if (Cmdline_multithreading == 0) {
 			//At least given the current collision-detection threading, 8 cores (if available) seems like a sweetspot, with more cores adding too much overhead.
@@ -181,6 +191,8 @@ namespace threading {
 	void shut_down_task_pool() {
 		spin_up_threaded_task(WorkerThreadTask::EXIT);
 
+		//Technically we could await spin_down_wait_complete here, but since we're returning and joining the threads here, there is no need
+
 		for(auto& thread : worker_threads) {
 			thread.join();
 		}

code/utils/threading.h

@@ -11,6 +11,9 @@ namespace threading {
 	//This _must_ be called on the main thread BEFORE a task completes on a thread of the task pool.
 	void spin_down_threaded_task();
 
+	//This should be called AFTER the command to finish a given task is given. This will block until all threads have returned into a state where they are able to listen to new commands.
+	void spin_down_wait_complete();
+
 	void init_task_pool();
 	void shut_down_task_pool();