[niffiwan]

As I can't compile FRED, I was hoping that another coder could help me out with what should be a really simple change FRED change.  Per this feature for Axem, I'd like to add a new mission flag to FRED. It's pretty simple, "MISSION_FLAG_END_TO_MAINHALL" which... is mostly self explanatory, the only difference is that unlike the current "end-mission" sexp it marks the current mission as completed.

(note that the final commit in the branch I linked above is my poor & coded blind attempt at copying another commit that also seemed to add a mission flag to FRED)

[karajorma]

I could probably add that tomorrow without much issue.

While we're talking about changes, I've got a couple of issues with r10587. I'm seeing lots of checks for MULTIPLAYER_MASTER. These are pretty redundant as the multi_send_ functions should be checking for this anyway (They check in cannot_send_data() ) and I choose to do it there deliberately so as to avoid making the SEXP code much harder to read with lots of if () statements. I'll admit I actually did that myself in the original function but there's no need to repeat my mistake.

I'm going to need to take a look at your changes though. I think they're going to result in the other object flags getting set twice on the client. That shouldn't really have any effect at the moment, but I suspect it's a problem waiting to happen.

[Goober5000]

As I can't compile FRED, I was hoping that another coder could help me out with what should be a really simple change FRED change.  Per this feature for Axem, I'd like to add a new mission flag to FRED. It's pretty simple, "MISSION_FLAG_END_TO_MAINHALL" which... is mostly self explanatory, the only difference is that unlike the current "end-mission" sexp it marks the current mission as completed.

Why are you using a mission flag for this?  Why aren't you adding a new parameter to the end-mission sexp itself?

EDIT: I see that you are using an extra parameter, but the extra parameter sets the flag. I want to take a look at this; there must be some way it can be achieved by posting an event.

Also, this raises an interesting distinction in the end-mission sexp.  That sexp does mark the mission as completed if it goes to debriefing -- by design -- but does not mark the mission as completed if it goes to the main hall -- also by design.  (This matches retail FS2 behavior, but Axem has apparently found that it's not sufficient for all cases.)

[The E]

I wonder what the rationale behind this was. Is it just to allow people to replay the last mission directly from the mainhall, or what?

[Axem]

Well I was thinking that in campaigns, a FREDder may wish to insert a "break" in the campaign for an act change or mainhall change (a hard core player could run through the entire FS2 campaign without ever seeing the Vasudan main hall), or to let them explore the tech room/intel with new entries (like X-Wing Alliance).

I realize there is a bit of potential trouble for life long FreeSpace 2 players to assume that "main hall = campaign over", but I've got some other tricks for that. Like a lua function that could return the next campaign mission's filename could allow a notification of sorts to the player. Like "Next Mission: Journey to Babel" or something.

[Goober5000]

I wonder what the rationale behind this was. Is it just to allow people to replay the last mission directly from the mainhall, or what?

The "dump the player to the main hall" parameter was originally added to allow non-standard "campaign over" conditions.  If the player commits an epic fail that by rights should mean the end of the campaign, the mission will fade out and the end-mission sexp will kick him out to the main hall -- but he has the option to replay the mission without completely restarting the campaign.  This matches the retail FS2 campaign where death-by-supernova is the non-standard campaign ending, but the player can replay the mission to get the "winning" ending.

[niffiwan]

I could probably add that tomorrow without much issue.

TYVM (assuming that this goes ahead with a mission flag involved)

While we're talking about changes, I've got a couple of issues with r10587. I'm seeing lots of checks for MULTIPLAYER_MASTER. These are pretty redundant as the multi_send_ functions should be checking for this anyway (They check in cannot_send_data() ) and I choose to do it there deliberately so as to avoid making the SEXP code much harder to read with lots of if () statements. I'll admit I actually did that myself in the original function but there's no need to repeat my mistake.

I'm going to need to take a look at your changes though. I think they're going to result in the other object flags getting set twice on the client. That shouldn't really have any effect at the moment, but I suspect it's a problem waiting to happen.

As this isn't related to this patch could we please take this discussion to a new thread?

Why are you using a mission flag for this?  Why aren't you adding a new parameter to the end-mission sexp itself?

EDIT: I see that you are using an extra parameter, but the extra parameter sets the flag. I want to take a look at this; there must be some way it can be achieved by posting an event.

Also, this raises an interesting distinction in the end-mission sexp.  That sexp does mark the mission as completed if it goes to debriefing -- by design -- but does not mark the mission as completed if it goes to the main hall -- also by design.  (This matches retail FS2 behavior, but Axem has apparently found that it's not sufficient for all cases.)

I used a mission flag for several reasons:

1) Flexibility, one use case (as Axem mentioned) was to dump a player to the mainhall so they could see a new mainhall (or intel entries), this would most likely be required for any successful outcome of the mission, therefore the use of the mission flag means an extra (otherwise redundant) end-mission sexp is not required.
2) IIRC, none of the GAME_EVENTS will mark the mission as completed when called from a sexp. That's why the current end-mission (with 2nd arg == TRUE which posts GS_EVENT_END_GAME) doesn't mark the mission as complete (I've also checked the rest of the events in gamesequence.h and from their names, none of them seem like they would work either)
3) A sexp-only solution won't allow a skipped training mission to end-mission-as-completed-and-to-mainhall (sexp's don't run in the briefing, but the mission flag is available). Ditto for skipping the mission after dying 5 times.

