Fix a model refcount bug on loadout change, and harden model_unload against stale handles (#7851)

* take a model reference when a ship changes class

ship_model_change() only loaded the model when the class had none, so a ship
switched to a class whose model had been loaded only by a select-screen
preview adopted that preview's single reference.  Closing the screen then
freed the model from under the ship, and the first simulation frame asserted
in model_get().  This is what happened on a replayed mission with a restored
loadout: ship select was skipped, the weapon-select overhead loaded the new
class once, the commit switched the player to it, and the screen close
released it.

Load unconditionally, as ship_create() already does, so the ship holds its own
reference.

Co-Authored-By: Claude Fable 5.1 <[email protected]>

* reject stale model ids in model_unload and release select-screen handles at level close

model_unload only used the slot part of the id it was given, so a handle kept
across missions would free whatever model had since been loaded into that slot.
Validate the id against the slot's occupant and warn instead.

Two holders could present such handles: the weapon-select icon models under a
scripted UI that overrides the state hooks (the native close never runs), and
the ship-select 3D model global, which was never reset between missions.
Release the select-screen handles from game_level_close, and unload the
ship-select model on screen close and on every selection change.

Co-Authored-By: Claude Fable 5.1 <[email protected]>

* consolidate the select-screen resource release

The weapon side released its icons, anims, and ship class data from two
functions keyed on whether the screen was open, and the level-close path added
a third copy.  Give the weapon side the same shape as the ship side: one
unconditional weapon_select_common_close(), with weapon_select_close() left as
pure UI teardown.  common_select_level_close() now just calls the two common
closes, and common_select_close() calls it in turn.  The ship-select 3D model
is released from ship_select_common_close() so both paths cover it.

Co-Authored-By: Claude Fable 5.1 <[email protected]>

---------

Co-authored-by: Claude Fable 5.1 <[email protected]>

code/missionui/missionscreencommon.cpp

@@ -979,10 +979,6 @@ void common_select_close()
 
 	nprintf(("Alan","entering common_select_close()\n"));
 
-	// catch open anims that weapon_select_init_team() opened when not in weapon_select - taylor
-	// *** not the same as weapon_select_close() ***
-	weapon_select_close_team();
-
 	weapon_select_close();
 
 	if(Game_mode & GM_MULTIPLAYER){
@@ -990,6 +986,7 @@ void common_select_close()
 	} 
 
 	ship_select_close();
+	common_select_level_close();
 	brief_close();	
 
 	common_free_interface_palette();
@@ -1015,6 +1012,16 @@ void common_select_close()
 	Common_select_inited = 0;
 }
 
+// Release the model and bitmap handles held by the select screens, whether or not the screens themselves
+// were ever opened.  Called from common_select_close() on a state change and from game_level_close(),
+// because a scripted UI that overrides the state hooks never reaches the former, and by the next mission
+// the model slots those handles name may belong to other models.
+void common_select_level_close()
+{
+	weapon_select_common_close();
+	ship_select_common_close();
+}
+
 // ------------------------------------------------------------------------
 //	load_wing_icons() creates the bitmaps for wing icons 
 //

code/missionui/missionscreencommon.h

@@ -94,6 +94,7 @@ enum class commit_pressed_status {
 void common_select_init(bool API_Access = false);
 int	common_select_do(float frametime);
 void	common_select_close();
+void	common_select_level_close();
 void	common_draw_buttons();
 void	common_check_buttons();
 void	common_check_keys(int k);

code/missionui/missionshipchoice.cpp

@@ -1323,7 +1323,7 @@ void ship_select_do(float frametime)
 	ss_maybe_drop_icon();
 
 	Assert( Ss_icons != NULL );
-
+		
 	if ( !Background_playing ) {	
 		GR_MAYBE_CLEAR_RES(Ship_select_background_bitmap);
 		gr_set_bitmap(Ship_select_background_bitmap);
@@ -1463,6 +1463,16 @@ void ship_select_do(float frametime)
 }
 
 
+// release the ship select model, if one is loaded
+static void ss_unload_select_model()
+{
+	if (ShipSelectModelNum >= 0)
+	{
+		model_unload(ShipSelectModelNum);
+		ShipSelectModelNum = -1;
+	}
+}
+
 // ------------------------------------------------------------------------
 //	ship_select_close() is called once when the ship select screen is exited
 //
@@ -1471,8 +1481,6 @@ void ship_select_close()
 {
 	key_flush();
 
-	ship_select_common_close();
-
 	if ( !Ship_select_open ) {
 		nprintf(("Alan","ship_select_close() returning without doing anything\n"));
 		return;
@@ -1636,7 +1644,7 @@ void start_ship_animation(int ship_class, int  /*play_sound*/)
 
 	if (ship_class < 0) {
 		mprintf(("No ship class passed in to start_ship_animation\n"));
-		ShipSelectModelNum = -1;
+		ss_unload_select_model();
 		return;
 	}
     
@@ -1652,8 +1660,13 @@ void start_ship_animation(int ship_class, int  /*play_sound*/)
 			Ship_anim_class = -1;
 		}
 
-		// Load the necessary model file
+		// Load the necessary model file; the previous one is released only afterwards so that
+		// re-selecting the same ship doesn't free and reload it
+		int previous_model = ShipSelectModelNum;
 		ShipSelectModelNum = model_load(sip, true);
+		if (previous_model >= 0) {
+			model_unload(previous_model);
+		}
 		
 		// page in ship textures properly (takes care of nondimming pixels)
 		model_page_in_textures(ShipSelectModelNum, ship_class);
@@ -1670,10 +1683,7 @@ void start_ship_animation(int ship_class, int  /*play_sound*/)
 		}
 
 		//If there was a model loaded for the previous ship, unload it
-		if (ShipSelectModelNum >= 0 ) {
-			model_unload(ShipSelectModelNum);
-			ShipSelectModelNum = -1;
-		}
+		ss_unload_select_model();
 
 		//unload the previous anim
 		if(Ship_anim_class > 0 && (*Ss_icons)[Ship_anim_class].ss_anim.num_frames > 0)
@@ -3136,6 +3146,8 @@ void ship_select_common_close()
 		ss_unload_team_icons(team_icons);
 		team_icons.clear();
 	}
