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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

SWORD1: Implement palette fades #5293

Merged
merged 12 commits into from Sep 2, 2023
Merged

SWORD1: Implement palette fades #5293

merged 12 commits into from Sep 2, 2023

Conversation

AndywinXp
Copy link
Contributor

Ahoy! This is the first one of a series of changes and PRs which aim at finishing and implementing all those things from the original Broken Sword 1 executable which we are currently missing in our implementation. We have a great port of the engine and it would be a shame not to finish those last tasks; after all the devil is in the details 馃檪

A full list can be seen here: https://wiki.scummvm.org/index.php?title=Sword1

What am I looking at here?

In this first part I have implemented accurate palette fade-ins and fade-outs. In order to achieve this, one of the necessities was to fix the game timers, which was part of the to-do plan anyway.
Also, I implemented fast and slow modes from the original debug commands because at one point I was going insane debugging this 馃檪

All of this was implemented from the original code, using the DOS implementation as the reference (hence the usage of some kind of emulation of an vertical blank interrupt). Of course this is not the DOS code verbatim:

  • Since we apparently don't have files for the WIN95 drivers but only for the DOS ones, I manually rewrote the palette fading routine from x86 asm to C++;
  • The original DOS version uses the vblank timer for the top and bottom menus animations, palette fades and in-game frame limiter. In this implementation, the latter is not using the vblank simulation but instead uses our current timer system (with the correct frame time though). This is because our current system is more precise and smooth than an emulated vblank interrupt system (I've reluctantly tried and I'm glad that it failed; I don't want a threaded game timer 馃檪).
  • The code for fades in the main menu seems to work like it should, but it's adapted for our control panel implementation which is quite different from the original. Said implementation will be changed to match the source code in one of the next PRs for this project (because of the big amount of differences noted in the wiki link above). Needless to say, this future reimplementation will take into account all past contributions and changes (by our past and current devs) which don't have a match on the source code in our possession, for e.g. Mac and PSX support, etc.

Let me know how is it. The moment this is merged, work will be resumed on all the other tasks mentioned on the wiki. Alternatively, if you prefer a bigger PR with more stuff poured into it, let me know.

Also begin work on palette and sound fades
They can only be activated via the original debug keys ('1' and '4'),
and only if ScummVM is launched in debug mode.
Had to reshuffle some stuff to make it work properly,
and in order to set up everything for future changes.
@bluegr
Copy link
Member

bluegr commented Aug 27, 2023

Nice work!

It would be better to keep PRs as targeted as possible, so it would be preferable to create separate PRs for the other major TODOs

@AndywinXp
Copy link
Contributor Author

AndywinXp commented Aug 27, 2023

Thanks! I'll do as suggested then 馃檪 (within reason of course, some of those features are too much interconnected)

@AndywinXp AndywinXp requested a review from criezy August 27, 2023 15:58
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.

Good stuff! Have some remarks/questions

engines/sword1/mouse.cpp Show resolved Hide resolved
engines/sword1/screen.cpp Outdated Show resolved Hide resolved
if (_paletteFadeInfo.paletteCount == 0) {
_paletteFadeInfo.paletteStatus = NO_FADE;
} else {
if (_paletteFadeInfo.paletteStatus == FADE_DOWN) {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps, I am too picky, but I'd write something like:

for (int i = 0; i < 256 * 3; i++) {
   if (_paletteFadeInfo.srcPalette[i] > 0 && (int8)_paletteFadeInfo.srcPalette[i] < (int8)_paletteFadeInfo.dstPalette[i])
    _paletteFadeInfo.srcPalette[i] += _paletteFadeInfo.paletteStatus;

Since FADE_UP = 1 and FADE_DOWN = -1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your way of writing this is certainly more concise and shorter; unfortunately I have tested it and it seems to break fades 馃檨 It appears the two conditions can't be used together.

If you are okay with it I'd leave the verbose code on

Copy link
Member

Choose a reason for hiding this comment

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

This indeed can't work as when fading up from black, srcPalette will be 0 and fading will never happen.
Maybe CLIP could be used but it seems safer to keep it the way it's written here as there could be some wrapping (if we fade from white to black for example).

engines/sword1/screen.h Outdated Show resolved Hide resolved
engines/sword1/sword1.cpp Show resolved Hide resolved
engines/sword1/sword1.cpp Outdated Show resolved Hide resolved
Copy link
Member

@criezy criezy left a comment

Choose a reason for hiding this comment

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

The Mac version of the game is known to use color 255 instead of 0 in some places, for example for the transparent color in parallax, or the background color visible in some screens, such as when opening the door in Nico's apartment.

And that recollection prompted me to test those places and find out that this pull request is actually breaking that and introducing a regression. When opening the door the color is now red instead of black:
image

By the way, while it seems to work I think the current code for the Mac version is probably not fully correct. I should take a look at the disassembly to check exactly what the original does.

engines/sword1/screen.cpp Outdated Show resolved Hide resolved
engines/sword1/control.cpp Show resolved Hide resolved
engines/sword1/control.cpp Show resolved Hide resolved
engines/sword1/screen.cpp Outdated Show resolved Hide resolved
engines/sword1/screen.cpp Show resolved Hide resolved
@AndywinXp
Copy link
Contributor Author

Hi @criezy, thanks for the review! About the Mac concerns: tonight I can definitely help and pull up a disassembly as well to try and see what is actually supposed to happen in the cases the screen has to fade from and to black.

@criezy
Copy link
Member

criezy commented Aug 30, 2023

The other place I had in mind where I had the issue of color 255 being used for black and appearing red without the mac-specific hack is for the Pillars in the St Ninian's Church. So I tested that and it seemed to highlight a different issue with the pull request: the whole animated cut-scene is considerably darker as if the palette was partially faded, to the point where the text is difficult to read. I don't remember the original being like that, so this seems more like a regression than a feature that was missing in ScummVM.

ScummVM 2.7.1 Your pull request
image image
image image

And I also noticed another issue, but this one is not introduced in this pull request, and also happens with ScummVM 2.7.1. This is a different issue than the one I had in mind and was fixed where red appeared on the pillars themselves, intertwined with the blue. But this might be another mac-specific issue that escaped my attention before. Or does the PC version also have those red bars on the sides?
image

@AndywinXp
Copy link
Contributor Author

Thanks for noting these issues! May I please ask for some savegames for those final sections?

@criezy
Copy link
Member

criezy commented Aug 30, 2023

Savegame at the St Ninian's church: sword1.023.zip

@criezy
Copy link
Member

criezy commented Aug 30, 2023

But this might be another mac-specific issue that escaped my attention before. Or does the PC version also have those red bars on the sides?

To answer my own question, the version from GOG has black bars there. So this appears to be a mac-specific issue (I must say I am not really surprised).

@AndywinXp
Copy link
Contributor Author

AndywinXp commented Aug 30, 2023

This was a very nice catch, thanks! And this is one of those times in which I'm grateful to have the original scripts at hand...

What is basically happening is that this ending scene is not a Smacker video but is instead a concatenation of (as they call them) "screens" functioning as camera angles. Normally when a screen is changed, a palette fade happens. But in this case, right before the sequence begins...

FN_set_palette_to_cut();	// all 'cuts' during this sequence, rather than fades

And it works! Kind of! There are no fades but the screen is dim as soon as a screen/palette change happens. Why? Because I forgot to left-shift the palette when a fade-cut was signaled to happen 馃檪

@AndywinXp
Copy link
Contributor Author

To answer my own question, the version from GOG has black bars there. So this appears to be a mac-specific issue (I must say I am not really surprised).

Do we have any video of someone playing this version on original hardware? I bet this could shed some light about whatever should happen in this case.

@criezy
Copy link
Member

criezy commented Aug 30, 2023

Do we have any video of someone playing this version on original hardware? I bet this could shed some light about whatever should happen in this case.

Not that I know. But I have an old MacOS 9 laptop that still works so I can give it a go. I will probably have to play from the start as I doubt I still have savegames for the original. Not that I mind, but it will probably take a little while to reach that point which is unfortunately at the end of the game.

@criezy
Copy link
Member

criezy commented Aug 30, 2023

Because I forgot to left-shift the palette when a fade-cut was signaled to happen

Ah indeed. And that reminds of another "issue" that I forgot to mention because it is actually harmless. The colors that you have added to the Screen class (_white, _red, _black, ...) are already shifted (so using 8 bits). There is however one place where one of those colors is used as a non-shifted (6-bits) color: in Screen::fnSetFadeTargetPalette. This does not matter though as that color is _black, and shifted or not shifted a 0 is still a 0.

@AndywinXp
Copy link
Contributor Author

AndywinXp commented Aug 30, 2023

Do we have any video of someone playing this version on original hardware? I bet this could shed some light about whatever should happen in this case.

Not that I know. But I have an old MacOS 9 laptop that still works so I can give it a go. I will probably have to play from the start as I doubt I still have savegames for the original. Not that I mind, but it will probably take a little while to reach that point which is unfortunately at the end of the game.

Hopefully this can help, launch the game from command line like this:

sword rev <boot-param>

where can be chosen from these ones . The main menu should appear if you already have savegames, in that case, just press restart. They do work on the dos executable.

Edit, boot-param 73 seems to be matching your savegame!

Edit 2: Darn, nevermind... I'm seeing from the disassembly that the only command line options are debug, tear, nofade and nosound...

Copy link
Member

@criezy criezy left a comment

Choose a reason for hiding this comment

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

Thank you. This looks good to me now.

The remaining issue with the mac version is not a regression from this pull request and already occurs with the current code from master. So there is no reason to block this pull request because of it.

@AndywinXp
Copy link
Contributor Author

Thank you for the thorough review, I really appreciate it! 馃檹

@sev-
Copy link
Member

sev- commented Sep 2, 2023

Thank you!

@sev- sev- merged commit 461009e into scummvm:master Sep 2, 2023
8 checks passed
@AndywinXp AndywinXp deleted the sword1fades branch September 2, 2023 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants