fix(mechanics): Properly disable triggers for failed missions

[tibetiroka]

Bug fix

This PR fixes a bug where mission triggers would activate for failed missions.

Summary

Mission triggers such as on disabled were missing checks for the mission state. This PR wraps all Mission::Do actions in a check, making them a no-on when the mission is failed.

Testing Done

I tested that the on disabled trigger doesn't trigger anymore for failed missions. I'm still testing whether this breaks any existing content; especially the NPC triggers.

Performance Impact

N/A

[TomGoodIdea]

Looks like someone forgot our bracket style

[TomGoodIdea]

If you turn the check into an if(HasFailed(player)) return; guard at the beginning, you don't need to change indentation of the whole function.

[tibetiroka]

Yeah, but that would make it harder to exclude some triggers from that check. I'm still worried that some NPC things could happen after the mission is failed...

I should go and check if on fail still works with this fix, now that I think about it...

[Amazinite]

Check on abort too.

[TomGoodIdea]

I think on abort should work correctly, but other triggers handled by the Mission::Do(Trigger ...) function don't check for failure.

[tibetiroka]

on fail and on abort still works. NPC triggers are kinda broken; they can fail a mission then do something, which now gets ignored.

[tibetiroka]

This change restores the previous NPC behaviour. This means NPCs can still fail a mission before showing a popup dialog.

The issue with this is that if there are multiple NPCs, none of them are checking the mission state, so you can get the popups even if the mission failed in a previous frame. (This is same as the previous behaviour, btw.)

I'm not sure how to fix it. My best idea so far is to only fail a mission at the end of a frame (or only update the value checked in Mission:Do at that time, and update it instantly for other functions checking the mission); that would prevent the above NPC issue while allowing a single trigger to execute multiple functions.

The real issue is that I can't tell if people were relying on the previous, bugged behaviour. If we fix the NPC issue, then you can abort a mission and shoot the NPCs without any record of it being made, which could cause issues if they were carrying some VIPs in a storyline. (Even the same ships showing up could be an issue story-wise.)

Test mission:

mission "Test Mission"
	source "New Boston"
	repeat
	landing
	on offer
		conversation
			choice
				`Mission.`
					accept
	npc disable
		government "Test Dummy"
		personality entering
		ship "Star Barge" "Tester"
		on kill
			fail "Test Mission"
			dialog
				"(Crying.)"
	npc disable
		government "Test Dummy"
		personality entering
		ship "Star Barge" "Tester 2"
		on kill
			dialog
				"(Crying.)"
	on disabled
		dialog
			`Disabled.`
	on fail
		dialog
			`Failed.`
	on abort
		dialog
			`Aborted.`
	on complete
		dialog
			`Done.`

[TomGoodIdea]

The real issue is that I can't tell if people were relying on the previous, bugged behaviour. If we fix the NPC issue, then you can abort a mission and shoot the NPCs without any record of it being made, which could cause issues if they were carrying some VIPs in a storyline. (Even the same ships showing up could be an issue story-wise.)

This is what the Unstable releases are for, right? To check if new features/fixes break things. If we get many complaints, we can revert it.

[TomGoodIdea]

This is the only place where Mission::Do(Mission::STOPOVER, ...) (the other "Do" function) is called and I'm pretty sure it means it still can be triggered if failed (probably shouldn't?).

endless-sky/source/PlayerInfo.cpp

Lines 4138 to 4142 in a01426e

	// If this is a stopover for the mission, perform the stopover action.
	mission.Do(Mission::STOPOVER, *this, ui);
	if(mission.IsFailed(*this))
	RemoveMission(Mission::FAIL, mission, ui);

[TomGoodIdea]

The flagship always belongs to the player, so you can move this to the if(event.TargetGovernment()->IsPlayer()) block above. yeah ik it wasn't there before, but since you're changing it anyway...

[tibetiroka]

I don't really see the point tbh, we aren't using it anywhere else.

[Amazinite]

I'm actually going to question if this should even be considered a bug. To me, saying this is bugged would imply that the original intention was for actions to not trigger after the mission has already failed. I don't get the impression that this was ever an intention, though, especially as this sounds like how missions have always behaved. The Do function for mission actions didn't check the failed state of the mission, and the Do function for ship events only checked the failed state to prevent displaying irrelevant messages.

Given that the behavior for so long has been to trigger the action when it's possible to do so, regardless of whether the mission itself has already failed, I think I'd prefer some method of explicitly flagging actions as being unable to run should the mission have already failed. That way we can pick and choose when we feel that it makes sense for an action to not run if the mission has failed.

[tibetiroka]

