Skip to content

Commit 6864eb1

Browse files
Goober5000claude
andcommitted
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>
1 parent 833c452 commit 6864eb1

3 files changed

Lines changed: 24 additions & 8 deletions

File tree

code/model/model.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1282,6 +1282,12 @@ typedef struct mc_info {
12821282
float radius = 0; // If MC_CHECK_THICK is set, checks a sphere moving with the radius.
12831283
int lod = 0; // Which detail level of the submodel to check instead
12841284

1285+
// Optional per-submodel collision_checked override array (indexed by submodel number).
1286+
// When non-null, model_collide uses this instead of pmi->submodel[].collision_checked,
1287+
// allowing callers to control which submodels are skipped without writing to shared state.
1288+
// This is essential for thread-safe ship-ship collision detection.
1289+
const char *collision_checked_override = nullptr;
1290+
12851291
// Return values
12861292
int num_hits = 0; // How many collisions were found
12871293
float hit_dist = 0.0f; // The distance from p0 to hitpoint

code/model/modelcollide.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1098,7 +1098,7 @@ void mc_check_subobj( int mn )
10981098
vm_vec_add2(&instance_offset, &csmi->canonical_offset);
10991099

11001100
blown_off = csmi->blown_off;
1101-
collision_checked = csmi->collision_checked;
1101+
collision_checked = Mc->collision_checked_override ? Mc->collision_checked_override[i] : csmi->collision_checked;
11021102
}
11031103

11041104
// Don't check it or its children if it is destroyed

code/object/collideshipship.cpp

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -259,29 +259,34 @@ int ship_ship_check_collision(collision_info_struct *ship_ship_hit_info)
259259
SCP_vector<int> submodel_vector;
260260
model_get_moving_submodel_list(submodel_vector, heavy_obj);
261261

262+
// Build a local collision_checked override array so we don't write to shared pmi state.
263+
// This is essential for thread safety when multiple pairs share the same heavy ship.
264+
SCP_vector<char> collision_checked_local(pm->n_models, 0);
265+
for (int j = 0; j < pm->n_models; j++) {
266+
collision_checked_local[j] = pmi->submodel[j].collision_checked;
267+
}
268+
262269
// turn off all moving submodels, collide against only 1 at a time.
263-
// turn off collision detection for all moving submodels
264270
for (auto submodel : submodel_vector) {
265-
pmi->submodel[submodel].collision_checked = true;
271+
collision_checked_local[submodel] = true;
266272
}
267273

268274
// Only check single submodel now, since children of moving submodels are handled as moving as well
269275
mc.flags = orig_flags | MC_SUBMODEL;
276+
mc.collision_checked_override = collision_checked_local.data();
270277

271278
if (heavy_sip->collision_lod > -1) {
272279
mc.lod = heavy_sip->collision_lod;
273280
}
274281

275282
// check each submodel in turn
276283
for (auto submodel : submodel_vector) {
277-
auto smi = &pmi->submodel[submodel];
278-
279284
// turn on just one submodel for collision test
280-
smi->collision_checked = false;
285+
collision_checked_local[submodel] = false;
281286

282-
if (smi->blown_off)
287+
if (pmi->submodel[submodel].blown_off)
283288
{
284-
smi->collision_checked = true;
289+
collision_checked_local[submodel] = true;
285290
continue;
286291
}
287292

@@ -315,8 +320,13 @@ int ship_ship_check_collision(collision_info_struct *ship_ship_hit_info)
315320
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);
316321
}
317322
}
323+
324+
// re-disable this submodel before enabling the next one
325+
collision_checked_local[submodel] = true;
318326
}
319327

328+
// Clear the override before subsequent model_collide calls
329+
mc.collision_checked_override = nullptr;
320330
}
321331

322332
// Now complete base model collision checks that do not take into account rotating submodels.

0 commit comments

Comments
 (0)