multithreaded collision fixes#7311
Conversation
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>
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>
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
code/object/collideshipweapon.cpp
Outdated
|
|
||
| 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} ; |
There was a problem hiding this comment.
Instead of being seperate bools, both should_detonate and should_update_danger_weapon should just update postproc when they are determined. That makes it more intuitive, as postproc is literally meant for any need of postprocessing this collision, as well as properly updating it for the return on line 509 which also needs the should_update_danger_weapon flag at least
code/model/model.h
Outdated
| // 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; |
There was a problem hiding this comment.
I believe this should not be optional.
This same safety issue will come back for all other forms when we parallelize them. In fact, even with the current changes, the issue would persist for ship ship collisions.
And, tbh, data used for a single collision should really not persist in the pmi data at all.
There's a case to be made for this to just store the SCP_vector directly here, being automatically initialized in model_collide if a pmi is present, and locking the irrelevant submodels with MC_SUBMODEL set.
code/object/collideshipweapon.cpp
Outdated
| if (ship_objp == Player_obj) | ||
| nprintf(("Jim", "Frame %i: Weapon %d set to detonate, dist = %7.3f.\n", Framecount, OBJ_INDEX(weapon_objp), dist)); |
There was a problem hiding this comment.
For thread-safety of prints this should also move to the resolve pass
code/object/collideshipweapon.cpp
Outdated
| 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 |
There was a problem hiding this comment.
this also needs to respect should_update_danger_weapon, both in the argument of the pass-out data, as well as in the postproc.
|
Looks like this is the last 25.0.1 tagged PR awaiting revision. I know you are super busy, @Goober5000, just wanted to check if you might have an estimate for when this might be ready or maybe it's hard to tell? |
I'll work on this now |
- 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>
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>
|
Ok, the PR has been updated pursuant to the feedback |
3 participants
Three commits,
threefour fixes to the multithreaded collision code. These correspond to items 1, 2, 3, and 5 from Claude's audit report.Also added @BMagnu 's fix for bug 4: