Skip to content

fix persistent variables and containers #7192

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Goober5000 wants to merge 1 commit into
base: master
Choose a base branch
from
+157 −298
Open

fix persistent variables and containers #7192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
238 changes: 69 additions & 169 deletions code/mission/missioncampaign.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -928,15 +928,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;
Copy link
Member

@jg18 jg18 Jan 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this functionality occurs three times in this file, consider adding a helper function

cmission* get_current_mission();

and calling that.

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
Expand Down Expand Up @@ -992,66 +988,49 @@ 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];

// handle variables that are saved on mission victory -------------------------------------
int cur = Campaign.current_mission;
auto mission_obj = &Campaign.missions[cur];
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);
}
if (store_red_alert) {
for (auto& current_rav : Campaign.red_alert_variables) {
Campaign.persistent_variables.push_back(current_rav);
Copy link
Member

@jg18 jg18 Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

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;
}
}
int num_sexp_variables = sexp_variable_count();
for (int i = 0; i < num_sexp_variables; i++) {
if (!(Sexp_variables[i].type & persistence_type)) {
continue;
}

// new variable
if (add_it) {
Campaign.persistent_variables.push_back(Sexp_variables[i]);
}
}
// player-persistent (aka "eternal")
Copy link
Member

@jg18 jg18 Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, I thought "player-persistent" != "eternal"?

IIRC:

  • player-persistent: "save on mission close"
  • campaign-persistent: "save on mission progress"
  • eternal: "save to player file"

Copy link
Member

@jg18 jg18 Jan 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sexp.cpp:5059 and also on lines 5048-5056 back up my recollection.

Copy link
Contributor Author

@Goober5000 Goober5000 Jan 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"player-persistent" has always meant "save to player file", and "campaign-persistent" has always meant "save to campaign file". A lot of the current code uses "eternal" and "non-eternal" to mean the same things, respectively. But originally "player-persistent" also carried the implicit additional meaning of "save-on-mission-close" whereas "campaign-persistent" also carried the implicit additional meaning of "save-on-mission-progress". This was the main motivation for the upgrade in 2018.

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);
Copy link
Member

@jg18 jg18 Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personal opinion is that find_item_with_string() is hard to understand, both how it's called and how it's implemented.

Given that player::variables is just an SCP_vector, I'd think that std::find_if() with a lambda as predicate would be both easier to understand and more idiomatic to C++.

Copy link
Contributor Author

@Goober5000 Goober5000 Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be more idiomatic, but it's also more verbose. The new find_item_with_string and related functions are intended to be shorthand versions of handling the common cases. The main confusing part is the pointer-to-member-field; if you ignore that, it works just like any standard index function.

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;

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]);
}
// 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]);
}
}
}
Expand All @@ -1063,58 +1042,57 @@ 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);
Campaign.persistent_containers.push_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 (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
if (!any(container.type & persistence_type)) {
Copy link
Member

@jg18 jg18 Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use none() instead of !any().

continue;
}

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

if (ppc_it != Player->containers.end()) {
*ppc_it = container;
} else {
// new player-persistent container
Player->containers.emplace_back(container);
Player->containers.push_back(container);
Copy link
Member

@jg18 jg18 Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why push_back()?

Campaign.persistent_containers below uses emplace_back() to add a new container.

Copy link
Contributor Author

@Goober5000 Goober5000 Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well they should indeed be consistent; thanks for catching that.

I tend to use push_back unless I specifically need emplace_back, e.g. if I'm constructing a new element in-place. I've seen push_back recommended as a rule-of-thumb, such as here:
https://quuxplusone.github.io/blog/2021/03/03/push-back-emplace-back/

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

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

/**
Expand Down Expand Up @@ -1146,9 +1124,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 ) {
Expand Down Expand Up @@ -1682,6 +1657,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();
Expand Down Expand Up @@ -1728,7 +1706,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();
Expand Down Expand Up @@ -1845,84 +1823,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) {
Expand Down
12 changes: 3 additions & 9 deletions code/mission/missioncampaign.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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);
Expand Down
Loading