Opened 15 years ago

Closed 13 years ago

Last modified 12 months ago

#1790 closed defect (fixed)

SCUMM/SMUSH: Ugly palette change when video finishes

Reported by: fingolfin Owned by: cyxx
Priority: normal Component: Engine: SCUMM
Keywords: Cc:
Game: The Dig

Description

Issue occurs in latest CVS and with 0.6.x, but not with 0.5.x.

When a SMUSH video finishes, the palette is changed, from the
video palette to the regular in-game palette. The problem is that a
redraw occurs at the wrong time (i.e. when fadeIn is called) so
some graphics are displayed using the wrong palette, resulting in a
very ugly "flash".

A good way to reproduce this is the end of the intro video in The
Dig. Or also in the Dig, the video you get when using the "elevator"
going down.

Ticket imported from: #1035739. Ticket imported from: bugs/1790.

Attachments (7)

dig.s34 (244.5 KB ) - added by fingolfin 15 years ago.
In front of alien elevator
indy3.png (50.8 KB ) - added by Kirben 15 years ago.
indy3 example
loom.png (12.2 KB ) - added by Kirben 15 years ago.
Loom example
1035739.diff (2.8 KB ) - added by cyxx 13 years ago.
Possible fix for TheDig (SVN Jun 2nd, 2006)
1035739-2.diff (4.1 KB ) - added by cyxx 13 years ago.
Possible fix for TheDig and FT (v2)
tentacle.s03 (26.9 KB ) - added by Kirben 13 years ago.
Wash the carriage to trigger storm
var_fade_delay.diff (1.8 KB ) - added by cyxx 13 years ago.
VAR_FADE_DELAY patch

Download all attachments as: .zip

Change History (36)

comment:1 by fingolfin, 15 years ago

The attached savegame nicely demonstrates the problem at the elevator:
just click on the stone slab to trigger the cutscene, and wait until its end.
In 0.5.0, we get a smooth (palette wise, at least) transition from movie
to game graphics, while with latest CVS (and 0.6.x) you get an ugly
palette change effect.

by fingolfin, 15 years ago

Attachment: dig.s34 added

In front of alien elevator

by Kirben, 15 years ago

Attachment: indy3.png added

indy3 example

comment:2 by Kirben, 15 years ago

Could this be the same palette issue causing glitches during
some fades in indy3/loom too ? see attached screenshots.

by Kirben, 15 years ago

Attachment: loom.png added

Loom example

comment:3 by eriktorbjorn, 15 years ago

Palette issues during this kind of transition effects are
pretty much unavoidable, aren't they? I mean, we're
displaying parts of two different rooms at once. If they use
different palettes one of them will look wrong. I remember
playing through EGA Loom with DOSbox to compare another
issue with the Rusty/dragon scene, and even with the
original interpreter Rusty the Grey briefly turns into Rusty
of Many Colours.

In The Dig case, though, my impression was that there wasn't
any transition effect at all. It just decided to change the
palette at the wrong moment.

comment:4 by Kirben, 15 years ago

Your right, I double checked and those glitches in indy3 and
loom occur in the original games too. I'm surprised they left
glitches like that in the games.

The palette glitches after smush playback are ScummVM
specific though, I compared palette glitches at start of Full
Throttle when skipping smush scenes and they don't occur in
the original game.

comment:5 by fingolfin, 15 years ago

It would be intersting to investigate..
* whether The Dig / COMI ever use "fade effects" anywhere
* if they do so, which ones do they use, and where?
* maybe we can analyze / RE their counterparts to fadeIn
and/or the SO_ROOM_FADE code

comment:6 by fingolfin, 15 years ago

I put a workaround for this into CVS (simply suppress fade
effects directly after a video played).

It would still be very nice to learn how the original engine
behaved in the elevator scene. It would also be nice to learn
about the camera position at the end of the cutscene video;
the camera "jumps" in ScummVM, maybe the original didn't
show this ugliness, but i dunno what it did instead...

comment:7 by fingolfin, 15 years ago

Another thing which is apparent here is that the original engine spaced
characterd in this scene wider than we do. I have no idea what might
control that, though.

comment:8 by fingolfin, 15 years ago

(ignore my last comment from 2004-10-09, it was meant for another
tracker item)

comment:9 by eriktorbjorn, 15 years ago

There are also some ugly palette glitches in Full Throttle
when two SMUSH movies are played after each other. E.g. at
the end of a "getting a new weapon" movie at the old mine road.

At least, that happened to me several times when using a
boot param to test the scene while trying to understand the
"invalid seek" bug.

comment:10 by sev-, 14 years ago

FT INSANE sequences behave exactly like in original. Same
palette glitches appear in original engine. It just loads
palette before SMUSH playback begins. Possible solution
would be to postpone palette change and do it at the same
time as first frame gets displayed.

comment:11 by fingolfin, 14 years ago