Lastly, I'm not sure why adding a mission flag is not preferred. Is there something I'm missing?

[General Battuta]

I wonder what the rationale behind this was. Is it just to allow people to replay the last mission directly from the mainhall, or what?

The "dump the player to the main hall" parameter was originally added to allow non-standard "campaign over" conditions.  If the player commits an epic fail that by rights should mean the end of the campaign, the mission will fade out and the end-mission sexp will kick him out to the main hall -- but he has the option to replay the mission without completely restarting the campaign.  This matches the retail FS2 campaign where death-by-supernova is the non-standard campaign ending, but the player can replay the mission to get the "winning" ending.

I'd argue that dying in the supernova is the subtextually 'correct' and most victorious ending to the campaign, but that's neither here nor there.

[karajorma]

The problem with a mission flag is that we then don't have an option if we want some mission outcomes to kick us to the mainhall while others don't. It's kinda like the stupid limitation we have with Red Alert mission. Although I suppose it might be possible to have a SEXP set the mission flag. Coding something like an alter-mission-flag SEXP isn't a bad idea.

I'll wait for the discussion to be resolved before I add the flag to FRED.

[niffiwan]

The problem with a mission flag is that we then don't have an option if we want some mission outcomes to kick us to the mainhall while others don't. It's kinda like the stupid limitation we have with Red Alert mission. Although I suppose it might be possible to have a SEXP set the mission flag. Coding something like an alter-mission-flag SEXP isn't a bad idea.

Yep - so I added the mission flag for the blanket case, and modified the end-mission sexp to set the mission flag conditionally if some outcomes don't warrant being sent to the mainhall (e.g. new ships didn't show up in the mission so their tech entries weren't added and there's nothing new to see).

And yes, an alter-mission-flag sounds pretty good

[Goober5000]

An alter-mission-flag sexp is a great idea, but as far as this particular feature, I want to look at the game state code.  I should have time in the next day or two.

[Goober5000]

Eh, s/day/month/ or something.

Now that I've actually had time to lookd at the state code, I see why niffiwan tied things to a mission flag.  There's a bunch of mission cleanup and campaign saving code that doesn't get called until the debriefing is accepted, and niffiwan's new flag is needed to allow this to run.  So I'm fine with this approach, in principle.

As for the patch itself, I notice that one of the newly added lines is

Code: [Select]

