multithreaded collision fixes (scp-fs2open#7311)

* rename misleading variable check_again to never_check_again

The first element of collision_result means "never check again" per the
typedef comment, but was destructured as check_again (opposite meaning).
Rename to never_check_again to match the actual semantics and prevent
future confusion.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix data races in ship-weapon collision threading

Two thread-safety bugs in ship_weapon_check_collision, which runs on
worker threads:

1. update_danger_weapon() wrote to Ai_info[] (danger_weapon_objnum,
   danger_weapon_signature) without synchronization. Multiple weapons
   targeting the same ship would race on the same Ai_info entry.

2. Homing missile detonation logic wrote wp->lifeleft and
   wp->weapon_flags directly on the Weapons[] global array.

Fix: defer both writes to the main-thread post-processing phase via
new flags in ship_weapon_collision_data. Also convert the collision
data from a 6-element tuple to a named struct for readability.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix data race in ship-ship collision submodel checking

ship_ship_check_collision() toggled pmi->submodel[].collision_checked
flags on the heavy ship's polymodel_instance to selectively enable/
disable submodel collision checks. When two collision pairs shared the
same heavy ship, worker threads would race on the same pmi entries.

Fix: add a collision_checked_override pointer to mc_info that lets
callers provide a thread-local array of collision_checked values.
model_collide() uses the override when set, falling back to the
pmi->submodel[] field otherwise. ship_ship_check_collision() now
builds a local SCP_vector<char> and passes it via the override,
completely avoiding writes to shared polymodel_instance state.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Ensure threads have spun down before continuing non-threaded execution

* address review feedback for ship-weapon collision threading

- postproc flag now accumulates incrementally instead of being OR'd
  at the final return, ensuring all return paths respect it
- early return for predictive collisions now preserves the
  should_update_danger_weapon flag in the collision data
- moved nprintf detonation debug print to the main-thread process
  pass for thread safety

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* move collision_checked from pmi into mc_info for thread safety

collision_checked flags were transient per-collision-pass state that
didn't belong in the shared polymodel_instance. Now mc_info owns an
SCP_vector<char> that model_collide auto-initializes from pmi when
empty. All collision callers (ship-ship, asteroid, debris, prop)
updated to manipulate mc.collision_checked instead of pmi->submodel[].

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Convert to CV and spinup counter

* Update comment

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: BMagnu <6238428+BMagnu@users.noreply.github.com>

Author: 3 people

code/asteroid/asteroid.cpp

@@ -1315,20 +1315,18 @@ int asteroid_check_collision(object *pasteroid, object *other_obj, vec3d *hitpos
 				model_get_moving_submodel_list(submodel_vector, heavy_obj);
 
 				// turn off all moving submodels, collide against only 1 at a time.
-				// turn off collision detection for all moving submodels
+				mc.collision_checked.assign(pm->n_models, 0);
 				for (auto submodel: submodel_vector) {
-					pmi->submodel[submodel].collision_checked = true;
+					mc.collision_checked[submodel] = true;
 				}
 
 				// Only check single submodel now, since children of moving submodels are handled as moving as well
 				mc.flags = orig_flags | MC_SUBMODEL;
 
 				// 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;
+					mc.collision_checked[submodel] = false;
 
 					// find the start and end positions of the sphere in submodel RF
 					model_instance_global_to_local_point(&p0, &light_obj->last_pos, pm, pmi, submodel, &heavy_obj->last_orient, &heavy_obj->last_pos, true);
@@ -1360,9 +1358,12 @@ int asteroid_check_collision(object *pasteroid, object *other_obj, vec3d *hitpos
 						}
 					}
 					// Don't look at this submodel again
-					smi->collision_checked = true;
+					mc.collision_checked[submodel] = true;
 				}
 
+				// Clear collision_checked before base model pass so it auto-inits fresh
+				mc.collision_checked.clear();
+
 			}
 
 			// Now complete base model collision checks that do not take into account rotating submodels.

code/debris/debris.cpp

@@ -1030,9 +1030,9 @@ int debris_check_collision(object *pdebris, object *other_obj, vec3d *hitpos, co
 				model_get_moving_submodel_list(submodel_vector, heavy_obj);
 
 				// turn off all moving submodels, collide against only 1 at a time.
-				// turn off collision detection for all moving submodels
+				mc.collision_checked.assign(pm->n_models, 0);
 				for (auto submodel : submodel_vector) {
-					pmi->submodel[submodel].collision_checked = true;
+					mc.collision_checked[submodel] = true;
 				}
 
 				// Only check single submodel now, since children of moving submodels are handled as moving as well
@@ -1044,10 +1044,8 @@ int debris_check_collision(object *pdebris, object *other_obj, vec3d *hitpos, co
 
 				// 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;
+					mc.collision_checked[submodel] = false;
 
 					// find the start and end positions of the sphere in submodel RF
 					model_instance_global_to_local_point(&p0, &light_obj->last_pos, pm, pmi, submodel, &heavy_obj->last_orient, &heavy_obj->last_pos, true);
@@ -1081,8 +1079,11 @@ int debris_check_collision(object *pdebris, object *other_obj, vec3d *hitpos, co
 					}
 
 					// Don't look at this submodel again
-					smi->collision_checked = true;
+					mc.collision_checked[submodel] = true;
 				}
+
+				// Clear collision_checked before base model pass so it auto-inits fresh
+				mc.collision_checked.clear();
 			}
 
 			// Now complete base model collision checks that do not take into account rotating submodels.

code/model/model.h

@@ -137,7 +137,6 @@ struct submodel_instance
 	TIMESTAMP stepped_translation_started;
 
 	bool	blown_off = false;						// If set, this subobject is blown off
