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

CINE: Add support for Operation Stealth. #2365

Merged
merged 3 commits into from
Jul 24, 2020
Merged

Conversation

karisal
Copy link
Contributor

@karisal karisal commented Jul 12, 2020

Hello!
Here's support for Operation Stealth PC 16 and 256 color versions with AdLib and Roland MT-32 sound.
The support is based on disassembly of PC versions of Future Wars and Operation Stealth using Interactive Disassembler.
Unfortunately the support is just a single commit because I simply worked on it locally until it worked. I'm just now starting to learn using Git (Previously I used SVN) but already it looks like it's a lot more powerful than SVN.

I think with this Operation Stealth is ready for testing, at least the PC versions. Amiga versions may work (Not sure about their audio side). Atari ST versions are completely untested.

Kari Salminen

Add support for Operation Stealth PC 16 and 256 color versions with
AdLib and Roland MT-32 sound. Add support for 20 extended savegames
(Thumbnails, playtime etc) for both Future Wars and Operation Stealth
(20 because it fits on screen using the original save/load interface).

Details:
 - Add versioning to Future Wars and Operation Stealth savegames.
 - Add fade in effect to both Future Wars and Operation Stealth.
 - Add mouse wheel support and keyboard support to moving in menus.
 - Map middle mouse button to pressing both left and right buttons.
 - Make interface more responsive (See manageEvents() and drawFrame()).
 - Amiga versions should be completable but sound may or may not work.
 - Atari ST versions completely untested.

Game options currently supported:
 - Using original save/load interface
 - Using transparent dialog boxes in 16 color scenes (Also for PC)

Console commands currently supported:
 - labyrinthCheat (For cheating in Operation Stealth's labyrinths)
 - disableLabyrinthCheat (Disabling labyrinth cheat)
 - disableHacks (Disabling hacks, useful for testing)
 - enableHacks (Enabling hacks, useful for testing. On by default)
@ghost
Copy link

ghost commented Jul 12, 2020

Congratulations 🎉. DeepCode analyzed your code in 4.809 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

Change structure initialization from C++11 style to older style to fix
clang compilation. Add null check to fix DeepCode warning about
possible undefined behaviour.
@karisal
Copy link
Contributor Author

karisal commented Jul 13, 2020

Now I'm just waiting for somebody (e.g. @sev- or anybody) to review or merge this. Thank you very much!

Kari Salminen

Copy link
Member

@sev- sev- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incredible comeback, Kari! Welcome!

There are a few minor issues I spotted and it will be ready for the merge.

Will you hang around for announcing support and fixing any Coverity (static analysis tool) issues if any?

g_cine->getPlatform() == Common::kPlatformDOS &&
g_sound->musicType() != MT_MT32 &&
(strstr(resourceName, ".SPL") || strstr(resourceName, ".H32")))
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code formatting is off. The parentheses must stick to the previous line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken into account.


for (int i = 0; i < ARRAYSIZE(resNameMapping); i++) {
if (scumm_stricmp(base, resNameMapping[i].from) == 0) {
strncpy(base, resNameMapping[i].to, sizeof(base));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect. Either use sizeof(base) - 1 or better use Common::strlcpy()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken into account (strlcpy).

}
}

const char* ext = (g_sound->musicType() == MT_ADLIB) ? ".ADL" : ".HP";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect code formatting. char *ext

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken into account.

char base[20];
removeExtention(base, resourceName);

