make model file checks consistent

The general expectation is that model filenames can be specified as 'none' or '<none>' for no model, but as discovered by no tea, this was not consistent across the code base.  For example, `$Model File: none` was handled as expected but `$External Model File: none` was not.  This PR makes the handling consistent for all ship, weapon, and shockwave POF files, and also adds some extra checks when models are loaded.

code/globalincs/pstypes.h

@@ -453,7 +453,7 @@ class camid
 //  - is not "none"
 //  - is not "<none>"
 inline bool VALID_FNAME(const char* x) {
-	return strlen((x)) && stricmp((x), "none") != 0 && stricmp((x), "<none>") != 0;
+	return (x[0] != '\0') && stricmp(x, "none") != 0 && stricmp(x, "<none>") != 0;
 }
 /**
  * @brief Checks if the specified string may be a valid file name

code/missionui/missionweaponchoice.cpp

@@ -1332,7 +1332,7 @@ void wl_load_icons(int weapon_class)
 
 	icon = &Wl_icons[weapon_class];
 
-	if (!Use_3d_weapon_icons || (wip->render_type == WRT_LASER && !strlen(wip->tech_model)))
+	if (!Use_3d_weapon_icons || (wip->render_type == WRT_LASER && !VALID_FNAME(wip->tech_model)))
 	{
 		first_frame = bm_load_animation(Weapon_info[weapon_class].icon_filename, &num_frames, nullptr, nullptr, nullptr, false, CF_TYPE_INTERFACE);
 
@@ -1343,9 +1343,9 @@ void wl_load_icons(int weapon_class)
 
 	multi_send_anti_timeout_ping();
 
-	if ( icon->model_index == -1 && ( ( strlen(wip->tech_model) && !strlen(wip->anim_filename) ) || (first_frame == -1) ) )
+	if ( icon->model_index == -1 && ( ( VALID_FNAME(wip->tech_model) && !VALID_FNAME(wip->anim_filename) ) || (first_frame == -1) ) )
 	{
-		if(strlen(wip->tech_model))
+		if(VALID_FNAME(wip->tech_model))
 		{
 			icon->model_index = model_load(wip->tech_model, 0, NULL, 0);
 		}
@@ -2742,7 +2742,7 @@ void weapon_select_do(float frametime)
 		weapon_info *wip = &Weapon_info[Selected_wl_class];
 
 		//Get the model
-		if (strlen(wip->tech_model)) {
+		if (VALID_FNAME(wip->tech_model)) {
 			modelIdx = model_load(wip->tech_model, 0, NULL, 0);
 		}
 		if (wip->render_type != WRT_LASER && modelIdx == -1) {

code/model/modelread.cpp

@@ -3259,7 +3259,7 @@ void model_load_texture(polymodel *pm, int i, char *file)
 //returns the number of the pof tech model if specified, otherwise number of pof model
 int model_load(ship_info* sip, bool prefer_tech_model)
 {
-	if (prefer_tech_model && sip->pof_file_tech[0] != '\0') {
+	if (prefer_tech_model && VALID_FNAME(sip->pof_file_tech)) {
 		// This cannot load into sip->subsystems, as this will overwrite the subsystems model_num to the
 		// techroom model, which is decidedly wrong for the mission itself.
 		return model_load(sip->pof_file_tech, 0, nullptr);
@@ -3298,6 +3298,11 @@ int model_load(const  char* filename, int n_subsystems, model_subsystem* subsyst
 		return -1;
 	}
 
+	// Valid file
+	if (!VALID_FNAME(filename)) {
+		return -1;
+	}
+
 	TRACE_SCOPE(tracing::LoadModelFile);
 
 	mprintf(( "Loading model '%s' into slot '%i'\n", filename, num ));

code/ship/ship.cpp

@@ -2941,7 +2941,7 @@ static void parse_ship_values(ship_info* sip, const bool is_template, const bool
 
 		// Goober5000 - if this is a modular table, and we're replacing an existing file name, and the file doesn't exist, don't replace it
 		if (replace)
-			if (sip->cockpit_pof_file[0] != '\0')
+			if (VALID_FNAME(sip->cockpit_pof_file))
 				if (!model_exists(temp))
 					valid = false;
 
@@ -3021,7 +3021,7 @@ static void parse_ship_values(ship_info* sip, const bool is_template, const bool
 
 		// Goober5000 - if this is a modular table, and we're replacing an existing file name, and the file doesn't exist, don't replace it
 		if (replace)
-			if (sip->pof_file[0] != '\0')
+			if (VALID_FNAME(sip->pof_file))
 				if (!model_exists(temp))
 					valid = false;
 
@@ -3041,7 +3041,7 @@ static void parse_ship_values(ship_info* sip, const bool is_template, const bool
 
 		// if this is a modular table, and we're replacing an existing file name, and the file doesn't exist, don't replace it
 		if (replace)
-			if (sip->pof_file_tech[0] != '\0')
+			if (VALID_FNAME(sip->pof_file_tech))
 				if (!cf_exists_full(temp, CF_TYPE_MODELS))
 					valid = false;
 
@@ -3102,7 +3102,7 @@ static void parse_ship_values(ship_info* sip, const bool is_template, const bool
 
 		// Goober5000 - if this is a modular table, and we're replacing an existing file name, and the file doesn't exist, don't replace it
 		if (replace)
-			if (sip->pof_file_hud[0] != '\0')
+			if (VALID_FNAME(sip->pof_file_hud))
 				if (!cf_exists_full(temp, CF_TYPE_MODELS))
 					valid = false;
 
@@ -3380,7 +3380,7 @@ static void parse_ship_values(ship_info* sip, const bool is_template, const bool
 			bool valid = true;
 
 			if (replace)
-				if (sip->generic_debris_pof_file[0] != '\0')
+				if (VALID_FNAME(sip->generic_debris_pof_file))
 					if (!cf_exists_full(temp, CF_TYPE_MODELS))
 						valid = false;
 
@@ -4346,7 +4346,7 @@ static void parse_ship_values(ship_info* sip, const bool is_template, const bool
 	{
 		stuff_vec3d(&sip->closeup_pos);
 	}
-	else if (first_time && strlen(sip->pof_file))
+	else if (first_time && VALID_FNAME(sip->pof_file))
 	{
 		//Calculate from the model file. This is inefficient, but whatever
 		int model_idx = model_load(sip->pof_file, 0, NULL);
@@ -10797,15 +10797,20 @@ int ship_create(matrix* orient, vec3d* pos, int ship_type, const char* ship_name
 	shipp->clear();
 	shipp->orders_allowed_against = ship_set_default_orders_against();
 
+	if (!VALID_FNAME(sip->pof_file))
+	{
+		Error(LOCATION, "Cannot create ship %s; pof file is not valid", sip->name);
+		return -1;
+	}
 	sip->model_num = model_load(sip->pof_file, sip->n_subsystems, &sip->subsystems[0]);		// use the highest detail level
 
-	if(strlen(sip->cockpit_pof_file))
+	if(VALID_FNAME(sip->cockpit_pof_file))
 	{
 		sip->cockpit_model_num = model_load(sip->cockpit_pof_file, 0, NULL);
 	}
 
 	// maybe load an optional hud target model
-	if(strlen(sip->pof_file_hud)){
+	if(VALID_FNAME(sip->pof_file_hud)){
 		// check to see if a "real" ship uses this model. if so, load it up for him so that subsystems are setup properly
 		for(auto it = Ship_info.begin(); it != Ship_info.end(); ++it){
 			if(!stricmp(it->pof_file, sip->pof_file_hud)){
@@ -10817,7 +10822,7 @@ int ship_create(matrix* orient, vec3d* pos, int ship_type, const char* ship_name
 		sip->model_num_hud = model_load(sip->pof_file_hud, 0, NULL);
 	}
 
-	if (strlen(sip->generic_debris_pof_file)) {
+	if (VALID_FNAME(sip->generic_debris_pof_file)) {
 		sip->generic_debris_model_num = model_load(sip->generic_debris_pof_file, 0, nullptr);
 		if (sip->generic_debris_model_num >= 0) {
 			polymodel* pm = model_get(sip->generic_debris_model_num);
@@ -11048,7 +11053,7 @@ static void ship_model_change(int n, int ship_type)
 	}
 
 	if ( sip->cockpit_model_num == -1 ) {
-		if ( strlen(sip->cockpit_pof_file) ) {
+		if ( VALID_FNAME(sip->cockpit_pof_file) ) {
 			sip->cockpit_model_num = model_load(sip->cockpit_pof_file, 0, NULL);
 		}
 	}

code/weapon/shockwave.cpp

@@ -93,10 +93,10 @@ int shockwave_create(int parent_objnum, vec3d* pos, shockwave_create_info* sci,
 
 	// try 2D shockwave first, then fall back to 3D, then fall back to default of either
 	// this should be pretty fool-proof and allow quick change between 2D and 3D effects
-	if ( strlen(sci->name) )
+	if ( VALID_FNAME(sci->name) )
 		info_index = shockwave_load(sci->name, false);
 
-	if ( (info_index < 0) && strlen(sci->pof_name) )
+	if ( (info_index < 0) && VALID_FNAME(sci->pof_name) )
 		info_index = shockwave_load(sci->pof_name, true);
 
 	if (info_index < 0) {
@@ -752,13 +752,13 @@ void shockwave_create_info_load(shockwave_create_info *sci)
 {
 	int i = -1;
 
-	// shockwave_load() will return -1 if the filename is "none" or "<none>"
+	// shockwave_load() will return -1 if the filename is "" or "none" or "<none>"
 	// checking for that case lets us handle a situation where a 2D shockwave
 	// of "none" was specified and a valid 3D shockwave was specified
 
-	if ( strlen(sci->name) )
+	if ( VALID_FNAME(sci->name) )
 		i = shockwave_load(sci->name, false);
 
-	if ( (i < 0) && strlen(sci->pof_name) )
+	if ( (i < 0) && VALID_FNAME(sci->pof_name) )
 		shockwave_load(sci->pof_name, true);
 }

code/weapon/weapons.cpp

@@ -1914,10 +1914,18 @@ int parse_weapon(int subtype, bool replace, const char *filename)
 		}
 	}
 
+	// this was added in commit 9fea22d551 and is specifically for allowing countermeasures to be parsed using the weapons code;
+	// but it should be handled the same as $Model file:
 	if(optional_string("$Model:"))
 	{
-		wip->render_type = WRT_POF;
 		stuff_string(wip->pofbitmap_name, F_NAME, MAX_FILENAME_LEN);
+
+		if (VALID_FNAME(wip->pofbitmap_name))
+			wip->render_type = WRT_POF;
+		else
+			wip->render_type = WRT_NONE;
+
+		diag_printf("Model pof file -- %s\n", wip->pofbitmap_name );
 	}
 
 	// handle rearm rate - modified by Goober5000
@@ -6342,10 +6350,15 @@ int weapon_create( const vec3d *pos, const matrix *porient, int weapon_type, int
 
 	// make sure we are loaded and useable
 	if ( (wip->render_type == WRT_POF) && (wip->model_num < 0) ) {
+		if (!VALID_FNAME(wip->pofbitmap_name)) {
+			Error(LOCATION, "Cannot create weapon %s; pof file is not valid", wip->name);
+			return -1;
+		}
+
 		wip->model_num = model_load(wip->pofbitmap_name, 0, NULL);
 
 		if (wip->model_num < 0) {
-			Int3();
+			Error(LOCATION, "Cannot create weapon %s; model file %s could not be loaded", wip->name, wip->pofbitmap_name);
 			return -1;
 		}
 	}
@@ -7784,7 +7797,7 @@ void weapons_page_in()
 
 		wip->external_model_num = -1;
 
-		if ( strlen(wip->external_model_name) )
+		if (VALID_FNAME(wip->external_model_name))
 			wip->external_model_num = model_load( wip->external_model_name, 0, NULL );
 
 		if (wip->external_model_num == -1)
@@ -7877,7 +7890,7 @@ void weapons_page_in_cheats()
 
 		wip->external_model_num = -1;
 
-		if ( strlen(wip->external_model_name) )
+		if (VALID_FNAME(wip->external_model_name))
 			wip->external_model_num = model_load( wip->external_model_name, 0, NULL );
 
 		if (wip->external_model_num == -1)
@@ -7975,7 +7988,7 @@ bool weapon_page_in(int weapon_type)
 
 		wip->external_model_num = -1;
 
-		if (strlen(wip->external_model_name))
+		if (VALID_FNAME(wip->external_model_name))
 			wip->external_model_num = model_load(wip->external_model_name, 0, NULL);
 
 		if (wip->external_model_num == -1)