Fix stale subsystem model_num references across mission boundaries (#3147)

Fixes the long-standing intermittent bug where the GTD Orion#Repulse in
Silent Threat: Reborn's "Forced Hand" mission spawns with all subsystems
unlinked, leaving the ship untargetable and undisableable.  The same root
cause has been reported since at least 2015 (HLP topic 89403) and was
provisionally closed in #3147 as fixed by #3858, but #3858 was an unrelated
parse_ship_values memory leak fix and never actually addressed this
behavior.  Recently the underlying check at ship.cpp ~8043 was promoted
from Warning to Error (#5591), which is why Release players are now
hitting a hard error dialog instead of just seeing missing subsystems.

Root cause
----------
model_unload() walks Ship_info and resets si.model_num = -1 on every
ship_info that referenced the model, but does not touch
si.subsystems[k].model_num.  Subsystem entries are left pointing at the
freed model id.  When a subsequent mission reloads the same pof under a
new id, ship variants that share the model rely on
ship_copy_subsystem_fixup() to re-link their subsystem arrays, but the
fixup matches by name via model_copy_subsystems() and any unmatched
destination subsystem stays at -1.  The mismatch case in
model_copy_subsystems() was guarded only by Int3(), which is a no-op in
Release builds, so the failure was completely silent until subsys_set()
errored out much later with no useful context.

The bug is intermittent because it depends on three independent ordering
factors: which prior mission was last played, which Orion variant that
mission used, and the iteration order of Ship_info during the next
mission's ship_page_in().

Changes
-------
* model_unload(): when clearing si.model_num for an unloaded model, also
  clear every si.subsystems[k].model_num that pointed at the same id.
  This is the structural fix and eliminates the entire class of stale
  subsystem pointers surviving across mission boundaries.

* model_copy_subsystems(): replace the silent Int3() in the name-mismatch
  branch with Error(LOCATION, ...) naming the unmatched subsystem.  This
  surfaces a failure mode that has been invisible in Release builds for
  over a decade and gives modders an actionable diagnostic when sibling
  ship classes that share a model have inconsistent subsystem names.

* ship.cpp: introduce verify_ship_subsystems_linked() helper that checks
  every model_subsystem on a ship_info is linked into the same model the
  ship is using, and fails with Error(LOCATION, ...) naming both
  polymodels if not.  This is the canonical post-condition for both
  ship_copy_subsystem_fixup() and model_load() (which is supposed to
  link subsystems via do_new_subsystem()).

* ship_create(): call verify_ship_subsystems_linked() after
  ship_copy_subsystem_fixup() so that any failure to link a subsystem
  is surfaced with full context (ship name, subsystem name, both model
  filenames) instead of propagating to a generic error in subsys_set()
  much later.  This closes a long-standing asymmetry where ship_page_in()
  had a guard (added by zookeeper in 2015) but ship_create() did not.

* ship_page_in(): three post-fixup verifications were gated by
  #ifndef NDEBUG with Warning() or Assertion() calls, both of which are
  no-ops in Release builds.  Replace each with a verify_ship_subsystems_linked()
  call so they fire in Release as well.

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

Author: Goober5000

code/model/modelread.cpp

@@ -277,6 +277,15 @@ void model_unload(int modelnum, int force)
 	for (auto& si : Ship_info) {
 		if (pm->id == si.model_num) {
 			si.model_num = -1;
+
+			// also reset any subsystem model_num references that pointed to this model,
+			// otherwise stale ids can survive across missions and cause subsystems to fail
+			// to re-link when the model is reloaded with a different id.
+			for (int k = 0; k < si.n_subsystems; k++) {
+				if (si.subsystems[k].model_num == pm->id) {
+					si.subsystems[k].model_num = -1;
+				}
+			}
 		}
 
 		if (pm->id == si.cockpit_model_num) {
@@ -592,7 +601,11 @@ void model_copy_subsystems( int n_subsystems, model_subsystem *d_sp, model_subsy
 			}
 		}
 		if ( j == n_subsystems )
-			Int3();							// get allender -- something is amiss with models
+			Error(LOCATION, "Subsystem '%s' could not be matched between two ship classes that share a model. "
+				"The destination ship will be missing this subsystem at runtime. "
+				"Check that subsystem names are spelled identically on both ship classes, "
+				"and that any modular table extensions to one ship are mirrored on the other.",
+				source->subobj_name);
 
 	}
 }

code/ship/ship.cpp

@@ -7917,6 +7917,30 @@ static void ship_copy_subsystem_fixup(ship_info *sip)
 
 }
 