if ( m_always_show_goals ) { (line 362, seen here).  This is a mismatch from the actual variable added, which is m_end_to_mainhall.

Also, you should modify the comment "// go to main hall, tha campaign is over!" at line 1792 here.  This is vitally important, because the comment was written for an earlier use and is now no longer accurate, and it is likely to confuse some future coder who sees it.  (One of my pet peeves is seeing coders update a section of code in such a way as to render the comments incorrect without updating the comments.  It happens all the time, frustratingly.)  But this paragraph may be obsolete, because...

As for the actual implementation of the patch, it's hard for me to tell without actually patching my local code, but I think the patch is a lot more tangled than it needs to be.  We already have the "no debriefing" flag, which makes this much simpler.  Just set the no-debriefing flag in end-mission, and then in debrief_accept, check for the end-to-main-hall flag and post a main hall event rather than a start-mission event.

[niffiwan]

Thanks for the review! I'll have a look at implementing those changes once I finish up the current mantis issue I'm working on.

[niffiwan]

OK, so I've fixed up the issue with m_always_show_goals and the comment(s).  The next bit though, about using the "no debriefing" flag has confused me.  Skipping the debriefing is not the desired behaviour, we want the normal debrief, then enter the mainhall after the debriefing is accepted. If the FREDer wants "no debriefing" and "end to mainhall" then my changes should still let that work, as "toggle debriefing" just auto-accepts the debriefing (calling debrief_accept() which is where my MISSION_FLAG_END_TO_MAINHALL check is).

Does that change your views on associating the 'end-to-mainhall' flag with the 'no debriefing' flag?

(Have I missed something?)

(new branch with the other fixes is here: https://github.com/niffiwan/fs2open.github.com/commits/end-mission-and-advance-v2)

[karajorma]

I think you should be able to do both. End to mainhall shouldn't be dependent on whether there is a debriefing or not. If that's the way it's coded, fine. But if adding the no-debrief flag means you don't check if you should go to the mainhall, then that needs to be fixed.

[Goober5000]

OK, so I've fixed up the issue with m_always_show_goals and the comment(s).

Cool.

The next bit though, about using the "no debriefing" flag has confused me.  Skipping the debriefing is not the desired behaviour

Oh, okay.  I thought it was.  One of my points was that if it was the desired behavior, we already had a flag for that, and that flag could be set in the relevant part of the end-mission sexp.

As for the other point (which began with "and then in debrief_accept..."), here's the change I had in mind:

In place of this:

Code: [Select]

if ( ok_to_post_start_game_event ) {
gameseq_post_event(GS_EVENT_START_GAME);
} else {
play_commit_sound = 0;
}
Use this:

Code: [Select]

if ( ok_to_post_start_game_event ) {
if (The_mission.flags & MISSION_FLAG_GOTO_MAIN_HALL) {
gameseq_post_event(GS_EVENT_MAIN_MENU);
} else {
gameseq_post_event(GS_EVENT_START_GAME);
}
} else {
play_commit_sound = 0;
}
That should should be the only place where you need to change code to make the flag work.

[niffiwan]

Only having the check for GOTO_MAINHALL in missiondebrief.cpp:debrief_accept(int ok_to_post_start_game_event) would work fine for the conventional mission-end case, but it wouldn't cover these two mission-end cases:

1) skipping a training mission (the end of training seems like a reasonable place to send the player to the mainhall to view new tech entries which is the reason for the change in missionbrief.cpp:brief_skip_training_pressed())
2) skipping a mission when the player has died 5 times (your suggestion would be correct if GOTO_MAINHALL was linked with NO_DEBRIEFING, but when they are separate it won't be - for completeness this is the change in missioncampaign.cpp:mission_campaign_skip_to_next(int start_game))

I've confirmed this by removing the changes in these two files and playing through the test campaign attached to the mantis ticket.

Does that sufficiently explain why those two files were included in the patch?

[Goober5000]

Aha, I see.  Well then, you are obviously correct on these two points.

[niffiwan]

Hurrah!

Karajorma, if you've still got time would you be able to see if you can add that new flag to FRED? This is my attempt at adding it: https://github.com/niffiwan/fs2open.github.com/commit/ca23258f683a099c87220459eb704dbb6e2b3f96.  I just copied what you'd done in r10279.

[karajorma]

Sure, I'll take a look at it sometime this week.