View Issue Details

IDProjectCategoryView StatusLast Update
0002753FSSCPuser interfacepublic2013-04-29 08:09
ReporterYarn Assigned ToZacam  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Platformx64OSWindows 7 
Product Version3.6.15 
Target Version3.7.0 
Summary0002753: Custom loadouts not loaded
DescriptionChanges in r9370 appear to be preventing custom loadouts from loading; only default loadouts are loaded.
Steps To ReproduceLoad 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.
TagsNo tags attached.

Relationships

related to 0002797 resolvedkarajorma Ship/weapon loadout doing TOO good of a job remembering? 
related to 0002802 resolvedniffiwan A miscalculation in the number of Avengers available. 
related to 0002855 resolvedniffiwan off-by-one error in csg_read_loadout clamp code 

Activities

MjnMixael

2012-12-14 03:03

manager   ~0014441

9370 was a fix by Karajorma. Assigning to him hoping he can take a look.

karajorma

2012-12-14 08:55

administrator   ~0014451

Fix committed to trunk@9430.

MjnMixael

2013-03-09 06:48

manager   ~0014758

This seems to be broken again in Trunk. I haven't narrowed down a commit revision yet.

Yarn

2013-03-09 07:48

developer   ~0014759

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.

karajorma

2013-04-23 02:09

administrator   ~0014953

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.

niffiwan

2013-04-23 09:03

developer   ~0014954

Last edited: 2013-04-23 09:46

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...

niffiwan

2013-04-23 11:31

developer   ~0014955

Last edited: 2013-04-23 11:41

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.

Zacam

2013-04-23 19:04

administrator   ~0014957

Last edited: 2013-04-23 20:50

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.

niffiwan

2013-04-28 01:32

developer  

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 );
 
mantis2753-svn.patch (5,501 bytes)   

niffiwan

2013-04-28 01:40

developer   ~0014966

Last edited: 2013-04-28 01:44

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

Zacam

2013-04-28 20:39

administrator   ~0014970

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)

niffiwan

2013-04-29 08:08

developer   ~0014973

Zacam confirmed the fix as good on IRC.

niffiwan

2013-04-29 08:09

developer   ~0014974

Fix committed to trunk@9657.

Related Changesets

fs2open: trunk r9430

2012-12-14 04:27

karajorma


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

niffiwan


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

Issue History

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