View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0002753 | FSSCP | user interface | public | 2012-12-14 00:53 | 2013-04-29 08:09 |
Reporter | Yarn | Assigned To | Zacam | ||
Priority | normal | Severity | minor | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Platform | x64 | OS | Windows 7 | ||
Product Version | 3.6.15 | ||||
Target Version | 3.7.0 | ||||
Summary | 0002753: Custom loadouts not loaded | ||||
Description | Changes in r9370 appear to be preventing custom loadouts from loading; only default loadouts are loaded. | ||||
Steps To Reproduce | Load any mission that allows loadout customization, and make a change to your loadout. Start the mission, exit it, then exit the game to ensure that your loadout is saved. Start the game and load the mission again, and check the loadout. It should be the one that you used earlier, not the default. | ||||
Tags | No tags attached. | ||||
|
9370 was a fix by Karajorma. Assigning to him hoping he can take a look. |
|
Fix committed to trunk@9430. |
|
This seems to be broken again in Trunk. I haven't narrowed down a commit revision yet. |
|
It looks like it was broken in 9561, the pilot code commit; it works fine in 9560. It's also broken in the Blue Planet builds, so it's almost certainly a problem with the new pilot code. |
|
The basic issue is that the Player_loadout.filename isn't written to the .csg file until the mission is completed successfully. The game however needs to check if this is the same as the mission currently being played so that it knows that the loadout it has saved in the csg file is valid. |
|
The new pilotcode added a reset & reread of the .csg after a mission is completed. That's why I re-added a Pilot.save_savefile() for 2809. However, that save occurs too early to save the loadout data. Moving the save to "commit_pressed()" in missionui/missionshipchoice.cpp would help. However, that doesn't seem to be the whole solution, as while some loadout data actually gets saved to the .csg now (e.g. filename & date), the rest seems to be 0xFF?!? (i.e. -1). The Player_loadout struct unit_data/weapon_pool/ship_pool seems borked. edit: or maybe not, something wierd is going on here... |
|
I think the problem might be poor/missing initialisation of Player_loadout. e.g. unit_data in my testing had 0 for the ship class of all ships not present in the mission (10 for sm1-01). Zero is a valid ship, probably should be -1. This causes missionui/missionscreencommon.cpp:1089 to abort the loadout restore because there are more Uly's (10) in last_loadout_ships than this_loadout_ships (0). edit: or maybe a problem with Wss_slots init, I think that's where Player_loadout->unit_data->ship_class initially gets set from. Need to trace the problem when it 1st occurs, but not right now, sleep. |
|
I traced in MSVC to trip on the saving of the Loadout, which did fire off. The structure for the storage of the weapons selections made isn't happened. It goes haywire on the data when it gets into the subsystem reading event to generate the data that it should be writing, as a result, nothing gets stored. To note: when you "Restart" a mission, any custom loadout changes made DO get retained and propagated, so there is storage happening, just not to the CSG as expected. |
|
mantis2753-svn.patch (5,501 bytes)
Index: code/pilotfile/csg.cpp =================================================================== --- code/pilotfile/csg.cpp (revision 9651) +++ code/pilotfile/csg.cpp (working copy) @@ -513,8 +513,7 @@ void pilotfile::csg_read_loadout() { - int j; - int count; + int j, count, ship_idx = -1, wep_idx = -1; uint idx, list_size = 0; if ( !m_have_info ) { @@ -555,31 +554,39 @@ } // ship - idx = cfread_int(cfp); + ship_idx = cfread_int(cfp); - if (idx > ship_list.size()) { - //mprintf(("CSG => Parse Warning: Invalid value for ship index (%d), Clamping to valid range.\n", idx)); - idx = ship_list.size(); + if (ship_idx > (int)ship_list.size()) { // on the casts, assume that ship & weapon lists will never exceed ~2 billion + //mprintf(("CSG => Parse Warning: Invalid value for ship index (%d), Clamping to valid range.\n", ship_idx)); + ship_idx = ship_list.size(); } if (slot) { - slot->ship_class = ship_list[idx].index; + if (ship_idx == -1) { // -1 means no ship in this slot + slot->ship_class = -1; + } else { + slot->ship_class = ship_list[ship_idx].index; + } } // primary weapons count = cfread_int(cfp); for (j = 0; j < count; j++) { - idx = cfread_int(cfp); + wep_idx = cfread_int(cfp); - if (idx > weapon_list.size()) { - //mprintf(("CSG => Parse Warning: Invalid value for weapon index (%d), Clamping to valid range.\n", idx)); - idx = weapon_list.size(); + if (wep_idx > (int)weapon_list.size()) { + //mprintf(("CSG => Parse Warning: Invalid value for weapon index (%d), Clamping to valid range.\n", wep_idx)); + wep_idx = weapon_list.size(); } if ( slot && (j < MAX_SHIP_PRIMARY_BANKS) ) { - slot->wep[j] = weapon_list[idx].index; + if (wep_idx == -1) { // -1 means no weapon in this slot + slot->wep[j] = -1; + } else { + slot->wep[j] = weapon_list[wep_idx].index; + } } idx = cfread_int(cfp); @@ -593,15 +600,19 @@ count = cfread_int(cfp); for (j = 0; j < count; j++) { - idx = cfread_int(cfp); + wep_idx = cfread_int(cfp); - if (idx > weapon_list.size()) { - //mprintf(("CSG => Parse Warning: Invalid value for weapon index (%d), Clamping to valid range.\n", idx)); - idx = weapon_list.size(); + if (wep_idx > (int)weapon_list.size()) { + //mprintf(("CSG => Parse Warning: Invalid value for weapon index (%d), Clamping to valid range.\n", wep_idx)); + wep_idx = weapon_list.size(); } if ( slot && (j < MAX_SHIP_SECONDARY_BANKS) ) { - slot->wep[j+MAX_SHIP_PRIMARY_BANKS] = weapon_list[idx].index; + if (wep_idx == -1) { // -1 means no weapon in this slot + slot->wep[j+MAX_SHIP_PRIMARY_BANKS] = -1; + } else { + slot->wep[j+MAX_SHIP_PRIMARY_BANKS] = weapon_list[wep_idx].index; + } } idx = cfread_int(cfp); Index: code/missionui/missionshipchoice.cpp =================================================================== --- code/missionui/missionshipchoice.cpp (revision 9651) +++ code/missionui/missionshipchoice.cpp (working copy) @@ -1965,6 +1965,7 @@ } // in single player we jump directly into the mission else { + Pilot.save_savefile(); gameseq_post_event(GS_EVENT_ENTER_GAME); } } @@ -2493,7 +2494,6 @@ change_ship_type(Player_obj->instance, si_index); Player->last_ship_flown_si_index = si_index; - Pilot.save_savefile(); // saves both Recent_mission & last_ship_flown_si_index (for quick-start-missions) } /* @@ -2924,6 +2924,7 @@ for ( i = 0; i < MAX_WING_BLOCKS; i++ ) { for ( j = 0; j < MAX_WING_SLOTS; j++ ) { slot = &Ss_wings[i].ss_slots[j]; + slot = &Ss_wings[i].ss_slots[j]; slot->status = WING_SLOT_LOCKED; slot->sa_index = -1; slot->original_ship_class = -1; Index: code/mission/missioncampaign.cpp =================================================================== --- code/mission/missioncampaign.cpp (revision 9651) +++ code/mission/missioncampaign.cpp (working copy) @@ -723,7 +723,35 @@ return mission_campaign_load_by_name( filename); } +/* + * initialise Player_loadout with default values + */ +void player_loadout_init() +{ + int i = 0, j = 0; + memset(Player_loadout.filename, 0, sizeof(Player_loadout.filename)); + memset(Player_loadout.last_modified, 0, sizeof(Player_loadout.last_modified)); + + for ( i = 0; i < MAX_SHIP_CLASSES; i++ ) { + Player_loadout.ship_pool[i] = 0; + } + + for ( i = 0; i < MAX_WEAPON_TYPES; i++ ) { + Player_loadout.weapon_pool[i] = 0; + } + + for ( i = 0; i < MAX_WSS_SLOTS; i++ ) { + Player_loadout.unit_data[i].ship_class = -1; + + for ( j = 0; j < MAX_SHIP_WEAPONS; j++ ) { + Player_loadout.unit_data[i].wep[j] = 0; + Player_loadout.unit_data[i].wep_count[j] = 0; + } + } + +} + /** * Initializes some variables then loads the default FreeSpace single player campaign. */ @@ -732,6 +760,8 @@ mission_campaign_clear(); Campaign_file_missing = 0; + + player_loadout_init(); } /** Index: code/mission/missioncampaign.h =================================================================== --- code/mission/missioncampaign.h (revision 9651) +++ code/mission/missioncampaign.h (working copy) @@ -154,6 +154,11 @@ // if the campaign file is missing this will get set for us to check against extern int Campaign_file_missing; +/* + * initialise Player_loadout with default values + */ +void player_loadout_init(); + // called at game startup time to load the default single player campaign void mission_campaign_init( void ); |
|
I believe the attached patch fixes the issue. When testing, please use a new pilot file as any existing pilot probably has corrupt data in the loadout section. Changes are: 1) move csg save to be the after the commit is pressed 2) when reading the loadout from the csg, use an int instead of a uint for ship/weapon indexes. With a uint a stored index value of -1 is read as approx 4 billion which corrupts the loadout data. 3) (being defensive) initialise the Player_loadout structure on game start. note: there's another off-by-one bug in the loadout index "clamp" code that I'll fix after this problem is resolved |
|
Testing. I'd also like to see the patch for that off-by-one, getting them both in would be good. (We'll probably want a separate Mantis and Code Review set up for that one and note which one requires the other if that is necessary) |
|
Zacam confirmed the fix as good on IRC. |
|
Fix committed to trunk@9657. |
fs2open: trunk r9430 2012-12-14 04:27 Ported: N/A Details Diff |
Fix Mantis 2753 (Customised Loadouts sometimes won't save for next time the mission is played). |
Affected Issues 0002753, 0002797 |
|
mod - /trunk/fs2_open/code/missionui/missionscreencommon.cpp | Diff File | ||
fs2open: trunk r9657 2013-04-29 05:01 Ported: N/A Details Diff |
Fix mantis 2753: save csg just before mission starts read loadout indexes into int instead of uint & ensure -1 is not used as array index initialise Player_loadout struct during FSO start |
Affected Issues 0002753 |
|
mod - /trunk/fs2_open/code/mission/missioncampaign.cpp | Diff File | ||
mod - /trunk/fs2_open/code/mission/missioncampaign.h | Diff File | ||
mod - /trunk/fs2_open/code/missionui/missionshipchoice.cpp | Diff File | ||
mod - /trunk/fs2_open/code/pilotfile/csg.cpp | Diff File |
Date Modified | Username | Field | Change |
---|---|---|---|
2012-12-14 00:53 | Yarn | New Issue | |
2012-12-14 03:03 | MjnMixael | Assigned To | => karajorma |
2012-12-14 03:03 | MjnMixael | Status | new => assigned |
2012-12-14 03:03 | MjnMixael | Note Added: 0014441 | |
2012-12-14 08:55 | karajorma | Changeset attached | => fs2open trunk r9430 |
2012-12-14 08:55 | karajorma | Note Added: 0014451 | |
2012-12-14 08:55 | karajorma | Status | assigned => resolved |
2012-12-14 08:55 | karajorma | Resolution | open => fixed |
2013-02-20 20:59 | Goober5000 | Relationship added | related to 0002797 |
2013-03-09 06:48 | MjnMixael | Note Added: 0014758 | |
2013-03-09 06:48 | MjnMixael | Status | resolved => assigned |
2013-03-09 07:48 | Yarn | Note Added: 0014759 | |
2013-04-21 01:09 | MjnMixael | Target Version | => 3.7.0 |
2013-04-22 03:08 | niffiwan | Relationship added | related to 0002802 |
2013-04-23 02:07 | karajorma | Assigned To | karajorma => Zacam |
2013-04-23 02:09 | karajorma | Note Added: 0014953 | |
2013-04-23 09:03 | niffiwan | Note Added: 0014954 | |
2013-04-23 09:04 | niffiwan | Note Edited: 0014954 | |
2013-04-23 09:04 | niffiwan | Note Edited: 0014954 | |
2013-04-23 09:46 | niffiwan | Note Edited: 0014954 | |
2013-04-23 11:31 | niffiwan | Note Added: 0014955 | |
2013-04-23 11:41 | niffiwan | Note Edited: 0014955 | |
2013-04-23 19:04 | Zacam | Note Added: 0014957 | |
2013-04-23 20:50 | Zacam | Note Edited: 0014957 | |
2013-04-28 01:32 | niffiwan | File Added: mantis2753-svn.patch | |
2013-04-28 01:40 | niffiwan | Note Added: 0014966 | |
2013-04-28 01:40 | niffiwan | Status | assigned => code review |
2013-04-28 01:43 | niffiwan | Note Edited: 0014966 | |
2013-04-28 01:44 | niffiwan | Note Edited: 0014966 | |
2013-04-28 20:39 | Zacam | Note Added: 0014970 | |
2013-04-28 21:11 | niffiwan | Relationship added | related to 0002855 |
2013-04-29 08:08 | niffiwan | Note Added: 0014973 | |
2013-04-29 08:09 | niffiwan | Changeset attached | => fs2open trunk r9657 |
2013-04-29 08:09 | niffiwan | Note Added: 0014974 | |
2013-04-29 08:09 | niffiwan | Status | code review => resolved |