2018-09-22 17:42 EDT


View Issue Details Jump to Notes ] Related Changesets ]
IDProjectCategoryView StatusLast Update
0002753FSSCPuser interfacepublic2013-04-29 04:09
ReporterYarn 
Assigned ToZacam 
PrioritynormalSeverityminorReproducibilityalways
StatusresolvedResolutionfixed 
Platformx64OSWindows 7OS Version
Product Version3.6.15 
Target Version3.7.0Fixed in Version 
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.
Attached Files
  • patch file icon mantis2753-svn.patch (5,501 bytes) 2013-04-27 21:32 -
    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 );
     
    
    patch file icon mantis2753-svn.patch (5,501 bytes) 2013-04-27 21:32 +

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

-Notes

~0014441

MjnMixael (manager)

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

~0014451

karajorma (administrator)

Fix committed to trunk@9430.

~0014758

MjnMixael (manager)

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

~0014759

Yarn (developer)

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.

~0014953

karajorma (administrator)

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.

~0014954

niffiwan (developer)

Last edited: 2013-04-23 05:46

View 4 revisions

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

~0014955

niffiwan (developer)

Last edited: 2013-04-23 07:41

View 2 revisions

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.

~0014957

Zacam (administrator)

Last edited: 2013-04-23 16:50

View 2 revisions

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.

~0014966

niffiwan (developer)

Last edited: 2013-04-27 21:44

View 3 revisions

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

~0014970

Zacam (administrator)

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)

~0014973

niffiwan (developer)

Zacam confirmed the fix as good on IRC.

~0014974

niffiwan (developer)

Fix committed to trunk@9657.
+Notes

+Related Changesets

-Issue History
Date Modified Username Field Change
2012-12-13 19:53 Yarn New Issue
2012-12-13 22:03 MjnMixael Assigned To => karajorma
2012-12-13 22:03 MjnMixael Status new => assigned
2012-12-13 22:03 MjnMixael Note Added: 0014441
2012-12-14 03:55 karajorma Changeset attached => fs2open trunk r9430
2012-12-14 03:55 karajorma Note Added: 0014451
2012-12-14 03:55 karajorma Status assigned => resolved
2012-12-14 03:55 karajorma Resolution open => fixed
2013-02-20 15:59 Goober5000 Relationship added related to 0002797
2013-03-09 01:48 MjnMixael Note Added: 0014758
2013-03-09 01:48 MjnMixael Status resolved => assigned
2013-03-09 02:48 Yarn Note Added: 0014759
2013-04-20 21:09 MjnMixael Target Version => 3.7.0
2013-04-21 23:08 niffiwan Relationship added related to 0002802
2013-04-22 22:07 karajorma Assigned To karajorma => Zacam
2013-04-22 22:09 karajorma Note Added: 0014953
2013-04-23 05:03 niffiwan Note Added: 0014954
2013-04-23 05:04 niffiwan Note Edited: 0014954 View Revisions
2013-04-23 05:04 niffiwan Note Edited: 0014954 View Revisions
2013-04-23 05:46 niffiwan Note Edited: 0014954 View Revisions
2013-04-23 07:31 niffiwan Note Added: 0014955
2013-04-23 07:41 niffiwan Note Edited: 0014955 View Revisions
2013-04-23 15:04 Zacam Note Added: 0014957
2013-04-23 16:50 Zacam Note Edited: 0014957 View Revisions
2013-04-27 21:32 niffiwan File Added: mantis2753-svn.patch
2013-04-27 21:40 niffiwan Note Added: 0014966
2013-04-27 21:40 niffiwan Status assigned => code review
2013-04-27 21:43 niffiwan Note Edited: 0014966 View Revisions
2013-04-27 21:44 niffiwan Note Edited: 0014966 View Revisions
2013-04-28 16:39 Zacam Note Added: 0014970
2013-04-28 17:11 niffiwan Relationship added related to 0002855
2013-04-29 04:08 niffiwan Note Added: 0014973
2013-04-29 04:09 niffiwan Changeset attached => fs2open trunk r9657
2013-04-29 04:09 niffiwan Note Added: 0014974
2013-04-29 04:09 niffiwan Status code review => resolved
+Issue History