fix persistent variables and containers #7192
Conversation
Goober5000
added
cleanup
Goober5000
force-pushed
the
variable_fix
branch
4 times, most recently
from
0f0de2a to
ef67aec
Compare
| } else { | ||
| // new player-persistent container | ||
| Player->containers.emplace_back(container); | ||
| Player->containers.push_back(container); |
There was a problem hiding this comment.
Why push_back()?
Campaign.persistent_containers below uses emplace_back() to add a new container.
There was a problem hiding this comment.
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/
Goober5000
force-pushed
the
variable_fix
branch
from
ef67aec to
adbfb8a
Compare
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.
Goober5000
force-pushed
the
variable_fix
branch
from
adbfb8a to
98ecb98
Compare
jg18
left a comment
jg18
left a comment
There was a problem hiding this comment.
Still reading, but adding a few thoughts.
| // 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); |
There was a problem hiding this comment.
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++.
There was a problem hiding this comment.
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.
| Campaign.persistent_variables.push_back(Sexp_variables[i]); | ||
| } | ||
| } | ||
| // player-persistent (aka "eternal") |
There was a problem hiding this comment.
Wait, I thought "player-persistent" != "eternal"?
IIRC:
- player-persistent: "save on mission close"
- campaign-persistent: "save on mission progress"
- eternal: "save to player file"
There was a problem hiding this comment.
sexp.cpp:5059 and also on lines 5048-5056 back up my recollection.
There was a problem hiding this comment.
"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.
| cur = Campaign.current_mission; | ||
|
|
||
| mission_obj = &Campaign.missions[cur]; | ||
| int cur = Campaign.current_mission; |
There was a problem hiding this comment.
Since this functionality occurs three times in this file, consider adding a helper function
cmission* get_current_mission();and calling that.
| } | ||
| if (store_red_alert) { | ||
| for (auto& current_rav : Campaign.red_alert_variables) { | ||
| Campaign.persistent_variables.push_back(current_rav); |
There was a problem hiding this comment.
We should probably check for naming collisions between existing persistent variables and the RA variables we're adding. Ditto for containers.
| } 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)) { |
2 participants
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.