fix persistent variables and containers #7192

Goober5000 · 2026-01-18T05:44:01Z

Storage of persistent variables had been broken since #1631 -- the player-persistent variable code would inadvertently not run if there were no campaign-persistent variables. Additionally, there were several places where the persistent variable logic was hard to follow, which could have hidden other related bugs. This fixes the check and cleans up and consolidates the relevant functions.

code/mission/missioncampaign.cpp

Storage of persistent variables had been broken since scp-fs2open#1631 -- the player-persistent variable code would inadvertently not run if there were no campaign-persistent variables. Additionally, there were several places where the persistent variable logic was hard to follow, which could have hidden other related bugs. This fixes the check and cleans up and consolidates the relevant functions.

jg18

Still reading, but adding a few thoughts.

code/mission/missioncampaign.cpp

jg18 · 2026-01-21T05:51:36Z

code/mission/missioncampaign.cpp

-			}
+	if (store_red_alert) {
+		for (auto& current_rav : Campaign.red_alert_variables) {
+			Campaign.persistent_variables.push_back(current_rav);


We should probably check for naming collisions between existing persistent variables and the RA variables we're adding. Ditto for containers.

jg18 · 2026-01-21T06:06:34Z

code/mission/missioncampaign.cpp

-		} 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
+		if (!any(container.type & persistence_type)) {


Use none() instead of !any().

…iables

jg18

Still studying, but one concern.

code/mission/missioncampaign.cpp

jg18

Tons and tons of good stuff here, some of which I wanted to call out explicitly, but some concerns and suggestions.

jg18 · 2026-01-30T17:28:21Z

code/mission/missioncampaign.cpp

-				if (add_it) {
-					Player->variables.push_back(Sexp_variables[i]);
-				}
+	if (store_red_alert) {


Just wondering why we now save red alert variables at the end, instead of at the start?

jg18 · 2026-01-30T17:30:34Z

code/mission/missioncampaign.cpp

-		} 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")


Optional but strongly encouraged: I think it's worth documenting in code somewhere the 5 types of persistence and what "campaign-persistent"/"player-persistent" originally meant, to avoid potential future confusion, but up to you.

jg18 · 2026-01-30T17:37:37Z

code/mission/missioncampaign.cpp

 			auto ppc_it = std::find_if(Player->containers.begin(),
 				Player->containers.end(),
-				[container](const sexp_container &ppc) {
+				[&container](const sexp_container& ppc) {


Good catch about the &container. I wasn't aware of that syntax.

jg18 · 2026-01-30T17:44:01Z

code/mission/missioncampaign.cpp

 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);


I'm confused on the reason for this change. Were we failing to save "save on mission progress" persistent data?

jg18 · 2026-01-30T17:49:12Z

code/mission/missionparse.cpp

-					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);


Out of scope, but we should probably create a helper function for assigning variables. Too easy to get it wrong or forget a line.

jg18 · 2026-01-30T17:58:27Z

code/pilotfile/plr.cpp

 		}
+
+		Assert(container.is_eternal());
+		if (!container.is_eternal()) {


Same concern as above that we're asserting that a condition is true (i.e., something's wrong if it's false) and taking action if the condition is false.

jg18 · 2026-01-30T17:59:54Z

code/missionui/redalert.cpp

 			gameseq_post_event(GS_EVENT_DEBRIEF);
 		} else {
-			mission_campaign_store_goals_and_events_and_variables();
+			mission_campaign_store_goals_and_events_and_variables(true);


Good catch here.

jg18 · 2026-01-30T18:04:23Z

code/pilotfile/csg.cpp

+
+			// 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) {


Same confusion about asserting that X had better be false but then handling the case where X is true, anyway.

This pattern has a very odd smell and is likely to confuse future readers.

If the code needs to handle the case where X is true, even though it shouldn't happen, but the situation doesn't actually indicate a code bug, then an assertion may be the wrong check to use.

jg18 · 2026-01-30T18:10:56Z

code/mission/missioncampaign.h


 // 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);


Good call on removing default values. Too much potential for hiding bugs, as we've learned here.

jg18 · 2026-01-30T18:20:28Z

code/missionui/missiondebrief.cpp

-			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);


Optional but strongly encouraged: given that

this pair of function calls occurs frequently

there's no scenario where we'd want to call one function but not the other

I suggest exporting a function like

void mission_campaign_store_persistent_data(int persistence_type, bool store_red_alert);

and not exporting these two functions anymore.

The new function can internally do the int-to-ContainerType conversion for storing containers.

jg18

Just noticed a concern about the save_on_close_*() functions getting dropped, even though I think that merging this functionality with the other "store data" function is a good idea overall.

jg18 · 2026-01-30T18:38:41Z

code/mission/missioncampaign.cpp

-
-	// 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;


This "save on close"-specific check is now getting lost, which makes me nervous about potentially introducing regressions.

From a quick look, seems like the difference is that "save on close" applies to single-player campaigns only.

Should "save on close" variables even be supported for multi campaigns?

Goober5000 added this to the Release 25.0 milestone Jan 18, 2026

Goober5000 added cleanup A modification or rewrite of code to make it more understandable or easier to maintain. fix A fix for bugs, not-a-bugs, and/or regressions. labels Jan 18, 2026

Goober5000 force-pushed the variable_fix branch 4 times, most recently from 0f0de2a to ef67aec Compare January 19, 2026 01:43

jg18 reviewed Jan 19, 2026

View reviewed changes

code/mission/missioncampaign.cpp Show resolved Hide resolved

Goober5000 force-pushed the variable_fix branch from ef67aec to adbfb8a Compare January 19, 2026 04:36

Goober5000 force-pushed the variable_fix branch from adbfb8a to 98ecb98 Compare January 19, 2026 04:43

Goober5000 marked this pull request as ready for review January 19, 2026 21:37

jg18 reviewed Jan 21, 2026

View reviewed changes

code/mission/missioncampaign.cpp Show resolved Hide resolved

code/mission/missioncampaign.cpp Show resolved Hide resolved

jg18 reviewed Jan 21, 2026

View reviewed changes

code/mission/missioncampaign.cpp Show resolved Hide resolved

jg18 reviewed Jan 21, 2026

View reviewed changes

check for name conflict between red-alert and persistent campaign var…

3fb7ed5

…iables

jg18 requested changes Jan 23, 2026

View reviewed changes

code/mission/missioncampaign.cpp Show resolved Hide resolved

wookieejedi modified the milestones: Release 25.0, Release 25.0.1 Jan 23, 2026

jg18 reviewed Jan 30, 2026

View reviewed changes

jg18 requested changes Jan 30, 2026

View reviewed changes

fix persistent variables and containers #7192

Are you sure you want to change the base?

fix persistent variables and containers #7192

Uh oh!

Conversation

Goober5000 commented Jan 18, 2026

Uh oh!

Uh oh!

jg18 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jg18 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jg18 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jg18 left a comment

Choose a reason for hiding this comment

Uh oh!

jg18 Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jg18 left a comment •

edited

Loading

jg18 Jan 30, 2026 •

edited

Loading