To me, saying this is bugged would imply that the original intention was for actions to not trigger after the mission has already failed. I don't get the impression that this was ever an intention, though, especially as this sounds like how missions have always behaved. The Do function for mission actions didn't check the failed state of the mission, and the Do function for ship events only checked the failed state to prevent displaying irrelevant messages.

Looking at #8606, where the on disabled trigger was added (which was causing the trouble), it seems that the on disabled is the first trigger that didn't already check the mission state somewhere else. I'm sure we can ask @TomGoodIdea whether that trigger was meant to work even after a mission is failed, but I see no indication of that in the PR.

This probably didn't come up as an issue before because there are no uses of on disabled in the game. The one PR where I saw it being used (Hai Legacy) was broken by the trigger unexpectedly triggering even after the mission failed.

Honestly, I'm having trouble thinking of a scenario where we would want such a trigger to work for a failed mission - in fact, failed missions should try to interact with the player as little as possible.

[TomGoodIdea]

In #8606 I thought that if mission triggers shouldn't work for failed missions, it's most likely handled somewhere else already. I probably looked at the waypoint trigger, which also doesn't check for failure.

[Amazinite]

Honestly, I'm having trouble thinking of a scenario where we would want such a trigger to work for a failed mission - in fact, failed missions should try to interact with the player as little as possible.

Since a failed mission will trigger its on fail action the moment you land, we wouldn't want any other actions that require you to be landed first to trigger. I believe this is already the case.

That means that we only need to consider actions which can be triggered in space, which includes the following:

• enter: Doesn't check the failed state.
• waypoint: Doesn't check the failed state.
• disabled: Doesn't check the failed state.
• daily: This one does check if you've failed the mission.
  

  endless-sky/source/PlayerInfo.cpp

  Lines 742 to 743 in ae5c7f1

  	if(!mission.IsFailed(*this))
  	mission.Do(Mission::DAILY, *this);

• NPC actions: Doesn't check the failed state.
• the old way of triggering dialogs and conversations when an NPC's objective is complete: Checks the failed state of the NPC itself, rather than the entire mission's failed state.

You have already noted how running an action after the mission has already failed can have a use.

If we fix the NPC issue, then you can abort a mission and shoot the NPCs without any record of it being made, which could cause issues if they were carrying some VIPs in a storyline.

Every action should check CanBeDone() before it is run. NPC actions currently don't do this, but #9830 makes it so that they do. Mission actions make this check, including daily, enter, disabled, and waypoint. CanBeDone() has a parameter for the player object and is called in locations that have access to the Mission object. Perhaps we make it so that CanBeDone() checks if the mission has been failed and returns false if this is the case, unless the action has some sort of "can run when failed" flag for the uncommon case where we actually do want the action to run even after the mission has failed. That is, the default would be to not trigger actions on failed missions, but we could have the option to still trigger stuff before the mission has been removed if we'd like.

[Hurleveur]

The fact it only triggers the failing convo on landing keeps bugging me when I write missions that can fail

[Amazinite]

I do think we should eventually have a means on triggering an action the instant a mission fails.

[tibetiroka]

The string token is a placeholder, and I need to test that it works at all.

[tibetiroka]

It's a hacky workaround, but it works; the previous behaviour can now be restored by adding "triggers for failed missions" as the first line in the mission action.

I do think creating separate actions for these would be better (something like on "disabled after fail"), though. Any thoughts?

[RisingLeaf]

I don't like the length of the token name but other than that it looks ok

[tibetiroka]

Yeah the token is just a placeholder for now

[Amazinite]

I do think creating separate actions for these would be better (something like on "disabled after fail"), though. Any thoughts?

I feel like it would bloat things to have duplicate action types for whether they can or can't (or only) trigger after the mission has failed. It would also only allow new functionality in very, very specific use cases.

I think a more customizable solution would be to allow multiple action nodes of the same type to exist under a mission. (We might leave some types out, such as on offer and on complete, but various other action types could conceivable appear multiple times.) Action nodes could then have a to trigger child which is a condition set that is evaluated in CanBeDone(). If multiple actions of the same type exist in a mission, it will then only run those actions where CanBeDone() is true. That way you could have something like this:

# This action always runs regardless.
on disabled
	...
# This action can only run if the mission has not been failed.
on disabled
	to trigger
		not "this mission: failed"
	...
# This action can only run if the mission has been failed.
on disabled
	to trigger
		has "this mission: failed"
	...

[tibetiroka]

That sounds really interesting. It's beyond the scope of this PR, but it would certainly enable more functionality.

An alternative could be to allow giving conditions to any action node. Or maybe both?