+// Verify that every model_subsystem on this ship_info is linked into the same
+// model the ship is using.  If any subsystem still points at a different
+// model_num, fail with a diagnostic naming both polymodels.  This is the
+// canonical post-condition check for both ship_copy_subsystem_fixup() and
+// model_load() (which is supposed to link subsystems via do_new_subsystem()).
+// context_label disambiguates the call site in the resulting Error text.
+static void verify_ship_subsystems_linked(const ship_info *sip, const char *context_label)
+{
+	for (int i = 0; i < sip->n_subsystems; i++) {
+		if (sip->subsystems[i].model_num == sip->model_num)
+			continue;
+
+		polymodel *sip_pm = (sip->model_num >= 0) ? model_get(sip->model_num) : nullptr;
+		polymodel *subsys_pm = (sip->subsystems[i].model_num >= 0) ? model_get(sip->subsystems[i].model_num) : nullptr;
+		Error(LOCATION, "%s, ship '%s' does not have subsystem '%s' linked into the model file, '%s'.\n\n(Ship_info model is '%s' and subsystem model is '%s'.)",
+			context_label,
+			sip->name,
+			sip->subsystems[i].subobj_name,
+			sip->pof_file,
+			(sip_pm != nullptr) ? sip_pm->filename : "NULL",
+			(subsys_pm != nullptr) ? subsys_pm->filename : "NULL");
+	}
+}
+
 // as with object, don't set next and prev to NULL because they keep the object on the free and used lists
 void ship_subsys::clear()
 {
@@ -11504,6 +11528,8 @@ int ship_create(matrix* orient, vec3d* pos, int ship_type, const char* ship_name
 	polymodel *pm = model_get(sip->model_num);
 
 	ship_copy_subsystem_fixup(sip);
+	verify_ship_subsystems_linked(sip, "After ship_copy_subsystem_fixup in ship_create");
+
 	show_ship_subsys_count();
 
 	if ( sip->num_detail_levels != pm->n_detail_levels )
@@ -19139,42 +19165,22 @@ void ship_page_in()
 					sip->subsystems[j].model_num = -1;
 
 				ship_copy_subsystem_fixup(&(*sip));
-
-#ifndef NDEBUG
-				for (j = 0; j < sip->n_subsystems; j++) {
-					if (sip->subsystems[j].model_num != sip->model_num) {
-						polymodel *sip_pm = (sip->model_num >= 0) ? model_get(sip->model_num) : NULL;
-						polymodel *subsys_pm = (sip->subsystems[j].model_num >= 0) ? model_get(sip->subsystems[j].model_num) : NULL;
-						Warning(LOCATION, "After ship_copy_subsystem_fixup, ship '%s' does not have subsystem '%s' linked into the model file, '%s'.\n\n(Ship_info model is '%s' and subsystem model is '%s'.)", sip->name, sip->subsystems[j].subobj_name, sip->pof_file, (sip_pm != NULL) ? sip_pm->filename : "NULL", (subsys_pm != NULL) ? subsys_pm->filename : "NULL");
-					}
-				}
-#endif
+				verify_ship_subsystems_linked(&(*sip), "After ship_copy_subsystem_fixup in ship_page_in");
 			} else {
 				// Just to be safe (I mean to check that my code works...)
 				Assert( sip->model_num >= 0 );
 				Assert( sip->model_num == model_previously_loaded );
 
-#ifndef NDEBUG
-				for (j = 0; j < sip->n_subsystems; j++) {
-					if (sip->subsystems[j].model_num != sip->model_num) {
-						polymodel *sip_pm = (sip->model_num >= 0) ? model_get(sip->model_num) : NULL;
-						polymodel *subsys_pm = (sip->subsystems[j].model_num >= 0) ? model_get(sip->subsystems[j].model_num) : NULL;
-						Warning(LOCATION, "Without ship_copy_subsystem_fixup, ship '%s' does not have subsystem '%s' linked into the model file, '%s'.\n\n(Ship_info model is '%s' and subsystem model is '%s'.)", sip->name, sip->subsystems[j].subobj_name, sip->pof_file, (sip_pm != NULL) ? sip_pm->filename : "NULL", (subsys_pm != NULL) ? subsys_pm->filename : "NULL");
-					}
-				}
-#endif
+				verify_ship_subsystems_linked(&(*sip), "Without ship_copy_subsystem_fixup in ship_page_in");
 			}
 		} else {
 			// Model not loaded, so load it
 			sip->model_num = model_load(sip->pof_file, &*sip);
 
 			Assert( sip->model_num >= 0 );
 
-#ifndef NDEBUG
-			// Verify that all the subsystem model numbers are updated
-			for (j = 0; j < sip->n_subsystems; j++)
-				Assertion( sip->subsystems[j].model_num == sip->model_num, "Model reference for subsystem %s (model num: %d) on model %s (model num: %d) is invalid.\n", sip->subsystems[j].name, sip->subsystems[j].model_num, sip->pof_file, sip->model_num );	// JAS
-#endif
+			// Verify that all the subsystem model numbers were updated by model_load
+			verify_ship_subsystems_linked(&(*sip), "After model_load in ship_page_in");
 		}
 
 		// more weapon marking, the weapon info in Ship_info[] is the default