View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0002888 | FSSCP | tables | public | 2013-06-03 22:45 | 2014-12-04 03:57 |
Reporter | FUBAR-BDHR | Assigned To | Goober5000 | ||
Priority | low | Severity | feature | Reproducibility | N/A |
Status | resolved | Resolution | reopened | ||
Product Version | 3.6.19 | ||||
Target Version | 3.7.2 | ||||
Summary | 0002888: Patch to allow for head .ani files without letters | ||||
Description | TBP never used a, b, or c versions of head .ani files. To work around the issue it was released with 3-4 copies of a number of files. This is only done for ones used in messages.tbl. Others don't have the letter added unless they start with the word head which is possibly a bug (see below). This patch adds a game_settings.tbl option to eliminate the use of the a, b, c versions and just use the name specified. While looking into this I noticed a discrepancy in the way head .ani files starting with head are treated. Currently anything starting with head has the letters added to it. The code defines the extension that these are supposed to be added to as head- but does a -1 when doing the compare so the - gets removed. I don't know if it does this on purpose or is a bug. | ||||
Tags | No tags attached. | ||||
|
no_suffix_head_anis.patch (1,997 bytes)
Index: mission/missionmessage.cpp =================================================================== --- mission/missionmessage.cpp (revision 9689) +++ mission/missionmessage.cpp (working copy) @@ -1181,8 +1181,9 @@ // support ships use a wingman head. // terran command uses its own set of heads. + // Added option to always use the actual file for mods that don't have a,b & c, versions by enabling No_suffix_head_anis - FUBAR int subhead_selected = FALSE; - if ( (q->message_num < Num_builtin_messages) || !(_strnicmp(HEAD_PREFIX_STRING, ani_name, strlen(HEAD_PREFIX_STRING)-1)) ) { + if (!No_suffix_head_anis && ( (q->message_num < Num_builtin_messages) || !(_strnicmp(HEAD_PREFIX_STRING, ani_name, strlen(HEAD_PREFIX_STRING)-1)) )) { persona_index = m->persona_index; // if this ani should be converted to a terran command, set the persona to the command persona Index: mod_table/mod_table.cpp =================================================================== --- mod_table/mod_table.cpp (revision 9689) +++ mod_table/mod_table.cpp (working copy) @@ -24,8 +24,9 @@ int Default_detail_level = 3; // "very high" seems a reasonable default in 2012 -zookeeper bool Full_color_head_anis = false; bool Weapons_inherit_parent_collision_group = false; +bool No_suffix_head_anis = false; - void parse_mod_table(char *filename) { int rval; @@ -109,6 +110,9 @@ stuff_boolean(&temp); Full_color_head_anis = !temp; } + if (optional_string("$Do not add suffix to head animations:")) { + stuff_boolean(&No_suffix_head_anis); + } optional_string("#SEXP SETTINGS"); Index: mod_table/mod_table.h =================================================================== --- mod_table/mod_table.h (revision 9689) +++ mod_table/mod_table.h (working copy) @@ -21,5 +21,7 @@ extern int Default_detail_level; extern bool Full_color_head_anis; extern bool Weapons_inherit_parent_collision_group; +extern bool No_suffix_head_anis; void mod_table_init(); |
|
Would it be more reliable here, and avoid a new mod setting, to just check if the filenames without extensions exist, via cfile, and if so use those instead? I think I remember doing some similar magic back with WCS' 3d shockwaves to see what was available and adjust the flag behavior based on what content the mod actually provided. |
|
I agree with that. This is not something that I think needs to be exposed in an explicit setting, changing the search path so that the engine checks for both the suffixed and the unsuffixed versions should suffice. |
|
2888_r10908.diff (1,918 bytes)
Index: mission/missionmessage.cpp =================================================================== --- mission/missionmessage.cpp (revision 10908) +++ mission/missionmessage.cpp (working copy) @@ -1169,8 +1169,9 @@ // support ships use a wingman head. // terran command uses its own set of heads. + // Added option to always use the actual file for mods that don't have a,b & c, versions by enabling No_suffix_head_anis - FUBAR int subhead_selected = FALSE; - if ( (q->message_num < Num_builtin_messages) || !(_strnicmp(HEAD_PREFIX_STRING, ani_name, strlen(HEAD_PREFIX_STRING)-1)) ) { + if (!No_suffix_head_anis && ( (q->message_num < Num_builtin_messages) || !(_strnicmp(HEAD_PREFIX_STRING, ani_name, strlen(HEAD_PREFIX_STRING)-1)) )) { persona_index = m->persona_index; // if this ani should be converted to a terran command, set the persona to the command persona Index: mod_table/mod_table.cpp =================================================================== --- mod_table/mod_table.cpp (revision 10908) +++ mod_table/mod_table.cpp (working copy) @@ -31,6 +31,7 @@ float Briefing_window_FOV = 0.29375f; bool Disable_hc_message_ani = false; bool Red_alert_applies_to_delayed_ships = false; +bool No_suffix_head_anis = false; void parse_mod_table(const char *filename) @@ -126,6 +127,9 @@ stuff_boolean(&temp); Full_color_head_anis = !temp; } + if (optional_string("$Do not add suffix to head animations:")) { + stuff_boolean(&No_suffix_head_anis); + } optional_string("#SEXP SETTINGS"); Index: mod_table/mod_table.h =================================================================== --- mod_table/mod_table.h (revision 10908) +++ mod_table/mod_table.h (working copy) @@ -26,5 +26,6 @@ extern float Briefing_window_FOV; extern bool Disable_hc_message_ani; extern bool Red_alert_applies_to_delayed_ships; +extern bool No_suffix_head_anis; void mod_table_init(); |
|
Ok, I did update the original patch because it was too out of date to apply, it even said it was malformed it was so out of line. This one applies to current trunk now. However, I'm trying to understand how bypassing lines 1174 - 1226 in missionmessage.cpp is what actually accomplishes the intended goal here. |
|
Hmm, I think the bug FUBAR saw in the strnicmp isn't in the fact that it compares by removing the - at the end of the name - in fact, I think that code was put there to accomplish what this ticket wants. But, they're checking with !(_strnicmp()), which seems flawed since strcmp and family return three values - < 0 if string A is less than string B, 0 if they are equal, and > 0 if A is greater than B. I have a feeling that check should be changed to whether the strnicmp returns > 0, and this might actually fix itself. Not sure what other behavior that may affect though. I just get worried whenever I see a strcmp function checked as if it were a boolean return. |
|
Nevermind I'm too rusty for this stuff, that's not the issue. |
|
2888_alt_2_r10911.diff (6,066 bytes)
Index: code/bmpman/bmpman.cpp =================================================================== --- code/bmpman/bmpman.cpp (revision 10911) +++ code/bmpman/bmpman.cpp (working copy) @@ -48,7 +48,15 @@ #define BMPMAN_NDEBUG #endif +// Extension type lists +const ubyte bm_type_list[] = { BM_TYPE_DDS, BM_TYPE_TGA, BM_TYPE_PNG, BM_TYPE_JPG, BM_TYPE_PCX }; +const char *bm_ext_list[] = { ".dds", ".tga", ".png", ".jpg", ".pcx" }; +const int BM_NUM_TYPES = sizeof(bm_type_list) / sizeof(ubyte); +const ubyte bm_ani_type_list[] = {BM_TYPE_EFF, BM_TYPE_ANI}; +const char *bm_ani_ext_list[] = {".eff", ".ani"}; +const int BM_ANI_NUM_TYPES = sizeof(bm_ani_type_list) / sizeof(ubyte); + // globals int GLOWMAP = -1; int SPECMAP = -1; @@ -530,22 +538,18 @@ // Lets find out what type it is { - const int NUM_TYPES = 5; - const ubyte type_list[NUM_TYPES] = { BM_TYPE_DDS, BM_TYPE_TGA, BM_TYPE_PNG, BM_TYPE_JPG, BM_TYPE_PCX }; - const char *ext_list[NUM_TYPES] = { ".dds", ".tga", ".png", ".jpg", ".pcx" }; - // see if it's already loaded (checks for any type with filename) if ( bm_load_sub_fast(filename, &handle) ) return handle; // if we are still here then we need to fall back to a file-based search - int rval = bm_load_sub_slow(filename, NUM_TYPES, ext_list, &img_cfp); + int rval = bm_load_sub_slow(filename, BM_NUM_TYPES, bm_ext_list, &img_cfp); if (rval < 0) return -1; - strcat_s(filename, ext_list[rval]); - type = type_list[rval]; + strcat_s(filename, bm_ext_list[rval]); + type = bm_type_list[rval]; } Assert(type != BM_TYPE_NONE); @@ -845,9 +849,6 @@ ubyte type = BM_TYPE_NONE, eff_type = BM_TYPE_NONE, c_type = BM_TYPE_NONE; int bpp = 0, mm_lvl = 0, img_size = 0; char clean_name[MAX_FILENAME_LEN]; - const int NUM_TYPES = 2; - const ubyte type_list[NUM_TYPES] = {BM_TYPE_EFF, BM_TYPE_ANI}; - const char *ext_list[NUM_TYPES] = {".eff", ".ani"}; if ( !bm_inited ) bm_init(); @@ -909,13 +910,13 @@ } // if we are still here then we need to fall back to a file-based search - int rval = bm_load_sub_slow(filename, NUM_TYPES, ext_list, &img_cfp, dir_type); + int rval = bm_load_sub_slow(filename, BM_ANI_NUM_TYPES, bm_ani_ext_list, &img_cfp, dir_type); if (rval < 0) return -1; - strcat_s(filename, ext_list[rval]); - type = type_list[rval]; + strcat_s(filename, bm_ani_ext_list[rval]); + type = bm_ani_type_list[rval]; } // If we found an animation then there is an extra 5 char size limit to adhere to. We don't do this check earlier since it's only needed if we found an anim Index: code/bmpman/bmpman.h =================================================================== --- code/bmpman/bmpman.h (revision 10911) +++ code/bmpman/bmpman.h (working copy) @@ -68,6 +68,12 @@ extern int Bm_paging; +extern const ubyte bm_type_list[]; +extern const char *bm_ext_list[]; +extern const int BM_NUM_TYPES; +extern const ubyte bm_ani_type_list[]; +extern const char *bm_ani_ext_list[]; +extern const int BM_ANI_NUM_TYPES; void bm_init(); Index: code/graphics/generic.cpp =================================================================== --- code/graphics/generic.cpp (revision 10911) +++ code/graphics/generic.cpp (working copy) @@ -20,6 +20,11 @@ //we check background type to avoid messed up colours for ANI #define ANI_BPP_CHECK (ga->ani.bg_type == BM_TYPE_PCX) ? 16 : 32 +bool generic_anim_exists(const char *filename) +{ + return cf_exists_full_ext(filename, CF_TYPE_ANY, BM_ANI_NUM_TYPES, bm_ani_ext_list); +} + // Goober5000 int generic_anim_init_and_stream(generic_anim *anim, const char *anim_filename, ubyte bg_type, bool attempt_hi_res) { Index: code/graphics/generic.h =================================================================== --- code/graphics/generic.h (revision 10911) +++ code/graphics/generic.h (working copy) @@ -52,6 +52,7 @@ int bitmap_id; } generic_bitmap; +bool generic_anim_exists(const char *filename); int generic_anim_init_and_stream(generic_anim *anim, const char *anim_filename, ubyte bg_type, bool attempt_hi_res); void generic_anim_init(generic_anim *ga); void generic_anim_init(generic_anim *ga, const char *filename); Index: code/mission/missionmessage.cpp =================================================================== --- code/mission/missionmessage.cpp (revision 10911) +++ code/mission/missionmessage.cpp (working copy) @@ -194,6 +194,7 @@ static int Message_wave_muted; static int Message_wave_duration; static int Next_mute_time; +static bool No_suffix_head_anis; #define MAX_DISTORT_PATTERNS 2 #define MAX_DISTORT_LEVELS 6 @@ -650,6 +651,7 @@ { int rval, i; static int table_read = 0; + char prefixless_name[MAX_FILENAME_LEN], with_prefix_name[MAX_FILENAME_LEN]; if ( !table_read ) { Default_command_persona = -1; @@ -714,6 +716,15 @@ Personas[i].flags &= ~PERSONA_FLAG_USED; } + // If an anim with name HEAD_PREFIX_STRING minus the hyphen exists, and not + // HEAD_PREFIX_STRING + 'a', then enable 'No_suffix_head_anis'. + No_suffix_head_anis = false; + strncpy(prefixless_name, HEAD_PREFIX_STRING, strlen(HEAD_PREFIX_STRING) - 1); + sprintf(with_prefix_name, "%s%c", HEAD_PREFIX_STRING, 'a'); + if(generic_anim_exists(prefixless_name) && !generic_anim_exists(with_prefix_name)){ + No_suffix_head_anis = true; + } + Message_wave_muted = 0; Next_mute_time = 1; @@ -1169,8 +1180,9 @@ // support ships use a wingman head. // terran command uses its own set of heads. + // Added option to always use the actual file for mods that don't have a,b & c, versions by enabling No_suffix_head_anis - FUBAR int subhead_selected = FALSE; - if ( (q->message_num < Num_builtin_messages) || !(_strnicmp(HEAD_PREFIX_STRING, ani_name, strlen(HEAD_PREFIX_STRING)-1)) ) { + if (!No_suffix_head_anis && ( (q->message_num < Num_builtin_messages) || !(_strnicmp(HEAD_PREFIX_STRING, ani_name, strlen(HEAD_PREFIX_STRING)-1)) )) { persona_index = m->persona_index; // if this ani should be converted to a terran command, set the persona to the command persona |
|
Ok, I have a suggested patch that doesn't add a new mod_table option but I think will still accomplish what's needed here. FUBAR, would you like to verify this does what you need? I'm not quite sure I understand what was needed but I think I got it. |
|
I'm trying to fully understand the problem here. The original patch allows you to use a filename that starts with 'head' without the engine adding -a, -b, -c, etc to it. But I'm still confused as to what causes the request for the 'head' animation in the first place. If this is a hardcoded name that is requested by the engine at times, then I don't see why the suggested patch would not work, as either anyone is only going to provide 'head' with no suffix, or they should provide the full set the engine is hardcoded to look for. The original patch doesn't really allow for any more flexibility there, like head and head-a both or something. The mod setting would still force you pretty much one way or the other, same as my suggested patch does. If the request for 'head' that this is trying to solve is coming from a tabled animation filename though, couldn't something else besides 'head' just be used in the table and the filename? I think that checking for whether a mod provided the suffix-enabled set by checking for the first one, or whether they provided just one suffix-free animation would work well, and is as flexible as the original patch from what I can tell. With the original patch, I suppose if the problem is that the 'head' name comes from a table, you wouldn't be able to provide just 'head' and 'head-a' and use both those filenames in a table, but then if it is table-controlled I don't see why any code change here is necessary at all. Hope this is somewhat understandable, I'm just genuinely confused because I haven't seen the way the assets are configured that this is supposed to help simplify. |
|
Ok, after further discussion and review, I'm inclined to see that there's really not a better solution here. It's not just the head* files that get this added, it's also any built in message personas. So short of scanning for every built in message persona and setting a flag for each one somehow, there's really not a better way to handle this. It could also be a per-message flag maybe, in the messages.tbl, but that could still be done after this patch. |
|
2888_take_3.diff (2,789 bytes)
Index: code/mission/missionmessage.cpp =================================================================== --- code/mission/missionmessage.cpp (revision 10943) +++ code/mission/missionmessage.cpp (working copy) @@ -344,7 +344,7 @@ } // parses an individual message -void message_parse(bool importing_from_fsm) +void message_parse(bool importing_from_fsm, bool builtin) { MissionMessage msg; char persona_name[NAME_LENGTH]; @@ -467,6 +467,18 @@ } } + // Default append_suffix based on whether we are parsing messages.tbl or adding a custom message, + // then check for the flag. -Chief1983 + if(builtin) { + msg.append_suffix = true; + } + else { + msg.append_suffix = false; + } + if(optional_string("$Append Suffix")){ + stuff_boolean(&msg.append_suffix); + } + Num_messages++; Messages.push_back(msg); } @@ -579,7 +591,7 @@ required_string("#Messages"); while (required_string_either("#End", "$Name:")){ - message_parse(); + message_parse(false, true); } required_string("#End"); @@ -1169,8 +1181,9 @@ // support ships use a wingman head. // terran command uses its own set of heads. + // Added option to always use the actual file for mods that don't have a,b & c, versions by enabling $Append Suffix int subhead_selected = FALSE; - if ( (q->message_num < Num_builtin_messages) || !(_strnicmp(HEAD_PREFIX_STRING, ani_name, strlen(HEAD_PREFIX_STRING)-1)) ) { + if (m->append_suffix && ( (q->message_num < Num_builtin_messages) || !(_strnicmp(HEAD_PREFIX_STRING, ani_name, strlen(HEAD_PREFIX_STRING)-1)) )) { persona_index = m->persona_index; // if this ani should be converted to a terran command, set the persona to the command persona @@ -2287,6 +2300,7 @@ msg.multi_team = multi_team; msg.avi_info.index = -1; msg.wave_info.index = -1; + msg.append_suffix = false; Messages.push_back(msg); Num_messages++; Index: code/mission/missionmessage.h =================================================================== --- code/mission/missionmessage.h (revision 10943) +++ code/mission/missionmessage.h (working copy) @@ -124,6 +124,7 @@ int multi_team; // multiplayer team filter (important for TvT only) int mood; SCP_vector<int> excluded_moods; + bool append_suffix; // unions for avi/wave information. Because of issues with Fred, we are using // the union to specify either the index into the avi or wave arrays above, @@ -189,7 +190,7 @@ // function to parse a message from either messages.tbl or the mission file. Both files have the // exact same format, so this function just gets reused in both instances. -void message_parse(bool importing_from_fsm = false); +void message_parse(bool importing_from_fsm = false, bool builtin = false); void persona_parse(); void messages_init(); |
|
Fix committed to trunk@11079. |
|
Figured I didn't get that quite right. The per-message thing might not work, at least not without a different default and FRED support. |
|
I think I can come up with a quick implementation of the originally proposed solution. |
|
missionmessage.cpp.patch (970 bytes)
Index: code/mission/missionmessage.cpp =================================================================== --- code/mission/missionmessage.cpp (revision 11178) +++ code/mission/missionmessage.cpp (working copy) @@ -1169,8 +1169,9 @@ // support ships use a wingman head. // terran command uses its own set of heads. - int subhead_selected = FALSE; - if ( (q->message_num < Num_builtin_messages) || !(_strnicmp(HEAD_PREFIX_STRING, ani_name, strlen(HEAD_PREFIX_STRING)-1)) ) { + if ( (anim_info->anim_data.first_frame < 0) && // note, first_frame will be >= 0 when ani is an existing file, and will be < 0 when the file does not exist and needs a, b, or c appended + ((q->message_num < Num_builtin_messages) || !(_strnicmp(HEAD_PREFIX_STRING, ani_name, strlen(HEAD_PREFIX_STRING)-1))) ) { + int subhead_selected = FALSE; persona_index = m->persona_index; // if this ani should be converted to a terran command, set the persona to the command persona |
|
Okay, have a look at the patch I just uploaded. This relies on the fact that the game has previously attempted to load the ANI specified by the message, so it already knows if it exists (first_frame >= 0) or not (first_frame < 0). |
|
Which originally proposed solution? FUBAR's was already attached as the first patch, and my first proposed solution seemed to be fundamentally flawed, due to my limited understanding of head anis and their interaction with the messages.tbl and also mission files. |
|
I meant the first comment: "Would it be more reliable here, and avoid a new mod setting, to just check if the filenames without extensions exist, via cfile, and if so use those instead?" The message code checks to see if the filename exists whenever it's provided with a new ANI. So if the message code is passed "Head-VP1" and first_frame is -1, it knows that Head-VP1.ani does not exist and that it should look for Head-VP1a.ani, etc. |
|
Yeah, I still think I found something flawed with that approach. Maybe it was just performance issues though since they would be hitting the disk in mission for different animations. |
|
My patch wouldn't have performance issues, because it uses the cached value of what the code has already done. |
|
Fix committed to trunk@11186. |
fs2open: trunk r11079 2014-09-23 11:53 Ported: N/A Details Diff |
Fix for Mantis 2888 - add Suffix support to disable suffixes for messages on a per message basis in messages.tbl. See the wiki. |
Affected Issues 0002888 |
|
mod - /trunk/fs2_open/code/mission/missionmessage.cpp | Diff File | ||
mod - /trunk/fs2_open/code/mission/missionmessage.h | Diff File | ||
fs2open: trunk r11090 2014-09-28 11:06 Ported: N/A Details Diff |
Fix Mantis 3109: Revert r11079 and reopen 0002888 since I didn't realize that Fred messages also often use built-in suffix appending. Need to either use FUBAR's original solution or re-engineer something else. |
Affected Issues 0002888, 0003109 |
|
mod - /trunk/fs2_open/code/mission/missionmessage.cpp | Diff File | ||
mod - /trunk/fs2_open/code/mission/missionmessage.h | Diff File | ||
fs2open: trunk r11186 2014-12-03 23:45 Ported: N/A Details Diff |
this should fix Mantis 0002888 (allow for head.ani files without letter suffixes) |
Affected Issues 0002888 |
|
mod - /trunk/fs2_open/code/mission/missionmessage.cpp | Diff File |
Date Modified | Username | Field | Change |
---|---|---|---|
2013-06-03 22:45 | FUBAR-BDHR | New Issue | |
2013-06-03 22:45 | FUBAR-BDHR | Status | new => assigned |
2013-06-03 22:45 | FUBAR-BDHR | Assigned To | => FUBAR-BDHR |
2013-06-03 22:45 | FUBAR-BDHR | File Added: no_suffix_head_anis.patch | |
2013-06-03 22:45 | FUBAR-BDHR | Status | assigned => code review |
2014-07-09 21:34 | chief1983 | Note Added: 0016050 | |
2014-07-10 08:47 | The_E | Note Added: 0016059 | |
2014-07-10 15:14 | chief1983 | File Added: 2888_r10908.diff | |
2014-07-10 15:17 | chief1983 | Note Added: 0016061 | |
2014-07-10 17:06 | chief1983 | Note Added: 0016062 | |
2014-07-10 17:56 | chief1983 | Note Added: 0016063 | |
2014-07-11 23:23 | chief1983 | File Added: 2888_alt_2_r10911.diff | |
2014-07-11 23:24 | chief1983 | Note Added: 0016065 | |
2014-07-18 02:21 | chief1983 | Note Added: 0016090 | |
2014-07-23 15:32 | chief1983 | Note Added: 0016119 | |
2014-07-25 14:55 | chief1983 | File Added: 2888_take_3.diff | |
2014-07-25 16:05 | chief1983 | File Deleted: 2888_take_3.diff | |
2014-07-25 16:05 | chief1983 | File Added: 2888_take_3.diff | |
2014-09-23 15:17 | chief1983 | Changeset attached | => fs2open trunk r11079 |
2014-09-23 15:17 | chief1983 | Note Added: 0016289 | |
2014-09-23 15:17 | chief1983 | Status | code review => resolved |
2014-09-23 15:17 | chief1983 | Resolution | open => fixed |
2014-09-27 02:29 | Yarn | Relationship added | related to 0003109 |
2014-09-28 14:32 | chief1983 | Assigned To | FUBAR-BDHR => chief1983 |
2014-09-28 14:32 | chief1983 | Note Added: 0016310 | |
2014-09-28 14:32 | chief1983 | Status | resolved => feedback |
2014-09-28 14:32 | chief1983 | Resolution | fixed => reopened |
2014-09-28 14:32 | chief1983 | Status | feedback => code review |
2014-11-24 02:44 | Goober5000 | Changeset attached | => fs2open trunk r11090 |
2014-11-24 02:45 | Goober5000 | Note Added: 0016400 | |
2014-11-24 02:45 | Goober5000 | Assigned To | chief1983 => Goober5000 |
2014-11-24 02:45 | Goober5000 | Status | code review => assigned |
2014-11-24 04:55 | Goober5000 | File Added: missionmessage.cpp.patch | |
2014-11-24 04:55 | Goober5000 | Note Added: 0016401 | |
2014-11-24 04:56 | Goober5000 | Status | assigned => code review |
2014-11-24 15:11 | chief1983 | Note Added: 0016404 | |
2014-11-25 01:14 | Goober5000 | Note Added: 0016405 | |
2014-11-25 04:28 | chief1983 | Note Added: 0016406 | |
2014-11-26 05:36 | Goober5000 | Note Added: 0016407 | |
2014-12-04 03:57 | Goober5000 | Changeset attached | => fs2open trunk r11186 |
2014-12-04 03:57 | Goober5000 | Note Added: 0016408 | |
2014-12-04 03:57 | Goober5000 | Status | code review => resolved |