From 4bf806c485c2cd8dfb7000ee93f616208a95c8c2 Mon Sep 17 00:00:00 2001 From: Goober5000 Date: Fri, 9 Feb 2024 17:00:45 -0500 Subject: [PATCH] make model file checks consistent The general expectation is that model filenames can be specified as 'none' or '' 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 | 2 +- code/missionui/missionweaponchoice.cpp | 8 ++++---- code/model/modelread.cpp | 7 ++++++- code/ship/ship.cpp | 25 +++++++++++++++---------- code/weapon/shockwave.cpp | 10 +++++----- code/weapon/weapons.cpp | 23 ++++++++++++++++++----- 6 files changed, 49 insertions(+), 26 deletions(-) diff --git a/code/globalincs/pstypes.h b/code/globalincs/pstypes.h index 10b0df75bd0..9176837d8d1 100644 --- a/code/globalincs/pstypes.h +++ b/code/globalincs/pstypes.h @@ -453,7 +453,7 @@ class camid // - is not "none" // - is not "" inline bool VALID_FNAME(const char* x) { - return strlen((x)) && stricmp((x), "none") != 0 && stricmp((x), "") != 0; + return (x[0] != '\0') && stricmp(x, "none") != 0 && stricmp(x, "") != 0; } /** * @brief Checks if the specified string may be a valid file name diff --git a/code/missionui/missionweaponchoice.cpp b/code/missionui/missionweaponchoice.cpp index 3cb623ec11b..6ed1677b09c 100644 --- a/code/missionui/missionweaponchoice.cpp +++ b/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) { diff --git a/code/model/modelread.cpp b/code/model/modelread.cpp index 97c1fc2f606..37b07d80ab6 100644 --- a/code/model/modelread.cpp +++ b/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 )); diff --git a/code/ship/ship.cpp b/code/ship/ship.cpp index 5ae927b0d57..d7355ef7628 100644 --- a/code/ship/ship.cpp +++ b/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); } } diff --git a/code/weapon/shockwave.cpp b/code/weapon/shockwave.cpp index 5c2e0ced8c7..4df74fa2244 100644 --- a/code/weapon/shockwave.cpp +++ b/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 "" + // shockwave_load() will return -1 if the filename is "" or "none" or "" // 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); } diff --git a/code/weapon/weapons.cpp b/code/weapon/weapons.cpp index 77186437703..d821cb2900d 100644 --- a/code/weapon/weapons.cpp +++ b/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)