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.

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

toss out the campaign savefile if we are restarting a previously finished campaign #1450

Merged
merged 2 commits into from Aug 26, 2017

Conversation

Projects
None yet
3 participants
@Goober5000
Contributor

Goober5000 commented Aug 26, 2017

When switching to a previously completed campaign, the game will currently attempt to make further progress due to the existing campaign savefile, which is obviously not possible. This PR deletes the old campaign savefile (which is retail behavior) and sets the next mission to the first in the campaign. This way, the player can start the campaign afresh. Pilot stats are not modified.

This PR fixes Mantis #3174.

Goober5000 added some commits Aug 26, 2017

Goober5000
@asarium

This comment has been minimized.

Show comment
Hide comment
@asarium

asarium Aug 26, 2017

Member

If I understand the changes and your comments correctly then whenever a user selects a finished campaign it will restart it? If that is true it seems a bit counterintuitive since we already have the "Restart campaign" button. If it's retail behavior then it does make sense to make this change but I would like to make sure I understand the changes before approving the code review.

Member

asarium commented Aug 26, 2017

If I understand the changes and your comments correctly then whenever a user selects a finished campaign it will restart it? If that is true it seems a bit counterintuitive since we already have the "Restart campaign" button. If it's retail behavior then it does make sense to make this change but I would like to make sure I understand the changes before approving the code review.

@Goober5000

This comment has been minimized.

Show comment
Hide comment
@Goober5000

Goober5000 Aug 26, 2017

Contributor

You understand correctly. Here's the quote from the relevant Mantis ticket:

If you play through a campaign until it's complete, then switch to a new campaign (and click OK), then try to switch back to the completed campaign (and click OK again), the button will play the error beep and FSO does not leave the campaign room.

Despite this, the asterisk indicator does move to the newly selected campaign, and if you exit the campaign room by pressing Escape, that campaign is now the active one. But when you try to go to the Ready Room, FSO will tell you that the campaign is over.

To actually be able to play the campaign, you need to go back to the campaign room and click Reset Campaign. The behavior should be that when you switch to a new campaign that has been completed, the campaign should start over from the beginning.

I suppose another way to fix it would be to play the success sound instead of the error beep, and ask the user whether he wants to restart the campaign. But I think this fix is the way to go, first because it's retail behavior, and second because there's very little reason to switch to a campaign in the completed state. In fact the only reason I can think of is to view the existing missions and cutscenes, but you can already do that with the Shift-S trick.

The "restart campaign" button is mainly used for going back to the beginning of a campaign that's currently in progress.

Contributor

Goober5000 commented Aug 26, 2017

You understand correctly. Here's the quote from the relevant Mantis ticket:

If you play through a campaign until it's complete, then switch to a new campaign (and click OK), then try to switch back to the completed campaign (and click OK again), the button will play the error beep and FSO does not leave the campaign room.

Despite this, the asterisk indicator does move to the newly selected campaign, and if you exit the campaign room by pressing Escape, that campaign is now the active one. But when you try to go to the Ready Room, FSO will tell you that the campaign is over.

To actually be able to play the campaign, you need to go back to the campaign room and click Reset Campaign. The behavior should be that when you switch to a new campaign that has been completed, the campaign should start over from the beginning.

I suppose another way to fix it would be to play the success sound instead of the error beep, and ask the user whether he wants to restart the campaign. But I think this fix is the way to go, first because it's retail behavior, and second because there's very little reason to switch to a campaign in the completed state. In fact the only reason I can think of is to view the existing missions and cutscenes, but you can already do that with the Shift-S trick.

The "restart campaign" button is mainly used for going back to the beginning of a campaign that's currently in progress.

@asarium

This comment has been minimized.

Show comment
Hide comment
@asarium

asarium Aug 26, 2017

Member

Ok. I just wanted to confirm that my understanding was correct. If it was retail behavior then these changes should be good to go. The alternative solution (showing a dialog) can be implemented separately.

Member

asarium commented Aug 26, 2017

Ok. I just wanted to confirm that my understanding was correct. If it was retail behavior then these changes should be good to go. The alternative solution (showing a dialog) can be implemented separately.

@asarium asarium merged commit 6cf7cb0 into scp-fs2open:master Aug 26, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chief1983

This comment has been minimized.

Show comment
Hide comment
@chief1983

chief1983 Aug 26, 2017

Member

