Improves FSO's compatibility with the FS2 Demo data #1156

Merged
merged 7 commits into from Jan 28, 2017

Conversation

Projects
None yet
3 participants
@MageKing17
Member

MageKing17 commented Jan 24, 2017

These commits prevent FSO from crashing when running with the data from the FS2 Demo. Some of these bugs could theoretically be encountered outside the demo, although it's unlikely. This PR makes no attempt to stop demo data from generating warnings; only errors or assertions.

Only commit I'm really iffy on is 559b07d, but I never liked the hi-res interface art check anyway.

MageKing17 added some commits Jan 20, 2017

Make "+Music:" optional in mainhalls.
It defaults to an empty string (resulting in no music) anyway, so there's no harm in it being missing, and the demo's mainhall.tbl doesn't contain these lines, so this change makes it possible to reach the mainhall with demo data.
Don't try to do anything with a nonexistent Fade_anim.
If Fade_anim.first_frame was -1 (like, for instance, if the animation did not exist, which it doesn't in the demo data), the briefing code would still try to do stuff with it, and then unload it when it closed. This seems like a bad idea in general.
Make bm_unload() check for a handle of -1.
Somewhat related to the previous commit: if, in the future, we have another instance of trying to unload a nonexistent graphic, it would make much more sense if BMPMAN returned -1 (like it already does for other failure conditions, like trying to unload an unused slot) rather than tripping the "n >= 0" assertion down below (calling with -2 or other negative values that can still trip that assertion unaccounted for, because those would definitely be mistakes in the code).
Remove unhelpful assertions from comp_training_lines_by_born_on_date().
If an event with a directive contains both an is-destroyed/is-destroyed-delay for a wing with multiple waves AND the event's Sexp_useful_number is 0 (either the event contains an "is-event-*" SEXP (or one of the "-delay" ones with the optional third argument not set), and that SEXP didn't return true, or some part of the event evaluated to SEXP_CANT_EVAL) AND the wing currently has 0 ships in the mission (so Directive_count gets set to DIRECTIVE_WING_ZERO to mark the directive as being "temporarily" true, until the next wave arrives), then the directive winds up in the list of displayed directives without actually having a born_on_date set, so it fails this assertion. Since this is caused by the data supplied (in the mission) rather than a coding mistake, and since this assertion doesn't actually seem to be guarding against anything (because nothing bad happens if the comparison happens anyway, just with a born_on_date of 0), and it behaves just fine in Release builds where the assertion isn't checked, the simplest solution would seem to be to just remove the assertions altogether.

Found this issue while testing with demo data, so blame whatever Canadian wrote the first demo mission to make the "Destroy Libra" event check for either Libra being destroyed OR an event (that can't ever become true until after at least one wave of Libra gets destroyed) becoming true.
Change check for 1024x768 interface art to not check 2_TechShipData-m.
The demo doesn't include techroom art because the demo doesn't have a techroom (or a campaign room, or a HUD config menu), but it still otherwise works with the higher-resolution interface.
Fix bitmap release ordering mistake in medals.cpp.
Initialization of the medal window locks the mask twice (once directly and once indirectly, through set_mask_bmap()), then closing the medal window tries to release it after only unlocking it once. bm_release() will refuse to do anything if the BMPMAN entry is still locked, though, so the first release therefore does nothing, and since it will then only be released once (indirectly, by Medals_window.destroy()) after being loaded twice, the mask stays loaded in memory until FSO exits. By moving the release until after Medals_window is destroyed, the mask should be successfully released when the medals window closes.
// if we don't have it then fall back to 640x480 mode instead
- if ( !has_sparky_hi ) {
+ if ( !cf_exists_full("2_ChoosePilot-m.pcx", CF_TYPE_ANY)) {

This comment has been minimized.

@asarium

asarium Jan 24, 2017

Member

The reason that both files were checked could be that the files were distributed in separate VPs (I'm not sure how the High-Res files were distributed for the retail version). If that's the case then this check could pass even though there are files missing for the high resolution interface.

@asarium

asarium Jan 24, 2017

Member

The reason that both files were checked could be that the files were distributed in separate VPs (I'm not sure how the High-Res files were distributed for the retail version). If that's the case then this check could pass even though there are files missing for the high resolution interface.

This comment has been minimized.

@MageKing17

MageKing17 Jan 24, 2017

Member

Of course they weren't distributed in separate VPs; if they were, then whichever one wasn't in sparky_hi_fs2.vp should've been removed from the check ages ago. And of course the check can pass even if there are files missing for the high resolution interface; that's true of either version. All you have to do is just stick two files called 2_ChoosePilot-m.pcx and 2_TechShipData-m.pcx into an installation that's missing sparky_hi_fs2.vp and poof, FSO will try to use nonexistant high-res interface art.

Fixing that problem is definitely outside the scope of this PR, although I would not mind having each individual screen use either the 640x480 or 1024x768 version of its own interface art depending on whichever was available, rather than it being determined globally.

@MageKing17

MageKing17 Jan 24, 2017

Member

Of course they weren't distributed in separate VPs; if they were, then whichever one wasn't in sparky_hi_fs2.vp should've been removed from the check ages ago. And of course the check can pass even if there are files missing for the high resolution interface; that's true of either version. All you have to do is just stick two files called 2_ChoosePilot-m.pcx and 2_TechShipData-m.pcx into an installation that's missing sparky_hi_fs2.vp and poof, FSO will try to use nonexistant high-res interface art.

Fixing that problem is definitely outside the scope of this PR, although I would not mind having each individual screen use either the 640x480 or 1024x768 version of its own interface art depending on whichever was available, rather than it being determined globally.

This comment has been minimized.

@asarium

asarium Jan 24, 2017

Member

Yeah, I just checked and both files are in the same VP. I have no idea why the code checked for both files but this change should not cause any issues.

@asarium

asarium Jan 24, 2017

Member

Yeah, I just checked and both files are in the same VP. I have no idea why the code checked for both files but this change should not cause any issues.

This comment has been minimized.

@Goober5000

Goober5000 Jan 28, 2017

Contributor

The only reason that I can think of for including both checks is if a mod included custom pilot selection screen interface art. FSPort does this, for instance. However, even FSPort only changes the visible art, not the mask. So I too think this commit should be safe.

@Goober5000

Goober5000 Jan 28, 2017

Contributor

The only reason that I can think of for including both checks is if a mod included custom pilot selection screen interface art. FSPort does this, for instance. However, even FSPort only changes the visible art, not the mask. So I too think this commit should be safe.

This comment has been minimized.

@MageKing17

MageKing17 Jan 28, 2017

Member

Even if a mod wanted to include a custom mask, I'd be surprised if PCX were the format of choice.

@MageKing17

MageKing17 Jan 28, 2017

Member

Even if a mod wanted to include a custom mask, I'd be surprised if PCX were the format of choice.

This comment has been minimized.

@Goober5000

Goober5000 Jan 28, 2017

Contributor

Mask files need to use PCX because the button indexes correspond to indexes in the PCX palette. Visible files don't have this requirement so they can use any format.

@Goober5000

Goober5000 Jan 28, 2017

Contributor

Mask files need to use PCX because the button indexes correspond to indexes in the PCX palette. Visible files don't have this requirement so they can use any format.

This comment has been minimized.

@MageKing17

MageKing17 Jan 28, 2017

Member

PCX files aren't the only files with an explicit palette; in 15 years, nobody made the mask code accept PNGs, at least?

@MageKing17

MageKing17 Jan 28, 2017

Member

PCX files aren't the only files with an explicit palette; in 15 years, nobody made the mask code accept PNGs, at least?

This comment has been minimized.

@Goober5000

Goober5000 Jan 28, 2017

Contributor

That's a good question; I don't actually know. In all the mods I've looked at that modify the mask file, PCX is the only format I've seen them use. And only 8-bit PCX, at that.

@Goober5000

Goober5000 Jan 28, 2017

Contributor

That's a good question; I don't actually know. In all the mods I've looked at that modify the mask file, PCX is the only format I've seen them use. And only 8-bit PCX, at that.

This comment has been minimized.

@MageKing17

MageKing17 Jan 28, 2017

Member

Well, maybe I'll take a look at it in the future if I decide to try tackling graphics-related code from a different direction (and we should probably stop having this conversation on a closed PR only tangentially related :P ).

@MageKing17

MageKing17 Jan 28, 2017

Member

Well, maybe I'll take a look at it in the future if I decide to try tackling graphics-related code from a different direction (and we should probably stop having this conversation on a closed PR only tangentially related :P ).

This comment has been minimized.

@Goober5000

Goober5000 Jan 28, 2017

Contributor

Heh. :p

@Goober5000

Goober5000 Jan 28, 2017

Contributor

Heh. :p

code/popup/popup.h
@@ -107,4 +107,7 @@ void popup_kill_any_active();
// change the text inside of the popup
void popup_change_text(const char *new_text);
+// Used if certain data is missing (e.g. running demo data).
+void game_feature_not_in_demo_popup();

This comment has been minimized.

@asarium

asarium Jan 24, 2017

Member

Since this is defined in popup.h/.cpp it should use the popup prefix.

@asarium

asarium Jan 24, 2017

Member

Since this is defined in popup.h/.cpp it should use the popup prefix.

This comment has been minimized.

@MageKing17

MageKing17 Jan 24, 2017

Member

I suppose that makes sense.

@MageKing17

MageKing17 Jan 24, 2017

Member

I suppose that makes sense.

Avoid crashing if certain interface artwork is missing.
Changes a few windows (tech room, campaign room, medal case, and HUD config menu) to issue Warnings instead of Errors if certain pieces of interface art are missing. If their mask bitmap is missing, they automatically close themselves and display the old "Sorry, this feature is available only in the retail version" popup from the demo, allowing FSO to run the demo data without crashing when you click on the wrong button.

It also restores the special demo popup for the campaign room, which allows you to at least restart the demo campaign without deleting and recreating your pilot.
// if we don't have it then fall back to 640x480 mode instead
- if ( !has_sparky_hi ) {
+ if ( !cf_exists_full("2_ChoosePilot-m.pcx", CF_TYPE_ANY)) {

This comment has been minimized.

@asarium

asarium Jan 24, 2017

Member

Yeah, I just checked and both files are in the same VP. I have no idea why the code checked for both files but this change should not cause any issues.

@asarium

asarium Jan 24, 2017

Member

Yeah, I just checked and both files are in the same VP. I have no idea why the code checked for both files but this change should not cause any issues.

@asarium asarium merged commit 39649e9 into scp-fs2open:master Jan 28, 2017

2 checks passed

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

@MageKing17 MageKing17 deleted the MageKing17:cleanup/demo-compatibility branch Jan 28, 2017

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