I haven't tested this in FT at all, actually, I am mostly talking about The
Dig (and as I stated, this used to work fine; I tried to track down what
changed by stepping through the code, but failed to find anything).

comment:12 by Kirben, 14 years ago

scumm/gfx.cpp revision 2.257, was when the palette glitch
when skipping the introduction of the Dig starts to occur.
But it just looks like the palette glitch was hidden in the past,
since the fade effect was not shown at all before that change.

comment:13 by fingolfin, 14 years ago

Summary: SMUSH: Ugly palette change when video finishesSCUMM/SMUSH: Ugly palette change when video finishes

comment:14 by sev-, 13 years ago

This doesn't happen for me in SVN, at least in the crypt.
Can anyone second that?

comment:15 by sev-, 13 years ago

Status: newpending

comment:16 by Kirben, 13 years ago

Status: pendingnew

comment:17 by Kirben, 13 years ago

The palette glitches when skipping the introduction of Full
Throttle or The Dig still occur.

comment:18 by fingolfin, 13 years ago

Status: newpending

comment:19 by fingolfin, 13 years ago

I can still trivially reproduce the issue: Just start The Dig in ScummVM, and skip
over the intro video by pressing ESC. The fade triggered by this shows the
problem quite nicely.

comment:20 by Kirben, 13 years ago

Status: pendingnew

comment:21 by cyxx, 13 years ago

A possible patch to fix the glitches in TheDig.

The original interpreter has a special flag for that, fadeIn
effects are simply disabled after a SMUSH movie (good catch,
Max). As the doEffect flag can be reinitialised elsewhere in
the code, I think it's better to add a special flag for that
(and remove the 'hack' in SmushPlayer::release()).

Adding the extra flag doesn't fix all the problems (in the
first cutscene at least). We should also disable/ignore the
_screenEffectFlag. I suppose this was added to "guarantee"
the fadeOut and fadeIn calls are sequential (in case of
scripts errors), but if we check this flag, then the screen
doesn't always get cleared and this lead to the glitch we
observe in the first cutscene.

This looks like a big hack, but considering how the original
scripts are written, I think it's the easiest solution.

FT seems to handle things yet differently.

by cyxx, 13 years ago

Attachment: 1035739.diff added

Possible fix for TheDig (SVN Jun 2nd, 2006)

comment:22 by cyxx, 13 years ago

Updated patch which should fix some FT glitches.

Original v7/v8 interpreters do update the palette before
drawing the dirty blocks of the screen. This fixes the
remaining glitches of FT and allow me to cleanup a bit my
previous patch.

Also introduced VAR_FADE_DELAY in transitionEffect() (to
match the original interpreters, again).

I haven't tested this extensively, so I wouldn't be
surprised it introduces regressions.

by cyxx, 13 years ago

Attachment: 1035739-2.diff added

Possible fix for TheDig and FT (v2)

comment:23 by Kirben, 13 years ago

A few notes about VAR_FADE_DELAY:
VAR_FADE_DELAY is used as a timer for several other screen
effects in earlier games too.

As far as I know VAR_FADE_DELAY is only changed in script
10 of DOTT, which is used when storm is trigger in game. I
have attached a saved game.

by Kirben, 13 years ago

Attachment: tentacle.s03 added

Wash the carriage to trigger storm

comment:24 by sev-, 13 years ago

the patch looks ok to me and several tests show no problem.
However I thinks i's too late for 0.9.0. May be commit it to
trunk?

comment:25 by Kirben, 13 years ago

I added patch to main branch of ScummVM SVN.

VAR_FADE_DELAY should also be used as a timer in
scrollEffect() too. But I'm not sure on the timer
calculation, compared to ScummVM.

comment:26 by Kirben, 13 years ago

Owner: set to cyxx
Resolution: fixed
Status: newclosed

comment:27 by cyxx, 13 years ago

Looking at the disassembly of monkey2, the timing is based
on the _scummTimer variable which is incremented in the dos
interrupt 8 handler. All of the screen effects functions
just busy wait like this :

_scummTimerOld = _scummTimer
// process the effect on the current frame
while (_scummTimer < _scummTimerOld + VAR(VAR_FADE_DELAY))

In the sound driver code, the counter 0 of the 8253 chip is
initialised with a value of 5041. Its internal clock rate
being equal to 1193180 Hz, _scummTimer is incremented 236
times a second. Which is almost equivalent to 1/4th of a
scumm engine jiffie.

So, in order to match the original interpreters, I think we
simply need to multiply the VAR_FADE_DELAY value by 4
(1000/240) to get the waiting delay.

Patch attached.

by cyxx, 13 years ago

Attachment: var_fade_delay.diff added

VAR_FADE_DELAY patch

comment:28 by Kirben, 13 years ago

Good work, I added the patch to ScummVM SVN.

comment:29 by digitall, 12 months ago

Component: --Unset--Engine: SCUMM
Game: The Dig
Note: See TracTickets for help on using tickets.