+
+	ss_unload_select_model();
 }
 
 // change any interface data based on updated Wss_slots[] and Ss_pool[]

code/missionui/missionweaponchoice.cpp

@@ -2048,16 +2048,13 @@ void weapon_select_init_team(int team_num)
 }
 
 /**
- * Close out what weapon_select_init_team() set up but only when we are not acutally
- * in the weapon select screen - taylor
+ * Release the icons, anims, and ship class data that weapon_select_init_team() set up
  */
-void weapon_select_close_team()
+void weapon_select_common_close()
 {
-	if (Weapon_select_open)
-		return;
-
 	wl_unload_all_teams_icons();
 	wl_unload_all_anims();
+	wl_free_ship_class_data();
 }
 
 /**
@@ -3014,11 +3011,6 @@ void weapon_select_close()
 	// unload bitmaps
 	bm_release(WeaponSelectMaskBitmap);
 
-	wl_unload_all_teams_icons();
-	wl_unload_all_anims();
-	// ...must be last...
-	wl_free_ship_class_data();
-
 	Selected_wl_class = -1;
 	Weapon_select_open = 0;
 

code/missionui/missionweaponchoice.h

@@ -28,7 +28,7 @@ void weapon_select_init();
 void weapon_select_common_init(bool API_Access = false);
 void weapon_select_do(float frametime);
 void weapon_select_close();
-void weapon_select_close_team();
+void weapon_select_common_close();
 
 void draw_3d_overhead_view(int model_num,
 	int ship_class,

code/missionui/redalert.cpp

@@ -368,7 +368,7 @@ void red_alert_close()
 		red_alert_voice_stop();
 		red_alert_voice_unload();
 
-		weapon_select_close_team();
+		weapon_select_common_close();
 
 		if (Background_bitmap >= 0) {
 			bm_release(Background_bitmap);

code/model/modelread.cpp

@@ -264,6 +264,13 @@ void model_unload(int modelnum, int force)
 		return;
 	}
 
+	// a full id that doesn't match the slot's occupant is a stale handle to a model that has already been
+	// freed; the slot has since been reused, so unloading it would take out an unrelated model
+	if ( modelnum >= MAX_POLYGON_MODELS && pm->id != modelnum )	{
+		Warning(LOCATION, "Ignoring unload of stale model id %d: slot %d now holds '%s' (id %d). Please backtrace and investigate.\n", modelnum, num, pm->filename, pm->id);
+		return;
+	}
+
 	Assert( pm->used_this_mission >= 0 );
 
 	if (!force && (--pm->used_this_mission > 0))

code/ship/ship.cpp

@@ -11669,15 +11669,12 @@ static void ship_model_change(int n, int ship_type)
 	objp = &Objects[sp->objnum];
 	pmi = model_get_instance(sp->model_instance_num);
 
-	// get new model
-	if (sip->model_num == -1) {
-		sip->model_num = model_load(sip->pof_file, sip);
-	}
+	// get new model; as in ship_create(), load even if the class already has one, so that this ship holds
+	// its own reference and the model survives a UI preview releasing the one it loaded
+	sip->model_num = model_load(sip->pof_file, sip);
 
-	if ( sip->cockpit_model_num == -1 ) {
-		if ( VALID_FNAME(sip->cockpit_pof_file) ) {
-			sip->cockpit_model_num = model_load(sip->cockpit_pof_file);
-		}
+	if ( VALID_FNAME(sip->cockpit_pof_file) ) {
+		sip->cockpit_model_num = model_load(sip->cockpit_pof_file);
 	}
 
 	pm = model_get(sip->model_num);