Source Code Project Mantis - FSSCP
View Issue Details
0002888FSSCPtablespublic2013-06-03 18:452014-12-03 22:57
ReporterFUBAR-BDHR 
Assigned ToGoober5000 
PrioritylowSeverityfeatureReproducibilityN/A
StatusresolvedResolutionreopened 
PlatformOSOS Version
Product Version3.6.19 
Target Version3.7.2Fixed in Version 
Summary0002888: Patch to allow for head .ani files without letters
DescriptionTBP 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.
TagsNo tags attached.
related to 0003109resolved Yarn Retail head ANIs no longer appearing 
Attached Filespatch no_suffix_head_anis.patch (1,997) 2013-06-03 18:45
http://scp.indiegames.us/mantis/file_download.php?file_id=2222&type=bug
diff 2888_r10908.diff (1,918) 2014-07-10 11:14
http://scp.indiegames.us/mantis/file_download.php?file_id=2460&type=bug
diff 2888_alt_2_r10911.diff (6,066) 2014-07-11 19:23
http://scp.indiegames.us/mantis/file_download.php?file_id=2462&type=bug
diff 2888_take_3.diff (2,789) 2014-07-25 12:05
http://scp.indiegames.us/mantis/file_download.php?file_id=2493&type=bug
patch missionmessage.cpp.patch (970) 2014-11-23 23:55
http://scp.indiegames.us/mantis/file_download.php?file_id=2624&type=bug

Notes
(0016050)
chief1983   
2014-07-09 17:34   
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.
(0016059)
The_E   
2014-07-10 04:47   
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.
(0016061)
chief1983   
2014-07-10 11:17   
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.
(0016062)
chief1983   
2014-07-10 13:06   
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.
(0016063)
chief1983   
2014-07-10 13:56   
Nevermind I'm too rusty for this stuff, that's not the issue.
(0016065)
chief1983   
2014-07-11 19:24   
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.
(0016090)
chief1983   
2014-07-17 22:21   
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.
(0016119)
chief1983   
2014-07-23 11:32   
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.
(0016289)
chief1983   
2014-09-23 11:17   
Fix committed to trunk@11079.
(0016310)
chief1983   
2014-09-28 10:32   
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.
(0016400)
Goober5000   
2014-11-23 21:45   
I think I can come up with a quick implementation of the originally proposed solution.
(0016401)
Goober5000   
2014-11-23 23:55   
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).
(0016404)
chief1983   
2014-11-24 10:11   
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.
(0016405)
Goober5000   
2014-11-24 20:14   
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.
(0016406)
chief1983   
2014-11-24 23:28   
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.
(0016407)
Goober5000   
2014-11-26 00:36   
My patch wouldn't have performance issues, because it uses the cached value of what the code has already done.
(0016408)
Goober5000   
2014-12-03 22:57   
Fix committed to trunk@11186.

Issue History
2013-06-03 18:45FUBAR-BDHRNew Issue
2013-06-03 18:45FUBAR-BDHRStatusnew => assigned
2013-06-03 18:45FUBAR-BDHRAssigned To => FUBAR-BDHR
2013-06-03 18:45FUBAR-BDHRFile Added: no_suffix_head_anis.patch
2013-06-03 18:45FUBAR-BDHRStatusassigned => code review
2014-07-09 17:34chief1983Note Added: 0016050
2014-07-10 04:47The_ENote Added: 0016059
2014-07-10 11:14chief1983File Added: 2888_r10908.diff
2014-07-10 11:17chief1983Note Added: 0016061
2014-07-10 13:06chief1983Note Added: 0016062
2014-07-10 13:56chief1983Note Added: 0016063
2014-07-11 19:23chief1983File Added: 2888_alt_2_r10911.diff
2014-07-11 19:24chief1983Note Added: 0016065
2014-07-17 22:21chief1983Note Added: 0016090
2014-07-23 11:32chief1983Note Added: 0016119
2014-07-25 10:55chief1983File Added: 2888_take_3.diff
2014-07-25 12:05chief1983File Deleted: 2888_take_3.diff
2014-07-25 12:05chief1983File Added: 2888_take_3.diff
2014-09-23 11:17chief1983Changeset attached => fs2open trunk r11079
2014-09-23 11:17chief1983Note Added: 0016289
2014-09-23 11:17chief1983Statuscode review => resolved
2014-09-23 11:17chief1983Resolutionopen => fixed
2014-09-26 22:29YarnRelationship addedrelated to 0003109
2014-09-28 10:32chief1983Assigned ToFUBAR-BDHR => chief1983
2014-09-28 10:32chief1983Note Added: 0016310
2014-09-28 10:32chief1983Statusresolved => feedback
2014-09-28 10:32chief1983Resolutionfixed => reopened
2014-09-28 10:32chief1983Statusfeedback => code review
2014-11-23 21:44Goober5000Changeset attached => fs2open trunk r11090
2014-11-23 21:45Goober5000Note Added: 0016400
2014-11-23 21:45Goober5000Assigned Tochief1983 => Goober5000
2014-11-23 21:45Goober5000Statuscode review => assigned
2014-11-23 23:55Goober5000File Added: missionmessage.cpp.patch
2014-11-23 23:55Goober5000Note Added: 0016401
2014-11-23 23:56Goober5000Statusassigned => code review
2014-11-24 10:11chief1983Note Added: 0016404
2014-11-24 20:14Goober5000Note Added: 0016405
2014-11-24 23:28chief1983Note Added: 0016406
2014-11-26 00:36Goober5000Note Added: 0016407
2014-12-03 22:57Goober5000Changeset attached => fs2open trunk r11186
2014-12-03 22:57Goober5000Note Added: 0016408
2014-12-03 22:57Goober5000Statuscode review => resolved