-	bool	collision_checked = false;
 
 	// These fields are the true standard reference for submodel rotation.  They should seldom be read directly
 	// and should almost never be written directly.  In most cases, coders should prefer cur_angle and prev_angle.
@@ -1282,6 +1281,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
 
+	// Per-submodel collision_checked flags (indexed by submodel number).
+	// When non-empty, model_collide uses this to determine which submodels to skip.
+	// Auto-initialized from pmi in model_collide when empty; callers may pre-populate
+	// to control which submodels are checked (e.g., rotating submodel collision).
+	SCP_vector<char> collision_checked;
+
 	// 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.empty() && Mc->collision_checked[i];
 		}
 
 		// Don't check it or its children if it is destroyed
@@ -1150,6 +1150,10 @@ int model_collide(mc_info *mc_info_obj)
 		Mc_pmi = NULL;
 	}
 
+	if (Mc_pmi && Mc->collision_checked.empty()) {
+		Mc->collision_checked.resize(Mc_pm->n_models, 0);
+	}
+
 	// DA 11/19/98 - disable this check for rotating submodels
 	// Don't do check if for very small movement
 //	if (Mc_mag < 0.01f) {

code/object/collideshipship.cpp

@@ -260,9 +260,9 @@ int ship_ship_check_collision(collision_info_struct *ship_ship_hit_info)
 			model_get_moving_submodel_list(submodel_vector, heavy_obj);
 
 			// turn off all moving submodels, collide against only 1 at a time.
-			// turn off collision detection for all moving submodels
+			mc.collision_checked.assign(pm->n_models, 0);
 			for (auto submodel : submodel_vector) {
-				pmi->submodel[submodel].collision_checked = true;
+				mc.collision_checked[submodel] = true;
 			}
 
 			// Only check single submodel now, since children of moving submodels are handled as moving as well
@@ -274,14 +274,12 @@ int ship_ship_check_collision(collision_info_struct *ship_ship_hit_info)
 
 			// 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;
+				mc.collision_checked[submodel] = false;
 
-				if (smi->blown_off)
+				if (pmi->submodel[submodel].blown_off)
 				{
-					smi->collision_checked = true;
+					mc.collision_checked[submodel] = true;
 					continue;
 				}
 
@@ -315,8 +313,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
+				mc.collision_checked[submodel] = true;
 			}
 
+			// Clear collision_checked before subsequent model_collide calls so it auto-inits fresh
+			mc.collision_checked.clear();
 		}
 
 		// 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,14 @@ 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;
+	bool postproc = should_update_danger_weapon;
 
 	int	valid_hit_occurred = 0;				// If this is set, then hitpos is set
 	int	quadrant_num = -1;
@@ -503,12 +511,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 { postproc, false, {std::nullopt, -1, false, -1, -1, ZERO_VECTOR, should_update_danger_weapon, false} }; //No hit, but continue checking
 	}
 
-	bool postproc = valid_hit_occurred || notify_ai_shield_down >= 0;
+	postproc = 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,17 +530,16 @@ 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);
-
-					if (ship_objp == Player_obj)
-						nprintf(("Jim", "Frame %i: Weapon %d set to detonate, dist = %7.3f.\n", Framecount, OBJ_INDEX(weapon_objp), dist));
+					// Detonation writes are deferred to the main thread for thread safety
+					should_detonate = true;
+					postproc = true;
 					valid_hit_occurred = 1; //No hit, continue checking
 				}
 			}
 		}
 	}
 
+	collision_data.should_detonate = should_detonate;
 	return { postproc, !static_cast<bool>(valid_hit_occurred), collision_data} ;
 }
 
@@ -543,15 +551,26 @@ 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 (ship_objp == Player_obj)
+			nprintf(("Jim", "Frame %i: Weapon %d set to detonate, dist = %7.3f.\n", Framecount, OBJ_INDEX(weapon_objp), vm_vec_dist_quick(&ship_objp->pos, &weapon_objp->pos)));
+	}
 
-	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 +602,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 +701,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 +711,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 +721,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/prop/prop.cpp

@@ -934,9 +934,9 @@ int prop_check_collision(object* prop_obj, object* other_obj, vec3d* hitpos, col
 				model_get_moving_submodel_list(submodel_vector, heavy_obj);
 
 				// turn off all moving submodels, collide against only 1 at a time.
-				// turn off collision detection for all moving submodels
+				mc.collision_checked.assign(pm->n_models, 0);
 				for (auto submodel : submodel_vector) {
-					pmi->submodel[submodel].collision_checked = true;
+					mc.collision_checked[submodel] = true;
 				}
 
 				// Only check single submodel now, since children of moving submodels are handled as moving as well
@@ -948,10 +948,8 @@ int prop_check_collision(object* prop_obj, object* other_obj, vec3d* hitpos, col
 
 				// 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;
+					mc.collision_checked[submodel] = false;
 
 					// find the start and end positions of the sphere in submodel RF
 					model_instance_global_to_local_point(&p0, &light_obj->last_pos, pm, pmi, submodel, &heavy_obj->last_orient, &heavy_obj->last_pos, true);
@@ -984,8 +982,11 @@ int prop_check_collision(object* prop_obj, object* other_obj, vec3d* hitpos, col
 					}
 
 					// Don't look at this submodel again
-					smi->collision_checked = true;
+					mc.collision_checked[submodel] = true;
 				}
+
+				// Clear collision_checked before base model pass so it auto-inits fresh
+				mc.collision_checked.clear();
 			}
 
 			// Now complete base model collision checks that do not take into account rotating submodels.