for (int i = 0; i < ARRAYSIZE(resNameMapping); i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be uint

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken into account.

delete out;

// Delete save file
char saveFileName[256];
sprintf(saveFileName, "%s.%1d", target, slot);
const char * saveFileName = getSavegameFile(slot, target);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const char *saveFileName

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken into account.

@@ -166,24 +175,39 @@ static void processEvent(Common::Event &event) {
case Common::KEYCODE_KP3:
moveUsingKeyboard(+1, -1); // Down & Right
break;
case Common::KEYCODE_LEFT:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is too much to ask, but if you could take a look into the new keymapper code (see Wintermute engine), that would be awesome.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is too much to ask at this point. So, not taken into account.


const Color firstColor = _colors[firstIndex];

for (int i = firstIndex; i < lastIndex; i++)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be uint

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken into account.

Fix code formatting, string copying (strncpy -> strlcpy) and signedness (int -> uint).
@karisal
Copy link
Contributor Author

karisal commented Jul 19, 2020

Incredible comeback, Kari! Welcome!

Thanks!

There are a few minor issues I spotted and it will be ready for the merge.

I submitted a commit fixing all but the keymapper code.

Will you hang around for announcing support and fixing any Coverity (static analysis tool) issues if any?

I can fix the Coverity issues if any come up recently after merge.
I can be around when support is announced (I don't need to be the
one who announces support, it can be someone else). I can perhaps
(Time permitting) be around for the bugfixing/testing period of the
first release to include this code but after that I will be gone i.e. not
available for any work related to ScummVM.

Kari Salminen

@sev-
Copy link
Member

sev- commented Jul 24, 2020

Great! I am merging this and will announce the support in couple of days.

@sev- sev- merged commit 580a7e6 into scummvm:master Jul 24, 2020
@sev-
Copy link
Member

sev- commented Aug 8, 2020

@karisal Could you please make 3-4 interesting screenshots which I could put to the announcement on Facebook? One could be the game title.

@karisal
Copy link
Contributor Author

karisal commented Aug 10, 2020 via email

@karisal
Copy link
Contributor Author

karisal commented Aug 18, 2020

@karisal Could you please make 3-4 interesting screenshots which I could put to the announcement on Facebook? One could be the game title.

Voila! If you (@sev- ) didn't already make an announcement here are some screenshots (OptiPNG + AdvPNG processed):

scummvm-os-screenshots.zip

karisal added a commit to karisal/scummvm that referenced this pull request Sep 9, 2020
In Future Wars the command line was not always updated and thus failed
sometimes to be up to date (i.e. showing wrong text, e.g. "EXAMINE" only
when it should have read "EXAMINE scaffolding" because the mouse cursor
was on the scaffolding).

Now we just always update the command line for both Future Wars and
Operation Stealth which seems to fix the command line updating.

I think this probably was a regression caused by adding support for
Operation Stealth (i.e. pull request scummvm#2365) and the efforts made in it
to make the user interface responsive.
karisal added a commit to karisal/scummvm that referenced this pull request Sep 9, 2020
In Future Wars the command line was not always updated and thus failed
sometimes to be up to date (i.e. showing wrong text, e.g. "EXAMINE" only
when it should have read "EXAMINE scaffolding" because the mouse cursor
was on the scaffolding).

Now we just always update the command line for both Future Wars and
Operation Stealth which seems to fix the command line updating.

I think this probably was a regression caused by adding support for
Operation Stealth (i.e. pull request scummvm#2365) and the efforts made in it
to make the user interface responsive.
criezy pushed a commit that referenced this pull request Sep 9, 2020
In Future Wars the command line was not always updated and thus failed
sometimes to be up to date (i.e. showing wrong text, e.g. "EXAMINE" only
when it should have read "EXAMINE scaffolding" because the mouse cursor
was on the scaffolding).

Now we just always update the command line for both Future Wars and
Operation Stealth which seems to fix the command line updating.

I think this probably was a regression caused by adding support for
Operation Stealth (i.e. pull request #2365) and the efforts made in it
to make the user interface responsive.
criezy pushed a commit that referenced this pull request Sep 9, 2020
In Future Wars the command line was not always updated and thus failed
sometimes to be up to date (i.e. showing wrong text, e.g. "EXAMINE" only
when it should have read "EXAMINE scaffolding" because the mouse cursor
was on the scaffolding).

Now we just always update the command line for both Future Wars and
Operation Stealth which seems to fix the command line updating.

I think this probably was a regression caused by adding support for
Operation Stealth (i.e. pull request #2365) and the efforts made in it
to make the user interface responsive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants