View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0002901 | FSSCP | Pilot data | public | 2013-07-08 05:11 | 2013-08-14 13:50 |
Reporter | niffiwan | Assigned To | niffiwan | ||
Priority | high | Severity | major | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Product Version | 3.7.0 RC2 | ||||
Target Version | 3.7.0 | Fixed in Version | 3.7.0 | ||
Summary | 0002901: pilot conversion failures | ||||
Description | As reported here: http://www.hard-light.net/forums/index.php?topic=84445.0 http://www.hard-light.net/forums/index.php?topic=83889.msg1700943#msg1700943 Multiple pilots provided fail to convert. Each has a different error. Occurs on both r9710 & r9584 (just after pilot code went to trunk) | ||||
Steps To Reproduce | Add relevant .pl2 & .cs2 pilotfiles to data/players/single/inferno Start FSO Debug with no mods (or mediavps_3612) selected Observe errors in log Observe that pilots campaign progress is gone Observe that pilots stats (kills/shots/etc) are gone | ||||
Additional Information | type (d) - but custom mod in use (for both pilots) PILOT: Beginning pilot file conversion... PL2 => Converting '/home/me/.fs2_open/data/players/single/inferno/[realnameremoved].pl2'... PLR => Conversion complete! CS2 => Converting '/home/me/.fs2_open/data/players/single/inferno/[realnameremoved].FreeSpace2.cs2'... CS2 => Import ERROR: Data mismatch (ship lookup)! PL2 => Converting '/home/me/.fs2_open/data/players/single/inferno/markoshark.pl2'... PLR => Conversion complete! CS2 => Converting '/home/me/.fs2_open/data/players/single/inferno/markoshark.FreeSpace2.cs2'... CS2 => Import ERROR: Data mismatch (weapon lookup)! ============================================ (Fixed / (a)) PL2 => Converting '/home/me/.fs2_open/data/players/single/inferno/Sacro.pl2'... PLR => Conversion complete! CS2 => Converting '/home/me/.fs2_open/data/players/single/inferno/Sacro.FreeSpace2.cs2'... CS2 => Import ERROR: Data failure (RedAlert-ship)! PILOT: Pilot file conversion complete! This pilot is a good example of error b) & c) ============================================ status summary: Also - the current patch in review only fixes one of potentially several pilot conversion bugs, i.e. a) complete conversion fail due to error reading the mainhall There's still these to investigate/fix: b) loss of kills/hit% stats during conversion c) loss of medals during conversion d) complete conversion fail with ship/weapon reference error | ||||
Tags | No tags attached. | ||||
|
Will take a look this week/weekend. |
|
If you want - but I'm already having a look as well :) |
|
Sacro's is caused by this tripping when ras.name is read as a null from the file, thus causing (ras.ship_class >= csg->ship_list.size()) because ras.ship_class is reading garbage values Ras.name is read as null because the count variable is ridiculously large, i.e. count = 167772160. The actual corruption may be introduced even earlier, as the temp value in pilotfile_convert::csg_import() is corrupt before entering pilotfile_convert::csg_import_red_alert() void pilotfile_convert::csg_import_red_alert() { ... int ship_list_size = (int)csg->ship_list.size(); ... for (idx = 0; idx < count; idx++) { red_alert_ship_status ras; ... ras.ship_class = cfread_int(cfp); if (ras.ship_class >= ship_list_size) { throw "Data failure (RedAlert-ship)!"; } ... } } |
|
Yeah, I suspect that there's something in the 3.6.18 .cs2 not being read correctly by something before this section: cfread_string(temp, NAME_LENGTH, cfp); csg->main_hall = temp; csg_import_red_alert(); and that leaves the cfp file offset in the "wrong place" == bad data reads into temp, count, etc. I recall that some commits removed various inferno related code, I wonder if the inferno var now isn't being set correctly and thus this .cs2 is being treated like a non-inferno pilot. Hmmm... well that inferno code wasn't removed, but this looks slightly odd & needs a closer look - is Get_file_list_child reset by cf_get_file_list? void convert_pilot_files() ... // get list of old pilots which may need converting, starting with inferno pilots Get_file_list_child = "inferno"; cf_get_file_list(old_files, CF_TYPE_SINGLE_PLAYERS, "*.pl2"); inf_count = old_files.size(); cf_get_file_list(old_files, CF_TYPE_SINGLE_PLAYERS, "*.pl2"); Yes, it is reset to NULL. Hmmm... |
|
OK - I think this commit is the problem: r8453 It changes the behaviour of csg_import to treat the mainhall as a string, not as the previous ubyte. Given that the import needs to read data created by 3.6.18 & prior (i.e. before modular mainhalls) I think this small section needs to be reverted. I'll test that now. csg->main_hall = cfread_ubyte(cfp); //cfread_string(temp, NAME_LENGTH, cfp); //csg->main_hall = temp; And yep - that looks like it :D |
|
mantis2901-svn.patch (397 bytes)
Index: code/pilotfile/csg_convert.cpp =================================================================== --- code/pilotfile/csg_convert.cpp (revision 9720) +++ code/pilotfile/csg_convert.cpp (working copy) @@ -642,8 +642,7 @@ csg_import_missions(inferno); - cfread_string(temp, NAME_LENGTH, cfp); - csg->main_hall = temp; + csg->main_hall = cfread_ubyte(cfp); csg_import_red_alert(); |
|
Patch for one issue is ready - the pilot conversion process needs to use the old way of reading the mainhall ubyte, not the new string. |
|
Too hasty... 1) need to remove a now unused variable (temp) 2) need to check that the csg->main_hall will handle being assigned a ubyte without side-effects. Worked in my test with Sacro's pilot, but I need to double check to be sure. Also added CommanderDJ to the monitor list to get his opinion on 2) given that he implemented modular mainhalls |
|
Apologies for my tardiness. At a glance this looks okay to me, but I'll take a proper look at this in the the next day or two. |
|
I've had a proper look at this, and I think this should be okay! |
|
Well, now, wait a minute. In order for both the old main hall code and the new modular main hall code to work, the code has to be able to handle reading both ubytes and strings from that field. N'est-ce pas? |
|
Well, I'm pretty sure it doesn't. The old main hall code is <= 3.6.18 only (ubyte / old pilotfile format). The new mainhall code is >= 3.6.19 only (string / new pilotfile format). The pilot conversion is the only code that needs to read a ubyte & output a string. The normal pilot reading code in both <= 3.6.18 & >= 3.6.19 just need to deal with their respective / independent formats. Have I missed something here? |
|
This was my thinking as well. Assuming the conversion only happens once per pilot and that it only happens on old pilots (someone correct me if these assumptions do not hold), the mainhall field will always be a ubyte during the conversion process. |
|
Oh, so the conversion code is distinct from the regular pilot code? I thought they overlapped. Separating them does make more sense. |
|
Fix committed to trunk@9730. |
|
Hmmm.. I was trying to avoid the auto-close hook :) |
|
mantis2901b-svn.patch (1,489 bytes)
Index: code/pilotfile/csg_convert.cpp =================================================================== --- code/pilotfile/csg_convert.cpp (revision 9739) +++ code/pilotfile/csg_convert.cpp (working copy) @@ -564,10 +564,10 @@ } // loadout info - for (idx = 0; idx < 12; idx++) { + for (idx = 0; idx < MAX_WSS_SLOTS_CONV; idx++) { csg->loadout.slot[idx].ship_index = cfread_int(cfp); - for (j = 0; j < 12; j++) { + for (j = 0; j < MAX_SHIP_WEAPONS_CONV; j++) { csg->loadout.slot[idx].wep[j] = cfread_int(cfp); csg->loadout.slot[idx].wep_count[j] = cfread_int(cfp); } Index: code/pilotfile/pilotfile_convert.h =================================================================== --- code/pilotfile/pilotfile_convert.h (revision 9739) +++ code/pilotfile/pilotfile_convert.h (working copy) @@ -29,6 +29,8 @@ #include "pilotfile/pilotfile.h" +static const int MAX_WSS_SLOTS_CONV = 12; // 3 wings * 4 slots +static const int MAX_SHIP_WEAPONS_CONV = 7; // 3 primary + 4 secondary typedef struct index_list_t { SCP_string name; @@ -88,8 +90,8 @@ typedef struct wss_unit_conv_t { int ship_index; - int wep[12]; - int wep_count[12]; + int wep[MAX_SHIP_WEAPONS_CONV]; + int wep_count[MAX_SHIP_WEAPONS_CONV]; wss_unit_conv_t() : ship_index(-1) @@ -101,7 +103,7 @@ SCP_string filename; SCP_string last_modified; - wss_unit_conv_t slot[12]; + wss_unit_conv_t slot[MAX_WSS_SLOTS_CONV]; SCP_vector<int> weapon_pool; SCP_vector<int> ship_pool; |
|
mantis2901b-svn.patch should fix the medals/stats conversion issue. I've successfully tested with three pilots, Sacro + two of mine. Further testing & review would be great. |
|
With the patches in place, the conversion completes for my pilot as well. I'm getting some parsing errors/warnings later on, though. I was just messing about in the mainhall and the barracks to check the stats. MVE: Buffer underun (First is normal) Got event GS_EVENT_GAME_INIT (49) in state NOT A VALID STATE (0) PLR => Unable to open 'z64555S.plr' for reading! PLR => Verifying 'Default.plr' with version 1... PLR => Parsing: Flags.. PLR => (0x0001) Attempted to read 4-byte(s) beyond length limit CSG => Loading 'z64555.FreeSpace2.csg' with version 2... CSG => Parsing: Flags... CSG => Warning: (0x0001) Short read, information may have been lost! |
|
mantis2901c-svn.patch (1,527 bytes)
Index: code/pilotfile/csg_convert.cpp =================================================================== --- code/pilotfile/csg_convert.cpp (revision 9743) +++ code/pilotfile/csg_convert.cpp (working copy) @@ -686,12 +686,6 @@ // tips cfwrite_ubyte((ubyte)plr->tips, cfp); - // mainhall - cfwrite_string(const_cast<char*>(csg->main_hall.c_str()), cfp); - - // cutscenes - cfwrite_int(csg->cutscenes, cfp); - endSection(); } Index: code/pilotfile/pilotfile_convert.h =================================================================== --- code/pilotfile/pilotfile_convert.h (revision 9743) +++ code/pilotfile/pilotfile_convert.h (working copy) @@ -126,10 +126,9 @@ // not carried over, just for reference during conversion process int version; - int is_multi; - // basic flags and settings + int is_multi; int tips; int rank; int skill_level; Index: code/pilotfile/plr_convert.cpp =================================================================== --- code/pilotfile/plr_convert.cpp (revision 9743) +++ code/pilotfile/plr_convert.cpp (working copy) @@ -421,8 +421,8 @@ throw "Unsupported file version!"; } - // multi flag, don't need it - cfread_ubyte(cfp); + // multi flag + plr->is_multi = (int)cfread_ubyte(cfp); // rank plr->rank = cfread_int(cfp); @@ -549,6 +549,9 @@ // special rank setting (to avoid having to read all stats on verify) cfwrite_int(plr->rank, cfp); + // What game mode we were in last on this pilot + cfwrite_int(plr->is_multi, cfp); + endSection(); } |
|
OK - patch to remove those warnings is attached. a) mainhall was written to CSG during conversion, but is not written to CSG for normal save/load (which seems a bit odd, but it seems to work OK regardless) b) cutscenes were written to CSG flags section AND cutscenes section during conversion, but only the cutscenes section for normal save/load c) the is_multi flag was written for normal PLR save/load but not for the conversion (flag is now used from the previous pilot format) |
|
Ok, those fixes worked for me, no longer getting the errors/warnings. Weakly related, a corrupted .cs2 file creates a nonsensical error message. I've uploaded the files in question as well as the fs2_log. Not sure how much you'll be able to garn from it, though. Um... I labeled the .zip as 2901d, but it's not the same "d" mentioned in the original ticket. <.< |
|
|
|
OK - I've had a look at that borked error message and it is not related to 2901, it's related to r9732 & r9739. I'll create a separate mantis for that issue and commit & resolve this one. |
|
see r9745 & r9746 |
fs2open: trunk r9730 2013-07-20 00:12 Ported: N/A Details Diff |
Partial fix for 2901: pilot conversion needs to read the ubyte mainhall from <= 3.6.18 |
Affected Issues 0002901 |
|
mod - /trunk/fs2_open/code/pilotfile/csg_convert.cpp | Diff File | ||
fs2open: trunk r9745 2013-08-11 05:33 Ported: N/A Details Diff |
Fix 2 for mantis 2901: 7 weapon slots (not 12) for pilot conversions |
Affected Issues 0002901 |
|
mod - /trunk/fs2_open/code/pilotfile/csg_convert.cpp | Diff File | ||
mod - /trunk/fs2_open/code/pilotfile/pilotfile_convert.h | Diff File |
Date Modified | Username | Field | Change |
---|---|---|---|
2013-07-08 05:11 | niffiwan | New Issue | |
2013-07-08 05:11 | niffiwan | Status | new => assigned |
2013-07-08 05:11 | niffiwan | Assigned To | => niffiwan |
2013-07-08 08:01 | Echelon9 | Note Added: 0015173 | |
2013-07-08 09:12 | niffiwan | Note Added: 0015174 | |
2013-07-08 14:47 | Echelon9 | Note Added: 0015175 | |
2013-07-08 14:57 | Echelon9 | Note Edited: 0015175 | |
2013-07-08 15:00 | Echelon9 | Note Edited: 0015175 | |
2013-07-08 15:06 | Echelon9 | Note Edited: 0015175 | |
2013-07-08 20:57 | niffiwan | Note Added: 0015178 | |
2013-07-08 21:01 | niffiwan | Note Edited: 0015178 | |
2013-07-08 21:09 | niffiwan | Note Edited: 0015178 | |
2013-07-08 21:15 | niffiwan | Note Edited: 0015178 | |
2013-07-09 09:37 | niffiwan | Note Added: 0015180 | |
2013-07-09 09:44 | niffiwan | Note Edited: 0015180 | |
2013-07-09 09:47 | niffiwan | File Added: mantis2901-svn.patch | |
2013-07-09 09:48 | niffiwan | Note Added: 0015181 | |
2013-07-09 09:48 | niffiwan | Status | assigned => code review |
2013-07-10 01:38 | niffiwan | Note Added: 0015182 | |
2013-07-14 05:38 | CommanderDJ | Note Added: 0015187 | |
2013-07-17 06:37 | CommanderDJ | Note Added: 0015190 | |
2013-07-18 22:45 | Goober5000 | Note Added: 0015194 | |
2013-07-18 23:10 | niffiwan | Note Added: 0015195 | |
2013-07-19 01:15 | CommanderDJ | Note Added: 0015196 | |
2013-07-19 01:37 | Goober5000 | Note Added: 0015197 | |
2013-07-20 02:57 | niffiwan | Additional Information Updated | |
2013-07-20 02:58 | niffiwan | Relationship added | related to 0002874 |
2013-07-20 03:06 | niffiwan | Changeset attached | => fs2open trunk r9730 |
2013-07-20 03:06 | niffiwan | Note Added: 0015198 | |
2013-07-20 03:06 | niffiwan | Status | code review => resolved |
2013-07-20 03:06 | niffiwan | Resolution | open => fixed |
2013-07-20 03:07 | niffiwan | Note Added: 0015199 | |
2013-07-20 03:07 | niffiwan | Status | resolved => feedback |
2013-07-20 03:07 | niffiwan | Resolution | fixed => reopened |
2013-07-20 03:07 | niffiwan | Status | feedback => assigned |
2013-07-20 03:11 | niffiwan | Description Updated | |
2013-07-20 03:11 | niffiwan | Additional Information Updated | |
2013-07-21 22:17 | niffiwan | Additional Information Updated | |
2013-08-05 08:00 | niffiwan | File Added: mantis2901b-svn.patch | |
2013-08-05 08:13 | niffiwan | Note Added: 0015207 | |
2013-08-05 08:13 | niffiwan | Status | assigned => code review |
2013-08-09 16:40 | z64555 | Note Added: 0015215 | |
2013-08-09 16:41 | z64555 | Note Edited: 0015215 | |
2013-08-10 02:45 | niffiwan | File Added: mantis2901c-svn.patch | |
2013-08-10 02:50 | niffiwan | Note Added: 0015218 | |
2013-08-10 02:51 | niffiwan | Note Edited: 0015218 | |
2013-08-10 04:27 | z64555 | Note Added: 0015219 | |
2013-08-10 04:28 | z64555 | File Added: 2901d.zip | |
2013-08-10 12:04 | z64555 | Note Edited: 0015219 | |
2013-08-10 12:11 | z64555 | Note Edited: 0015219 | |
2013-08-11 08:22 | niffiwan | Note Added: 0015222 | |
2013-08-11 08:24 | niffiwan | Note Added: 0015225 | |
2013-08-11 08:24 | niffiwan | Status | code review => resolved |
2013-08-11 08:24 | niffiwan | Fixed in Version | => 3.7.0 |
2013-08-11 08:24 | niffiwan | Resolution | reopened => fixed |
2013-08-14 13:50 | Goober5000 | Changeset attached | => fs2open trunk r9745 |