Possible regression: explosion-effect sexp number out of range

[ariel-]

As documented here
@niffiwan

Steps to reproduce:

• Get latest Blue Planet: Complete and FSO nightly from Knossos
• Execute with following cmdline:
  fs2_open_3_8_1_20190205_caea0f5_x64_SSE2-FASTDBG.exe -window -mod blueplanetcomplete-1.0.1,MVPS-3.7.2 -start_mission bp2-23.fs2 -parse_cmdline_only

Reference log: https://fsnebula.org/log/5c77691dcb0d3350a87569c1

This behaviour was not present in 3.8.0-3 release version

[Goober5000]

This looks like it is related to the fireball.tbl changes I added recently. Will take a look.

[Goober5000]

I tried several times to run Blue Planet Complete in a debug build (within the Visual Studio debugger) and kept getting Out Of Memory errors.

However, from a visual inspection of the log and the mod, this is almost certainly a mod bug that was uncovered by the stricter error checking in the new fireballs.tbl. Note that there have only ever been six default fireballs in the table, numbered 0 through 5; an index of 6 is out of range. Here are the definitions from 3.8.0...

#define FIREBALL_EXPLOSION_MEDIUM	0		// Used for the 4 little explosions before a ship explodes
#define FIREBALL_WARP				1		// Used for the warp in / warp out effect
#define FIREBALL_KNOSSOS			2		// Used for the KNOSSOS warp in / warp out effect
#define FIREBALL_ASTEROID			3
#define FIREBALL_EXPLOSION_LARGE1	4		// Used for the big explosion when a ship breaks into pieces
#define FIREBALL_EXPLOSION_LARGE2	5		// Used for the big explosion when a ship breaks into pieces

There are no fireballs.tbl or *-fbl.tbm tables in bpc-core.vp, and mv_effects-fbl.tbm just modifies the existing entries without adding any new ones. Thus there could never have been a fireball with an index of 6, even in 3.8.0.

The only change in behavior here is that in the new code, this error is caught when the mission is parsed rather than silently failing at the time the sexp is triggered.

[MageKing17]

Why is this a fatal error rather than a warning, though? There's a really obvious recovery path for FSO to take: namely, just keep running without spawning a nonexistent fireball.

[Goober5000]

All parse errors halt mission load in this way. An out of range numeric literal for a sexp data type has always been treated as a parse error.

If somehow the mission managed to load (e.g. if the value were returned from a variable) and this sexp were invoked, then the fireball would be skipped and the mission would keep running.

[MageKing17]

All parse errors halt mission load in this way.

Yes, but why?

[Goober5000]

You would have to ask Volition. The relevant code block is here:

fs2open.github.com/code/mission/missionparse.cpp

Lines 5841 to 5858 in 223e789

	// entering this if statement will result in program termination!!!!!
	// print out an error based on the return value from check_sexp_syntax()
	if ( result ) {
	SCP_string sexp_str;
	SCP_string error_msg;
	convert_sexp_to_string(sexp_str, i, SEXP_ERROR_CHECK_MODE);
	truncate_message_lines(sexp_str, 30);
	sprintf(error_msg, "%s.\n\nIn sexpression: %s\n(Error appears to be: %s)", sexp_error_message(result), sexp_str.c_str(), Sexp_nodes[bad_node].text);
	if (!Fred_running) {
	nprintf(("Error", "%s", error_msg.c_str()));
	Error(LOCATION, "%s", error_msg.c_str());
	} else {
	nprintf(("Warning", "%s", error_msg.c_str()));
	Warning(LOCATION, "%s", error_msg.c_str());
	}
	}

[MageKing17]

You would have to ask Volition.

I'm not interested in who implemented the behavior; I'm asking why we should keep it.

[Goober5000]

Because generally a mission that cannot be parsed is one that cannot be run. There are a whole class of parse errors of which this is only one variant. Other possibilities include an unknown operator or a missing parenthesis.

I wish you would say what you mean. My impression is that you really want to know if there's a more user-friendly way to handle missions that can't be parsed. That can be changed from a hard crash to simply aborting with an "Attempt to load the mission failed" message. I can make a PR for this easily enough.

[MageKing17]

Because generally a mission that cannot be parsed is one that cannot be run.

I'll bet I could make it run real easily; namely, by just ignoring this one SEXP that happens to have an invalid argument.

I wish you would say what you mean. My impression is that you really want to know if there's a more user-friendly way to handle missions that can't be parsed.

