fix persistent variables and containers (#7192)

code/mission/missioncampaign.cpp

@@ -935,15 +935,11 @@ void mission_campaign_eval_next_mission()
  */
 void mission_campaign_store_goals_and_events()
 {
-	int cur;
-	cmission *mission_obj;
-
 	if (!(Game_mode & GM_CAMPAIGN_MODE) || (Campaign.current_mission < 0))
 		return;
 
-	cur = Campaign.current_mission;
-
-	mission_obj = &Campaign.missions[cur];
+	int cur = Campaign.current_mission;
+	auto mission_obj = &Campaign.missions[cur];
 
 	// first we must save the status of the current missions goals in the campaign mission structure.
 	// After that, we can determine which mission is tagged as the next mission.  Finally, we
@@ -999,66 +995,54 @@ void mission_campaign_store_goals_and_events()
 
 void mission_campaign_store_variables(int persistence_type, bool store_red_alert)
 {
-	int cur, i, j;
-	cmission *mission_obj;
-
 	if (!(Game_mode & GM_CAMPAIGN_MODE) || (Campaign.current_mission < 0))
 		return;
 
-	cur = Campaign.current_mission;
-	mission_obj = &Campaign.missions[cur];
+	int cur = Campaign.current_mission;
+	auto mission_obj = &Campaign.missions[cur];
 
-	// handle variables that are saved on mission victory -------------------------------------
 	mission_obj->variables.clear();
 
-	int num_mission_variables = sexp_campaign_file_variable_count();
-
-	if (num_mission_variables > 0) {
-		
-		if (store_red_alert) {
-			for (auto& current_rav : Campaign.red_alert_variables) {
-				Campaign.persistent_variables.push_back(current_rav);
-			}
+	int num_sexp_variables = sexp_variable_count();
+	for (int i = 0; i < num_sexp_variables; i++) {
+		if (!(Sexp_variables[i].type & persistence_type)) {
+			continue;
 		}
 
-		for (i = 0; i < sexp_variable_count(); i++) {
-			if (!(Sexp_variables[i].type & SEXP_VARIABLE_SAVE_TO_PLAYER_FILE)) {
-				if (Sexp_variables[i].type & persistence_type) {
-					bool add_it = true;
-
-					// see if we already have a variable with this name
-					for (j = 0; j < (int)Campaign.persistent_variables.size(); j++) {
-						if (!(stricmp(Sexp_variables[i].variable_name, Campaign.persistent_variables[j].variable_name))) {
-							add_it = false;
-							Campaign.persistent_variables[j].type = Sexp_variables[i].type;
-							strcpy_s(Campaign.persistent_variables[j].text, Sexp_variables[i].text);
-							break;
-						}
-					}
-
-					// new variable
-					if (add_it) {
-						Campaign.persistent_variables.push_back(Sexp_variables[i]);
-					}
-				}
+		// player-persistent (aka "eternal")
+		if (Sexp_variables[i].type & SEXP_VARIABLE_SAVE_TO_PLAYER_FILE) {
+			// see if we already have a variable with this name
+			int j = find_item_with_string(Player->variables, &sexp_variable::variable_name, Sexp_variables[i].variable_name);
+			if (j >= 0) {
+				Player->variables[j].type = Sexp_variables[i].type;
+				strcpy_s(Player->variables[j].text, Sexp_variables[i].text);
 			}
-			// we might need to save some eternal variables
-			else if ((persistence_type & SEXP_VARIABLE_SAVE_ON_MISSION_PROGRESS) && (Sexp_variables[i].type & persistence_type) && (Sexp_variables[i].type & SEXP_VARIABLE_SAVE_TO_PLAYER_FILE)) {
-				bool add_it = true;
+			// new variable
+			else {
+				Player->variables.push_back(Sexp_variables[i]);
+			}
+		}
+		// campaign-persistent
+		else {
+			// see if we already have a variable with this name
+			int j = find_item_with_string(Campaign.persistent_variables, &sexp_variable::variable_name, Sexp_variables[i].variable_name);
+			if (j >= 0) {
+				Campaign.persistent_variables[j].type = Sexp_variables[i].type;
+				strcpy_s(Campaign.persistent_variables[j].text, Sexp_variables[i].text);
+			}
+			// new variable
+			else {
+				Campaign.persistent_variables.push_back(Sexp_variables[i]);
+			}
+		}
+	}
 
-				for (j = 0; j < (int)Player->variables.size(); j++) {
-					if (!(stricmp(Sexp_variables[i].variable_name, Player->variables[j].variable_name))) {
-						Player->variables[j] = Sexp_variables[i];
-
-						add_it = false;
-						break;
-					}
-				}
-
-				// if not found then add new entry
-				if (add_it) {
-					Player->variables.push_back(Sexp_variables[i]);
-				}
+	if (store_red_alert) {
+		for (const auto& current_rav : Campaign.red_alert_variables) {
+			if (find_item_with_string(Campaign.persistent_variables, &sexp_variable::variable_name, current_rav.variable_name) < 0) {
+				Campaign.persistent_variables.push_back(current_rav);
+			} else {
+				Warning(LOCATION, "A red alert variable has the same name as a persistent variable!");
 			}
 		}
 	}
@@ -1070,40 +1054,17 @@ void mission_campaign_store_containers(ContainerType persistence_type, bool stor
 	if (!(Game_mode & GM_CAMPAIGN_MODE) || (Campaign.current_mission < 0))
 		return;
 
-	if (!sexp_container_has_persistent_non_eternal_containers()) {
-		// nothing to do
-		return;
-	}
-
-	if (store_red_alert) {
-		for (const auto& current_con : Campaign.red_alert_containers) {
-			Campaign.persistent_containers.emplace_back(current_con);
-		}
-	}
-
 	for (const auto &container : get_all_sexp_containers()) {
-		if (!container.is_eternal()) {
-			if (any(container.type & persistence_type)) {
-				// see if we already have a container with this name
-				auto cpc_it = std::find_if(Campaign.persistent_containers.begin(),
-					Campaign.persistent_containers.end(),
-					[container](const sexp_container &cpc) {
-						return cpc.name_matches(container);
-					});
+		if (none(container.type & persistence_type)) {
+			continue;
+		}
 
-				if (cpc_it != Campaign.persistent_containers.end()) {
-					*cpc_it = container;
-				} else {
-					// new container
-					Campaign.persistent_containers.emplace_back(container);
-				}
-			}
-		} else if (any(persistence_type & ContainerType::SAVE_ON_MISSION_PROGRESS) &&
-				   any(container.type & persistence_type) && container.is_eternal()) {
-			// we might need to save some eternal player-persistent containers
+		// player-persistent (aka "eternal")
+		if (container.is_eternal()) {
+			// see if we already have a container with this name
 			auto ppc_it = std::find_if(Player->containers.begin(),
 				Player->containers.end(),
-				[container](const sexp_container &ppc) {
+				[&container](const sexp_container& ppc) {
 					return ppc.name_matches(container);
 				});
 
@@ -1111,17 +1072,50 @@ void mission_campaign_store_containers(ContainerType persistence_type, bool stor
 				*ppc_it = container;
 			} else {
 				// new player-persistent container
-				Player->containers.emplace_back(container);
+				Player->containers.push_back(container);
+			}
+		}
+		// campaign-persistent
+		else {
+			// see if we already have a container with this name
+			auto cpc_it = std::find_if(Campaign.persistent_containers.begin(),
+				Campaign.persistent_containers.end(),
+				[&container](const sexp_container& cpc) {
+					return cpc.name_matches(container);
+				});
+
+			if (cpc_it != Campaign.persistent_containers.end()) {
+				*cpc_it = container;
+			} else {
+				// new container
+				Campaign.persistent_containers.push_back(container);
+			}
+		}
+	}
+
+	if (store_red_alert) {
+		for (const auto& container : Campaign.red_alert_containers) {
+			// see if we already have a container with this name
+			auto cpc_it = std::find_if(Campaign.persistent_containers.begin(),
+				Campaign.persistent_containers.end(),
+				[&container](const sexp_container& cpc) {
+					return cpc.name_matches(container);
+				});
+
+			if (cpc_it == Campaign.persistent_containers.end()) {
+				Campaign.persistent_containers.push_back(container);
+			} else {
+				Warning(LOCATION, "A red alert container has the same name as a persistent container!");
 			}
 		}
 	}
 }
 
-void mission_campaign_store_goals_and_events_and_variables()
+void mission_campaign_store_goals_and_events_and_variables(bool store_red_alert_data)
 {
 	mission_campaign_store_goals_and_events();
-	mission_campaign_store_variables(SEXP_VARIABLE_SAVE_ON_MISSION_PROGRESS);
-	mission_campaign_store_containers(ContainerType::SAVE_ON_MISSION_PROGRESS);
+	mission_campaign_store_variables(SEXP_VARIABLE_SAVE_ON_MISSION_PROGRESS, store_red_alert_data);
+	mission_campaign_store_containers(ContainerType::SAVE_ON_MISSION_PROGRESS, store_red_alert_data);
 }
 
 /**
@@ -1153,9 +1147,6 @@ void mission_campaign_mission_over(bool do_next_mission)
 		Campaign.weapons_allowed[Granted_weapons[i]] = 1;	
 	}
 
-	// Goober5000 - player-persistent variables are handled when the mission is
-	// over, not necessarily when the mission is accepted
-
 	// update campaign.mission stats (used to allow backout inRedAlert)
 	// .. but we don't do this if we are inside of the prev/current loop hack
 	if ( Campaign.prev_mission != Campaign.current_mission ) {
@@ -1686,6 +1677,9 @@ void mission_campaign_end_init()
 
 void mission_campaign_end_do()
 {
+	mission_campaign_store_variables(SEXP_VARIABLE_SAVE_ON_MISSION_PROGRESS, false);
+	mission_campaign_store_containers(ContainerType::SAVE_ON_MISSION_PROGRESS, false);
+
 	// close out the mission
 	event_music_level_close();
 	mission_goal_fail_incomplete();
@@ -1732,7 +1726,7 @@ void mission_campaign_skip_to_next()
 	mission_goal_mark_events_complete();
 
 	// store
-	mission_campaign_store_goals_and_events_and_variables();
+	mission_campaign_store_goals_and_events_and_variables(false);
 
 	// now set the next mission
 	mission_campaign_eval_next_mission();
@@ -1849,84 +1843,6 @@ bool mission_campaign_jump_to_mission(const char* filename, bool no_skip)
 	}
 }
 
-// Goober5000
-void mission_campaign_save_on_close_variables()
-{
-	int i;
-
-	// make sure we are actually playing a single-player campaign
-	if (!(Game_mode & GM_CAMPAIGN_MODE) || (Campaign.type != CAMPAIGN_TYPE_SINGLE) || (Campaign.current_mission < 0))
-		return;
-
-	// now save variables
-	for (i = 0; i < sexp_variable_count(); i++) {
-		// we only want the on mission close type. On campaign progress type are dealt with elsewhere
-		if ( !(Sexp_variables[i].type & SEXP_VARIABLE_SAVE_ON_MISSION_CLOSE) ) {
-			continue;
-		}
-
-		bool found = false;
-
-		// deal with eternals 
-		if ((Sexp_variables[i].type & SEXP_VARIABLE_SAVE_TO_PLAYER_FILE)) {
-			// check if variable already exists and updated it
-			for (auto& current_variable : Player->variables) {
-				if (!(stricmp(Sexp_variables[i].variable_name, current_variable.variable_name))) {
-					current_variable = Sexp_variables[i];
-
-					found = true;
-					break;
-				}
-			}
-
-			// if not found then add new entry
-			if (!found) {
-				Player->variables.push_back(Sexp_variables[i]);
-			}
-		}
-
-	}
-
-	// store any non-eternal on mission close variables
-	mission_campaign_store_variables(SEXP_VARIABLE_SAVE_ON_MISSION_CLOSE, false);
-}
-
-// jg18 - adapted from mission_campaign_save_on_close_variables()
-void mission_campaign_save_on_close_containers()
-{
-	// make sure we are actually playing a single-player campaign
-	if (!(Game_mode & GM_CAMPAIGN_MODE) || (Campaign.type != CAMPAIGN_TYPE_SINGLE) || (Campaign.current_mission < 0))
-		return;
-
-	// now save containers
-	for (const auto &container : get_all_sexp_containers()) {
-		// we only want the on mission close type. On campaign progress type are dealt with elsewhere
-		if (none(container.type & ContainerType::SAVE_ON_MISSION_CLOSE)) {
-			continue;
-		}
-
-		// deal with eternals
-		if (container.is_eternal()) {
-			// check if container already exists and update it
-			auto ppc_it = std::find_if(Player->containers.begin(),
-				Player->containers.end(),
-				[container](const sexp_container &ppc) {
-					return ppc.name_matches(container);
-				});
-
-			if (ppc_it != Player->containers.end()) {
-				*ppc_it = container;
-			} else {
-				// if not found then add new entry
-				Player->containers.emplace_back(container);
-			}
-		}
-	}
-
-	// store any non-eternal on mission close containers
-	mission_campaign_store_containers(ContainerType::SAVE_ON_MISSION_CLOSE, false);
-}
-
 void mission_campaign_load_failure_popup()
 {
 	if (Campaign_load_failure == 0) {

code/mission/missioncampaign.h

@@ -226,13 +226,13 @@ int mission_load_up_campaign(bool fall_back_from_current = false);
 void mission_campaign_store_goals_and_events();
 
 // stores variables which will be saved only on mission progression
-void mission_campaign_store_variables(int persistence_type, bool store_red_alert = true);
+void mission_campaign_store_variables(int persistence_type, bool store_red_alert);
 
 // stores containers which will be saved only on mission progression
-void mission_campaign_store_containers(ContainerType persistence_type, bool store_red_alert = true);
+void mission_campaign_store_containers(ContainerType persistence_type, bool store_red_alert);
 
 // does all three of the above
-void mission_campaign_store_goals_and_events_and_variables();
+void mission_campaign_store_goals_and_events_and_variables(bool store_red_alert_data);
 
 // evaluates next mission and possible loop mission
 void mission_campaign_eval_next_mission();
@@ -254,12 +254,6 @@ void mission_campaign_end_init();
 void mission_campaign_end_close();
 void mission_campaign_end_do();
 
-// save eternal variables
-extern void mission_campaign_save_on_close_variables();
-
-// save eternal containers
-extern void mission_campaign_save_on_close_containers();
-
 extern void mission_campaign_load_failure_popup();
 
 SCP_string mission_campaign_get_name(const char* filename);

code/mission/missionparse.cpp

@@ -6336,7 +6336,7 @@ void parse_asteroid_fields(mission *pm)
 
 void parse_variables()
 {
-	int i, j, num_variables = 0;
+	int j, num_variables = 0;
 
 	if (! optional_string("#Sexp_variables") ) {
 		return;
@@ -6350,56 +6350,41 @@ void parse_variables()
 		return;
 	}
 
-	// Goober5000 - now set the default value, if it's a variable saved on mission progress
+	// Goober5000 - now set the default value, if it's a persistent variable
+
 	// loop through the current mission's variables
 	for (j = 0; j < num_variables; j++) {
-		// check against existing variables
-		for (auto& current_pv : Campaign.persistent_variables) {
-			// if the active mission has a variable with the same name as a variable saved to the campaign file override its initial value with the previous mission's value
-			if ( !stricmp(Sexp_variables[j].variable_name, current_pv.variable_name) ) {
-				// if this is an eternal that shares the same name as a non-eternal warn but do nothing
+		// check against existing campaign variables first
+		for (const auto &campaign_var : Campaign.persistent_variables) {
+			// if the active mission has a variable with the same name as a variable saved to the campaign file, override its initial value with the persistent value
+			if ( !stricmp(Sexp_variables[j].variable_name, campaign_var.variable_name) ) {
+				// if the mission variable is eternal and shares the same name as a non-eternal persistent variable, warn but do nothing
 				if (Sexp_variables[j].type & SEXP_VARIABLE_SAVE_TO_PLAYER_FILE) {
-					error_display(0, "Variable %s is marked eternal but has the same name as another persistent variable. One of these should be renamed to avoid confusion", Sexp_variables[j].text);
+					error_display(0, "Variable %s is marked eternal in the mission file but has the same name as another non-eternal persistent variable in the campaign.  One of these should be renamed to avoid confusion.", Sexp_variables[j].text);
 				}
-				else if (Sexp_variables[j].type  & SEXP_VARIABLE_IS_PERSISTENT) {
-					Sexp_variables[j].type = current_pv.type;
-					strcpy_s(Sexp_variables[j].text, current_pv.text);
-					break;
+				else if (Sexp_variables[j].type & SEXP_VARIABLE_IS_PERSISTENT) {
+					Sexp_variables[j].type = campaign_var.type;
+					strcpy_s(Sexp_variables[j].text, campaign_var.text);
 				} else {
-					error_display(0, "Variable %s has the same name as another persistent variable. One of these should be renamed to avoid confusion", Sexp_variables[j].text);
+					error_display(0, "Variable %s is not marked persistent in the mission file but has the same name as another persistent variable in the campaign.  One of these should be renamed to avoid confusion, or the mission variable should be marked persistent.", Sexp_variables[j].text);
 				}
+				break;
 			}
 		}
-	}
 
-	// next, see if any eternal variables are set loop through the current mission's variables
-	for (j = 0; j < num_variables; j++) {
-		// check against existing variables
-		for (i = 0; i < (int)Player->variables.size(); i++) {
-			// if the active mission has a variable with the same name as a variable saved to the player file override its initial value with the previous mission's value
-			if ( !stricmp(Sexp_variables[j].variable_name, Player->variables[i].variable_name) ) {
-				if (Sexp_variables[j].type & SEXP_VARIABLE_IS_PERSISTENT) {
-					// if the variable in the player file is marked as eternal but the version in the mission file is not, we assume that the player file one is rogue
-					// and use the one in the mission file instead.
-					if ((Player->variables[i].type & SEXP_VARIABLE_SAVE_TO_PLAYER_FILE) && !(Sexp_variables[j].type & SEXP_VARIABLE_SAVE_TO_PLAYER_FILE)) {
-						break;
-					}
-					// replace the default values with the ones saved to the player file
-					Sexp_variables[j].type = Player->variables[i].type;
-					strcpy_s(Sexp_variables[j].text, Player->variables[i].text);
-
-					/*
-					// check that the eternal flag has been set. Players using a player file from before the eternal flag was added may have old player-persistent variables
-					// these should be converted to non-eternals
-					if (!(Player->variables[i].type & SEXP_VARIABLE_SAVE_TO_PLAYER_FILE)) {
-						Sexp_variables[j].type &= ~SEXP_VARIABLE_SAVE_TO_PLAYER_FILE;
-					}
-					*/
-
-					break;
+		// now check against existing player (aka "eternal") variables
+		for (const auto &player_var : Player->variables) {
+			// if the active mission has a variable with the same name as a variable saved to the player file, override its initial value with the persistent value
+			if ( !stricmp(Sexp_variables[j].variable_name, player_var.variable_name) ) {
+				// the variable in the mission file must be persistent and marked eternal,
+				// otherwise we assume that the player file variable is rogue and we do not use the persistent value
+				if ((Sexp_variables[j].type & SEXP_VARIABLE_SAVE_TO_PLAYER_FILE) && (Sexp_variables[j].type & SEXP_VARIABLE_IS_PERSISTENT)) {
+					Sexp_variables[j].type = player_var.type;
+					strcpy_s(Sexp_variables[j].text, player_var.text);
 				} else {
-					error_display(0, "Variable %s has the same name as an eternal variable. One of these should be renamed to avoid confusion", Sexp_variables[j].variable_name);
+					error_display(0, "Variable %s is not marked persistent and/or not marked eternal in the mission file but has the same name as another eternally persistent variable.  This may be an unintentional name collision, or the mission variable may need to be marked eternally persistent.", Sexp_variables[j].text);
 				}
+				break;
 			}
 		}
 	}
@@ -6435,11 +6420,11 @@ void parse_sexp_containers()
 		if (p_container != nullptr) {
 			auto &container = *p_container;
 
-			// if this is an eternal container that shares the same name as a non-eternal, warn but do nothing
+			// if the mission container is eternal and shares the same name as a non-eternal persistent container, warn but do nothing
 			if (container.is_eternal()) {
 				error_display(0,
-					"SEXP container %s is marked eternal but has the same name as another persistent container. One of "
-					"these should be renamed to avoid confusion",
+					"SEXP container %s is marked eternal in the mission file but has the same name as another non-eternal persistent container in the campaign.  One of "
+					"these should be renamed to avoid confusion.",
 					container.container_name.c_str());
 			} else if (container.is_persistent()) {
 				if (container.type_matches(current_pc)) {
@@ -6450,53 +6435,48 @@ void parse_sexp_containers()
 					container = current_pc;
 				} else {
 					error_display(0,
-						"SEXP container %s is marked persistent but its type (%x) doesn't match a similarly named "
-						"persistent container's type (%x). One of "
-						"these should be renamed to avoid confusion",
+						"SEXP container %s is marked persistent in the mission file but its type (%x) doesn't match a similarly named "
+						"persistent container's type (%x) in the campaign.  One of these should be renamed to avoid confusion.",
 						container.container_name.c_str(),
 						(int)container.get_non_persistent_type(),
 						(int)current_pc.get_non_persistent_type());
 				}
 			} else {
 				error_display(0,
-					"SEXP container %s has the same name as another persistent container. One of these should be "
-					"renamed to avoid confusion",
+					"SEXP container %s is not marked persistent in the mission file but has the same name as another persistent container in the campaign.  One of these should be "
+					"renamed to avoid confusion, or the mission container should be marked persistent.",
 					container.container_name.c_str());
 			}
 		}
 	}
 
 	// then update this mission's containers from player-persistent containers
-	for (const auto& player_container : Player->containers) {
+	for (const auto &player_container : Player->containers) {
 		auto *p_container = get_sexp_container(player_container.container_name.c_str());
 		if (p_container != nullptr) {
 			auto &container = *p_container;
 
-			if (container.is_persistent()) {
-				if (player_container.is_eternal() && !container.is_eternal()) {
-					// use the mission's non-eternal container over the player-persistent eternal container
-					continue;
-				} else {
-					if (container.type_matches(player_container)) {
-						// TODO: when network containers are supported, review whether replacement should occur
-						// if one container is marked for network use and the other isn't
+			// the container in the mission file must be persistent and marked eternal,
+			// otherwise we assume that the player file container is rogue and we do not use the persistent values
+			if (container.is_eternal() && container.is_persistent()) {
+				if (container.type_matches(player_container)) {
+					// TODO: when network containers are supported, review whether replacement should occur
+					// if one container is marked for network use and the other isn't
 
-						// replace!
-						container = player_container;
-					} else {
-						error_display(0,
-							"SEXP container %s is marked persistent but its type (%x) doesn't match a similarly named "
-							"eternal container's type (%x). One of "
-							"these should be renamed to avoid confusion",
-							container.container_name.c_str(),
-							(int)container.get_non_persistent_type(),
-							(int)player_container.get_non_persistent_type());
-					}
+					// replace!
+					container = player_container;
+				} else {
+					error_display(0,
+						"SEXP container %s is marked persistent in the mission file but its type (%x) doesn't match a similarly named "
+						"persistent container's type (%x) in the player file.  One of these should be renamed to avoid confusion.",
+						container.container_name.c_str(),
+						(int)container.get_non_persistent_type(),
+						(int)player_container.get_non_persistent_type());
 				}
 			} else {
 				error_display(0,
-					"SEXP container %s has the same name as an eternal container. One of these should be renamed "
-					"to avoid confusion",
+					"SEXP container %s is not marked persistent and/or not marked eternal in the mission file but has the same name as another eternally persistent container.  This "
+					"may be an unintentional name collision, or the mission container may need to be marked eternally persistent.",
 					container.container_name.c_str());
 			}
 		}

code/missionui/missionbrief.cpp

@@ -346,7 +346,7 @@ void brief_skip_training_pressed()
 	// tricky part.  Need to move to the next mission in the campaign.
 	mission_goal_mark_objectives_complete();
 	mission_goal_fail_incomplete();
-	mission_campaign_store_goals_and_events_and_variables();
+	mission_campaign_store_goals_and_events_and_variables(false);
 
 	mission_campaign_eval_next_mission();
 	mission_campaign_mission_over();	

code/missionui/missiondebrief.cpp

@@ -1325,8 +1325,8 @@ void debrief_accept(int ok_to_post_start_game_event, bool API_Access)
 		// mission that isn't in a campaign.
 		if ( Game_mode & GM_CAMPAIGN_MODE ) {
 
-			mission_campaign_store_variables(SEXP_VARIABLE_SAVE_ON_MISSION_PROGRESS);
-			mission_campaign_store_containers(ContainerType::SAVE_ON_MISSION_PROGRESS);
+			mission_campaign_store_variables(SEXP_VARIABLE_SAVE_ON_MISSION_PROGRESS, false);
+			mission_campaign_store_containers(ContainerType::SAVE_ON_MISSION_PROGRESS, false);
 
 			// check for possible mission loop
 			// check for (1) mission loop available, (2) don't have to repeat last mission

code/missionui/redalert.cpp

@@ -1232,7 +1232,7 @@ void red_alert_maybe_move_to_next_mission()
 			// this should handle close-out and moving to next mission
 			gameseq_post_event(GS_EVENT_DEBRIEF);
 		} else {
-			mission_campaign_store_goals_and_events_and_variables();
+			mission_campaign_store_goals_and_events_and_variables(true);
 			scoring_level_close();
 			mission_campaign_eval_next_mission();
 			mission_campaign_mission_over();

code/network/multi.cpp

@@ -1752,7 +1752,7 @@ void multi_standalone_postgame_init()
 	if ( Game_mode & GM_CAMPAIGN_MODE ) {
 		// MUST store goals and events first - may be used to evaluate next mission
 		// store goals and events
-		mission_campaign_store_goals_and_events_and_variables();
+		mission_campaign_store_goals_and_events_and_variables(false);
 
 		// evaluate next mission
 		mission_campaign_eval_next_mission();

code/parse/sexp.cpp

@@ -36358,22 +36358,6 @@ int sexp_variable_count()
 	return count;
 }
 
-/**
- * Count number of persistent sexp_variables that are set
- */
-int sexp_campaign_file_variable_count()
-{
-	int count = 0;
-
-	for (int i=0; i<MAX_SEXP_VARIABLES; i++) {
-		if ( (Sexp_variables[i].type & SEXP_VARIABLE_SET) && (Sexp_variables[i].type & SEXP_VARIABLE_IS_PERSISTENT) && !(Sexp_variables[i].type & SEXP_VARIABLE_SAVE_TO_PLAYER_FILE) ) {
-			count++;
-		}
-	}
-
-	return count;
-}
-
 /**
  * Given an index in Sexp_variables, returns the number variables of a type in the array until this point
  */

code/parse/sexp.h

@@ -1108,7 +1108,7 @@ enum class sexp_mode
 #define SEXP_ATOM				2
 
 // flags for sexpressions -- masked onto the end of the type field
-#define SEXP_FLAG_PERSISTENT				(1<<31)		// should this sexp node be persistant across missions
+#define SEXP_FLAG_PERSISTENT				(1<<31)		// should this sexp node be kept across missions, i.e. not freed -- note, NOT the same as variable/container persistence
 #define SEXP_FLAG_VARIABLE					(1<<30)
 
 // sexp variable definitions
@@ -1132,6 +1132,18 @@ enum class sexp_mode
 #define SEXP_VARIABLE_NETWORK				(1<<28)
 #define SEXP_VARIABLE_SAVE_TO_PLAYER_FILE	(1<<27)
 
+// There are three types of persistence for variables and containers:
+// 1. No persistence: the value is only kept for the duration of a mission's gameplay
+// 2. Campaign-persistence: the value is scoped to a campaign, and has no value outside the campaign
+// 3. Player-persistence: the value is scoped to the player/pilot file, and can be referenced in any campaign
+// And there are two ways that persistent variables/containers are saved:
+// 1. When the mission progresses with an outcome that is "accepted" by the player
+// 2. When the mission closes in any way (progress, quit, restart)
+// So, there can be four combinations of persistence (campaign/player times progress/close).  When persistent variables
+// were first implemented, there was an assumption that player-persistence implied save-on-close, and that
+// campaign-persistence implied save-on-progress, and the original mission parsing code reflects that.  But after the
+// 2018 rework, either type can be used with either save.  The 2018 rework also introduced new terminology:
+// "eternal" means player-persistent, and "non-eternal" means campaign-persistent.
 #define SEXP_VARIABLE_IS_PERSISTENT (SEXP_VARIABLE_SAVE_ON_MISSION_PROGRESS|SEXP_VARIABLE_SAVE_ON_MISSION_CLOSE)
 
 #define BLOCK_EXP_SIZE					6
@@ -1517,7 +1529,6 @@ bool sexp_replace_variable_names_with_values(char *text, int max_len);	// Goober
 bool sexp_replace_variable_names_with_values(SCP_string &text);	// Goober5000
 int get_nth_variable_index(int nth, int variable_type);	// Karajorma
 int sexp_variable_count();
-int sexp_campaign_file_variable_count();	// Goober5000
 int sexp_variable_typed_count(int sexp_variables_index, int variable_type); // Karajorma
 void sexp_variable_delete(int index);
 void sexp_variable_sort();

code/parse/sexp_container.cpp

@@ -150,7 +150,7 @@ ContainerType sexp_container::get_data_type() const
 
 bool sexp_container::name_matches(const sexp_container &container) const
 {
-	return !stricmp(container.container_name.c_str(), container_name.c_str());
+	return lcase_equal(container.container_name, container_name);
 }
 
 bool sexp_container::empty() const
@@ -812,18 +812,6 @@ bool sexp_container_CTEXT_helper(int &node, sexp_container &container, SCP_strin
 	}
 }
 
-// inspired by sexp_campaign_file_variable_count()
-bool sexp_container_has_persistent_non_eternal_containers()
-{
-	for (const auto &container : Sexp_containers) {
-		if (container.is_persistent() && !container.is_eternal()) {
-			return true;
-		}
-	}
-
-	return false;
-}
-
 const char *sexp_container_CTEXT(int node)
 {
 	auto *p_container = get_sexp_container(Sexp_nodes[node].text);

code/parse/sexp_container.h

@@ -209,9 +209,6 @@ bool sexp_container_replace_refs_with_values(char *text, size_t max_len);
 
 const char *sexp_container_CTEXT(int node);
 
-// persistence
-bool sexp_container_has_persistent_non_eternal_containers();
-
 // SEXPs
 int sexp_container_eval_status_sexp(int op_num, int node);
 int sexp_container_eval_change_sexp(int op_num, int node);

code/pilotfile/csg.cpp

@@ -1183,7 +1183,13 @@ void pilotfile::csg_read_variables()
 			temp_var.type = cfread_int(cfp);
 			cfread_string_len(temp_var.text, TOKEN_LENGTH, cfp);
 			cfread_string_len(temp_var.variable_name, TOKEN_LENGTH, cfp);
-			Campaign.persistent_variables.push_back(temp_var);
+			Campaign.persistent_variables.push_back(std::move(temp_var));
+
+			// eternal variables belong in the player file
+			Assert(!(temp_var.type & SEXP_VARIABLE_SAVE_TO_PLAYER_FILE));
+			if (temp_var.type & SEXP_VARIABLE_SAVE_TO_PLAYER_FILE) {
+				Campaign.persistent_variables.pop_back();
+			}
 		}
 	}
 
@@ -1202,7 +1208,7 @@ void pilotfile::csg_read_variables()
 				temp_var.type = cfread_int(cfp);
 				cfread_string_len(temp_var.text, TOKEN_LENGTH, cfp);
 				cfread_string_len(temp_var.variable_name, TOKEN_LENGTH, cfp);
-				Campaign.red_alert_variables.push_back(temp_var);
+				Campaign.red_alert_variables.push_back(std::move(temp_var));
 			}
 		}
 	}
@@ -1217,11 +1223,9 @@ void pilotfile::csg_write_variables()
 	cfwrite_int((int)Campaign.persistent_variables.size(), cfp);
 
 	for (idx = 0; idx < (int)Campaign.persistent_variables.size(); idx++) {
-		if (!(Campaign.persistent_variables[idx].type & SEXP_VARIABLE_SAVE_TO_PLAYER_FILE)) {
-			cfwrite_int(Campaign.persistent_variables[idx].type, cfp);
-			cfwrite_string_len(Campaign.persistent_variables[idx].text, cfp);
-			cfwrite_string_len(Campaign.persistent_variables[idx].variable_name, cfp);
-		}
+		cfwrite_int(Campaign.persistent_variables[idx].type, cfp);
+		cfwrite_string_len(Campaign.persistent_variables[idx].text, cfp);
+		cfwrite_string_len(Campaign.persistent_variables[idx].variable_name, cfp);
 	}
 
 	cfwrite_int((int)Campaign.red_alert_variables.size(), cfp);
@@ -1499,6 +1503,12 @@ void pilotfile::csg_read_containers()
 		Campaign.persistent_containers.emplace_back();
 		auto& container = Campaign.persistent_containers.back();
 		csg_read_container(container);
+
+		// eternal containers belong in the player file
+		Assert(!container.is_eternal());
+		if (container.is_eternal()) {
+			Campaign.persistent_containers.pop_back();
+		}
 	}
 
 	Campaign.red_alert_containers.clear();
@@ -1551,7 +1561,6 @@ void pilotfile::csg_write_containers()
 	cfwrite_int((int)Campaign.persistent_containers.size(), cfp);
 
 	for (const auto& container : Campaign.persistent_containers) {
-		Assert(!container.is_eternal()); // eternal containers should be written to player file
 		csg_write_container(container);
 	}
 

code/pilotfile/plr.cpp

@@ -356,7 +356,10 @@ void pilotfile::plr_read_variables()
 		handler->readString("text", n_var.text, TOKEN_LENGTH);
 		handler->readString("variable_name", n_var.variable_name, TOKEN_LENGTH);
 
-		p->variables.push_back( n_var );
+		Assert(n_var.type & SEXP_VARIABLE_SAVE_TO_PLAYER_FILE);
+		if (n_var.type & SEXP_VARIABLE_SAVE_TO_PLAYER_FILE) {
+			p->variables.push_back(std::move(n_var));
+		}
 	}
 	handler->endArrayRead();
 }
@@ -413,6 +416,11 @@ void pilotfile::plr_read_containers()
 		} else {
 			UNREACHABLE("Unknown container type %d", (int)container.type);
 		}
+
+		Assert(container.is_eternal());
+		if (!container.is_eternal()) {
+			p->containers.pop_back();
+		}
 	}
 	handler->endArrayRead();
 }

code/scripting/api/libs/ui.cpp

@@ -804,7 +804,7 @@ ADE_FUNC(skipTraining,
 	// tricky part.  Need to move to the next mission in the campaign.
 	mission_goal_mark_objectives_complete();
 	mission_goal_fail_incomplete();
-	mission_campaign_store_goals_and_events_and_variables();
+	mission_campaign_store_goals_and_events_and_variables(false);
 
 	mission_campaign_eval_next_mission();
 	mission_campaign_mission_over();

freespace2/freespace.cpp

@@ -916,9 +916,9 @@ void game_level_close()
 	//to accidentally use an override here without realizing it.
 	if (!scripting::hooks::OnMissionEndHook->isActive() || !scripting::hooks::OnMissionEndHook->isOverride())
 	{
-		// save player-persistent variables and containers
-		mission_campaign_save_on_close_variables();	// Goober5000
-		mission_campaign_save_on_close_containers(); // jg18
+		// save on-close variables and containers
+		mission_campaign_store_variables(SEXP_VARIABLE_SAVE_ON_MISSION_CLOSE, false);	// Goober5000
+		mission_campaign_store_containers(ContainerType::SAVE_ON_MISSION_CLOSE, false);	// jg18
 
 		// De-Initialize the game subsystems
 		obj_delete_all();