Not sure this wasn't premature. If anything, I feel like tossing campaign stats in retail may have been a bug that the current behavior was supposed to fix. I am pretty sure we have a lot of players who like having long-running pilots with completed campaigns. I think the retail behavior probably only existed because multiple campaign states could not be tracked by the retail pilot code? But now we can track multiple campaign states easily within one pilot, so tossing the data doesn't necessarily make sense at all now. It is counter-intuitive to me at least that simply switching campaigns will eliminate my progress. New users may not know about the 'cheats' to view all missions, cutscenes, etc. We do still get a lot of those from GOG and Steam now. Also, aren't medals tied to your campaign progress as well? I imagine that would be getting tossed out with this then? I believe some users would prefer not to lose those either. It would not have been a big deal in retail because it was known you needed a pilot per campaign to save progress. Now, that we have multi-campaign support within a pilot, dropping progress on change seems very confusing to me. If I misunderstood anything in here please let me know, but I think a non-dropping by default solution needs to be implemented for this soon, or perhaps reverted until a long term solution is devised.

Member

chief1983 commented Aug 26, 2017

Not sure this wasn't premature. If anything, I feel like tossing campaign stats in retail may have been a bug that the current behavior was supposed to fix. I am pretty sure we have a lot of players who like having long-running pilots with completed campaigns. I think the retail behavior probably only existed because multiple campaign states could not be tracked by the retail pilot code? But now we can track multiple campaign states easily within one pilot, so tossing the data doesn't necessarily make sense at all now. It is counter-intuitive to me at least that simply switching campaigns will eliminate my progress. New users may not know about the 'cheats' to view all missions, cutscenes, etc. We do still get a lot of those from GOG and Steam now. Also, aren't medals tied to your campaign progress as well? I imagine that would be getting tossed out with this then? I believe some users would prefer not to lose those either. It would not have been a big deal in retail because it was known you needed a pilot per campaign to save progress. Now, that we have multi-campaign support within a pilot, dropping progress on change seems very confusing to me. If I misunderstood anything in here please let me know, but I think a non-dropping by default solution needs to be implemented for this soon, or perhaps reverted until a long term solution is devised.

@Goober5000

This comment has been minimized.

Show comment
Hide comment
@Goober5000

Goober5000 Aug 26, 2017

Contributor

You've got the situation backwards. This fix only tosses the campaign savefile. It does not modify the pilot stats (as I said in the first post) so the number of kills, weapon shots, points, and so forth are not affected. Furthermore, without this PR, the only way to replay a campaign would be for the player to hit the "Reset Campaign" button, which tosses the savefile anyway.

Ironically, pilot all-time stats are indeed zeroed out when switching campaigns, and they aren't supposed to be. My PR #1451 is an attempt to fix this, but I haven't quite got the full solution yet.

Contributor

Goober5000 commented Aug 26, 2017

You've got the situation backwards. This fix only tosses the campaign savefile. It does not modify the pilot stats (as I said in the first post) so the number of kills, weapon shots, points, and so forth are not affected. Furthermore, without this PR, the only way to replay a campaign would be for the player to hit the "Reset Campaign" button, which tosses the savefile anyway.

Ironically, pilot all-time stats are indeed zeroed out when switching campaigns, and they aren't supposed to be. My PR #1451 is an attempt to fix this, but I haven't quite got the full solution yet.

@Goober5000 Goober5000 deleted the Goober5000:mantis_3174_fix branch Aug 26, 2017

@chief1983

This comment has been minimized.

Show comment
Hide comment
@chief1983

chief1983 Aug 26, 2017

Member

Ok, if stats are global to the pilot and not a campaign, that partially helps. Still, what about medals? Those are per campaign right? Or is that per mod/TC? Also, it still seems intuitive to me that the Reset Campaign button would be the way to reset your campaign, and not that it would 'just happen' when changing campaigns.

Member

chief1983 commented Aug 26, 2017

Ok, if stats are global to the pilot and not a campaign, that partially helps. Still, what about medals? Those are per campaign right? Or is that per mod/TC? Also, it still seems intuitive to me that the Reset Campaign button would be the way to reset your campaign, and not that it would 'just happen' when changing campaigns.

@Goober5000

This comment has been minimized.

Show comment
Hide comment
@Goober5000

Goober5000 Aug 26, 2017

Contributor

No, all-time medals are global to the pilot just like all-time ship kills are. There is an index_list_t type that handles any table index mismatch.

I guess "intuitive" is a matter of opinion; it seems intuitive to me that switching to a completed campaign would automatically reset it. Requiring the player to manually reset it is an extra step.

Again, if you can come up with any reason to keep the campaign savefile in the "this campaign is over" state, do share that, because I can't think of one.

Contributor

Goober5000 commented Aug 26, 2017

No, all-time medals are global to the pilot just like all-time ship kills are. There is an index_list_t type that handles any table index mismatch.

I guess "intuitive" is a matter of opinion; it seems intuitive to me that switching to a completed campaign would automatically reset it. Requiring the player to manually reset it is an extra step.

Again, if you can come up with any reason to keep the campaign savefile in the "this campaign is over" state, do share that, because I can't think of one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment