Skip to content
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

SUPERNOVA: Add support for Mission Supernova 2 #1744

Merged
merged 160 commits into from Jul 28, 2019

Conversation

@vyzigold
Copy link
Contributor

commented Jul 11, 2019

This pull request adds support for Mission Supernova 2 to supernova engine as described here: https://wiki.scummvm.org/index.php?title=Summer_of_Code/GSoC_Ideas_2019#Mission_Supernova_games

There is SUPERNOVA2 as a subsystem name in the first half of commits. That is, because I implemented support for MS2 as a separate engine at first and then I merged the two engines.

vyzigold added some commits May 27, 2019

SUPERNOVA2: add tool to generate engine data file
Most of the tool is copied from create supernova
Add correct gametext.h and strings_en.po for supernova2
Doesn't handle images yet
SUPERNOVA2: Begin intro animation
Copy and modify all the code needed for animation
from supernova engine and display Mission Supernova
logo.
SUPERNOVA2: Synchronize with main repository
Better support for pbm format was added to
create_supernova in main repository, this
commit just mirrors theese changes
SUPERNOVA2: Add drawMapExits from supernova
I am keeping there the TODO from the original code,
which can be resolved pretty easily, but I want to
leave the code as similar to supernova engine as possible
so it could eventualy be merged together.
@criezy

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

For those who want to test the games, they are available for free on the original developer website: http://outpost.simplicity.de/
Both parts can now be played in ScummVM and should be completable.
There is also a partial walkthrough (for most of the first part) on the wiki: https://wiki.scummvm.org/index.php?title=Mission_Supernova/walkthrough

Also since I am not sure what is best, I will ask the question here and see what others think: @vyzigold has added an optional improved mode in ScummVM to remove some of the tedious repetitions in the game (such as doing the same sequence of actions again and again each time we have to pass through the airlock room in the first game), adding shortcuts (such as using the number keys to select verbs), and a few other improvements. Currently this is implemented as a separate detection entry, but I am wondering if it would be better to make it an engine option instead.

@lotharsm

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

@vyzigold: Thanks for your contribution, looking forward to it!

Also since I am not sure what is best, I will ask the question here and see what others think: @vyzigold has added an optional improved mode in ScummVM to remove some of the tedious repetitions in the game (such as doing the same sequence of actions again and again each time we have to pass through the airlock room in the first game), adding shortcuts (such as using the number keys to select verbs), and a few other improvements. Currently this is implemented as a separate detection entry, but I am wondering if it would be better to make it an engine option instead.

I think making it an engine option would be a better integration than separate detection entries.

@bluegr
Copy link
Member

left a comment

Overall, nice work!
The commits should be amended so that they refer to SUPERNOVA2

static const PlainGameDescriptor supernovaGames[] = {
{"msn1", "Mission Supernova 1"},
{"msn2", "Mission Supernova 2"},
{"msn1-i", "Mission Supernova 1 improved"},
{"msn2-i", "Mission Supernova 2 improved"},

This comment has been minimized.

Copy link
@bluegr

bluegr Jul 14, 2019

Member

As mentioned, these should be game options, not separate game entries

if (!strncmp(target, "msn1", 4))
pattern = Common::String::format("msn_save.###");
if (!strncmp(target, "msn2", 4))
pattern = Common::String::format("ms2_save.###");

This comment has been minimized.

Copy link
@bluegr

bluegr Jul 14, 2019

Member

Why are you changing the target name here?

This comment has been minimized.

Copy link
@vyzigold

vyzigold Jul 15, 2019

Author Contributor

I don't change the target there, I just use a different save game pattern for each game.

This comment has been minimized.

Copy link
@criezy

criezy Jul 15, 2019

Member

I assume the question is not so much in this specific place, as it has to be consistent with what is done in loadGame and saveGame, but rather why in this engine we don't use the target name for the save file names, as is usually done in other engines.

This is not really specific to the support of the second part, as we already have this behaviour for the first part in the engine in master. From what I remember this was done to share the same set of saves for all the languages (currently German and English) since they do not depend on the languages. This way we can easily start to play in German and continue in English, without having to rename save files.

@@ -148,7 +190,8 @@ SaveStateList SupernovaMetaEngine::listSaves(const char *target) const {
Common::InSaveFile *savefile = g_system->getSavefileManager()->openForLoading(*file);
if (savefile) {
uint saveHeader = savefile->readUint32LE();
if (saveHeader == SAVEGAME_HEADER) {
if ((saveHeader == SAVEGAME_HEADER && !strncmp(target, "msn1", 4)) ||
(saveHeader == SAVEGAME_HEADER2 && !strncmp(target, "msn2", 4))) {

This comment has been minimized.

Copy link
@bluegr

bluegr Jul 14, 2019

Member

There is no need to check for a target name here, since you will only get the saved games for the game that is currently running

This comment has been minimized.

Copy link
@vyzigold

vyzigold Jul 15, 2019

Author Contributor

I wanted to match the game with the correct SAVEGAME_HEADER. So for example if the user takes a save from the first game and renames it to match the ms2_save.### pattern, thanks to this it shouldn't get listed when listing saves for MS2.

This comment has been minimized.

Copy link
@criezy

criezy Jul 15, 2019

Member

@bluegr This is in the MetaEngine here, not in the Engine. This is for example used to list the save games in the launcher. So there may not be a game currently running.

Doing some sanity checks on the header to catch cases where the user manually renames a save file, or it gets corrupted, doesn't sound a bad idea to me.

g_system->updateScreen();
g_system->delayMillis(_vm->_delay);
}
}

This comment has been minimized.

Copy link
@bluegr

bluegr Jul 14, 2019

Member

These three methods could be merged into one. You could add a parameter that would filter the expected event types, e.g. :

int GameManager::getInput(bool checkMouseEvents, bool checkKeyboardEvents) {
    do {
        updateEvents();
        g_system->updateScreen();
        g_system->delayMillis(_vm->_delay);
    } while (!_vm->shouldQuit() && !(_mouseClicked && checkMouseEvents) && !(_keyPressed && checkKeyboardEvents));
}
updateEvents();
g_system->updateScreen();
} while (_time < end && !_vm->shouldQuit() && !_keyPressed && !_mouseClicked);
}

This comment has been minimized.

Copy link
@bluegr

bluegr Jul 14, 2019

Member

The input check could be a separate input parameter flag, so these two could be merged

_MSPart = 2;
else if (ConfMan.get("gameid") == "msn1-i") {
_MSPart = 1;
_improved = true;

This comment has been minimized.

Copy link
@bluegr

bluegr Jul 14, 2019

Member

Again, these entries should be toggled by game options, not game IDs

@vyzigold

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

I made the improved mode an engine option instead of separate detection entry and I also merged the methods pointed out by @bluegr.

If I change the commit messages, should they really say SUPERNOVA2? Because it's the SUPERNOVA engine that got changed.

SUPERNOVA: Fix most of Codacy issues
* Fix mismatched delete in create_image.cpp
* Add shebangs to create_ms2_data.***.sh
* Reduce scopes of some variables
* Substitute bitwise and for logical and in
    GamaManager2::passageConstruction
@criezy

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

If there is no objection, I would like to merge this PR this weekend.
If somebody would like more time to review the changes, please let me know.

@criezy

This comment has been minimized.

Copy link
Member

commented Jul 28, 2019

OK. Time to merge this.

@criezy criezy merged commit fc3ae4c into scummvm:master Jul 28, 2019

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.