I thought I was fairly clear when I said I wanted to know why we're halting execution when an obvious recovery path is available, but maybe I just need to rephrase it for clarity, so here goes: halting execution when we get bad data that can easily be ignored is really stupid behavior. Stopping loading the mission is... better, but still not good. I suppose it's a fallback if we really can't ignore something, but I can't imagine what scenario that is other than "file doesn't appear to be a .fs2 at all". I mean, if a parsing error like this were truly unrecoverable, it wouldn't be a Warning in FRED, but it is, because recovering from it isn't really all that hard.

Now, I'm not saying I have time to refactor mission loading to recover more gracefully from parsing errors, nor am I demanding anyone else do so. I'm just asking if there's a good reason for us not to eventually get around to it, because "Volition did it" isn't a good reason (and neither is "the mission won't run as intended"; lots of bad data doesn't work as intended, and the engine still loads it).

[chief1983]

I can think of one potential reason it's a good idea to stop. Just because you can recover from it would not seem to guarantee that anything like the intended behavior is what actually happens. Stopping forces someone to clean up the mess now, rather than sweep it under the rug. I know we do this for other kinds of data, but perhaps with SEXPs, the possibility of snowballing errors and obscure behavior changes being attributed incorrectly when it's really due to a parse error would be greater than many other places. I don't know that this is actually the case but maybe it's close to the reason it was done in the first place?

[Goober5000]

Because generally a mission that cannot be parsed is one that cannot be run.

I'll bet I could make it run real easily; namely, by just ignoring this one SEXP that happens to have an invalid argument.

Which would work fine in this specific case. Not every case is this clear-cut.

I thought I was fairly clear when I said I wanted to know why we're halting execution when an obvious recovery path is available, but maybe I just need to rephrase it for clarity, so here goes: halting execution when we get bad data that can easily be ignored is really stupid behavior. Stopping loading the mission is... better, but still not good. I suppose it's a fallback if we really can't ignore something, but I can't imagine what scenario that is other than "file doesn't appear to be a .fs2 at all". I mean, if a parsing error like this were truly unrecoverable, it wouldn't be a Warning in FRED, but it is, because recovering from it isn't really all that hard.

If I run across this in FRED (and on infrequent occasions I have), I usually need to open up Notepad++, correct the error manually, and reload the mission. If I try to correct the error in FRED itself I sometimes end up with deleted sexps (because do-some-strange-action is not recognized) or invalid references. If the error is a missing parenthesis then the entire mission will be hosed. So in general, recovery cannot be assumed. There are, of course, several specific cases where it works just fine.

Now, I'm not saying I have time to refactor mission loading to recover more gracefully from parsing errors, nor am I demanding anyone else do so. I'm just asking if there's a good reason for us not to eventually get around to it, because "Volition did it" isn't a good reason (and neither is "the mission won't run as intended"; lots of bad data doesn't work as intended, and the engine still loads it).

The block of code I highlighted is the first line of defense. If this is bypassed, the player is likely to encounter Assertion errors or other unfriendly error conditions that are intended for programmers rather than end-users.

One probably safe way is to mark the entire event as invalid so that the game never tries to run any bad sexps, but what would be the point of this? Most events have some story-critical information or actions, and the FREDder deserves to have his mission experienced as intended. I consider "the mission won't run as intended" to be an entirely appropriate reason.

A slightly more sophisticated way is to have the game use sensible defaults if the value can't be parsed. For this new fireball type, the default would be the standard explosion fireball. For a number, the default would be 0. For a ship, the default could be the first ship in the mission. But again, this solution would be extra work to implement when there shouldn't be any reason to need a default in the first place. And the new sexp using this default argument would almost certainly be wrong from a gameplay standpoint.

The best reason, in my opinion, is the practical one. Adding some sort of general-purpose error recovery routine here is of marginal utility for the work required.

[MageKing17]

I consider "the mission won't run as intended" to be an entirely appropriate reason.

In that case, why is referencing a nonexistent AI profile a Warning (not even a Warning, a WarningEx, which even debug builds won't show unless you use a specific command-line option to make it show up) instead of an Error? After all, not using the specified AI profile will definitely make the mission not run as intended.

You can use the exact same argument for literally every warning in missionparse.cpp, so no, it's not an appropriate reason in-and-of-itself.

[Goober5000]

Fine, then strike that part and refer to the other reasons in my comment.

[Goober5000]

#2003 merged which makes the error recovery friendlier. A proper fix, however, is for the Blue Planet team to use a fireball that is in range.