Opened 13 years ago

Closed 3 years ago

#2687 closed defect (fixed)

BASS: Officer Blunt wrong animation (?)

Reported by: SF/adventureguy Owned by: digitall
Priority: normal Component: Engine: Sky
Keywords: Cc:
Game: Beneath a Steel Sky

Description

Hello!
I think I found a bug in the game Beneath a Steel Sky.
I tested the English Talkie game on Windows with the
ScummVM stable 0.8.2 built and the 0.9.0pre built on
17th June 2006. When I try to talk or give something to
Officer Blunt on the ground level after the dog is
swimming in the lake, he talks in his normal speech
animation, so it looks like he's standing in the lake.
I attached a screenshot. Was this also a problem in the
original game or is this a bug in ScummVM?

Ticket imported from: #1507756. Ticket imported from: bugs/2687.

Attachments (8)

blunt.PNG (15.4 KB ) - added by SF/adventureguy 13 years ago.
Blunt's animation
SKY-VM.SAV (105 bytes ) - added by SF/adventureguy 13 years ago.
Might be needed for loading savgame. It's savegame 5.
SKY-VM.004 (14.9 KB ) - added by SF/adventureguy 13 years ago.
BASS savegame
SKY-VM.008 (14.8 KB ) - added by digitall 9 years ago.
20100829 ScummVM savegame for replication
original-0372-interpreter.zip (157.9 KB ) - added by digitall 9 years ago.
Original 0.0372 Interpreter to show failure to replicate
original-0372-savegames.zip (94.4 KB ) - added by digitall 9 years ago.
Original 0.0372 Savegames to show failure to replicate
build-scripts-for-gcc-4.tar.gz (1.0 KB ) - added by digitall 9 years ago.
Build Scripts to build ScummVM ~r16000-17000 with GCC-4
corrected-sky-cpt.zip (125.7 KB ) - added by digitall 9 years ago.
Corrected SKY.CPT

Download all attachments as: .zip

Change History (23)

by SF/adventureguy, 13 years ago

Attachment: blunt.PNG added

Blunt's animation

comment:1 by sev-, 13 years ago

Summary: Officer Blunt wrong animation (?)BASS: Officer Blunt wrong animation (?)

by SF/adventureguy, 13 years ago

Attachment: SKY-VM.SAV added

Might be needed for loading savgame. It's savegame 5.

by SF/adventureguy, 13 years ago

Attachment: SKY-VM.004 added

BASS savegame

comment:2 by SF/adventureguy, 13 years ago

I now added a savegame. Just give Blunt any item, he will
act strangely (at least for me).

comment:3 by joostp, 13 years ago

"Was this also a problem in the original game or is this a bug in ScummVM?" - it would be great if someone could confirm/deny this problem also exists in the original.

comment:4 by sev-, 11 years ago

What is the status of this item?

comment:5 by digitall, 9 years ago

Replicated with latest bass-cd-1.2.zip i.e. "Beneath a Steel Sky (v0.0372 cd)" on :
ScummVM 1.2.0svn52433 (Aug 29 2010 03:40:31)
Features compiled in: Vorbis FLAC MP3 ALSA SEQ RGB zLib FluidSynth
on Linux x86_32 2.4.34

Will try to replicate with original interpreter.

by digitall, 9 years ago

Attachment: SKY-VM.008 added

20100829 ScummVM savegame for replication

comment:6 by digitall, 9 years ago

Failed to replicate with original interpreter 0.0372 CD under Dosbox. This looks like a real regression.
Attaching savegames and original interpreter executables to allow replication of failure to replicate.

by digitall, 9 years ago

Original 0.0372 Interpreter to show failure to replicate

by digitall, 9 years ago

Attachment: original-0372-savegames.zip added

Original 0.0372 Savegames to show failure to replicate

comment:7 by digitall, 9 years ago

Played through to this point with ScummVM 0.7.0 prebuilt win32 and same game data and bug is not present
Tried the savegame produced on v1.1.1 prebuilt win32 and bug is present.

Determined that v0.7.1 does not exhibit the bug, but v0.8.0 does.
This indicates that the regression is between r17267 and r19355.

Will bisect between these to narrow the cause.

comment:8 by digitall, 9 years ago

Ah, I had neglected to realise that those revisions are correct, but the code is from the release branches, not the trunk.

0.7.1 was derived from 0.7.0 which forked from the trunk at r16049 and 0.8.0 was forked at r19131, so the correct range for the cause on the trunk was between r16049 and r19131.

Have traced back with builds to r17116 on trunk with the bug still occuring, but tracing earlier is presenting some difficulties with newer GCC, but the most likely hypothesis is as follows:

That the bug is not occuring in 0.7.* as these versions do not use the external compact file, SKY.CPT, instead using data built into the executable. This was changed in trunk revision r16053 which falls in the range above, so that 0.8.0 and later builds which use the external SKY.CPT file exhibit the bug.

comment:9 by digitall, 9 years ago

I have now managed to builds earlier than r17116 and can confirm that this hypothesis is confirmed.

The last build without this issue is r16052.
r16053 then moved the data out to the external sky.cpt file :
------------------------------------------------------------------------
r16053 | lavosspawn | 2004-12-15 06:48:08 +0000 (Wed, 15 Dec 2004) | 3 lines

moved compacts and related static data out of the scummvm.exe into an external file
(available from www.lavosspawn.de/tmp/sky_cpt.zip)
------------------------------------------------------------------------

After this, there are a number of revisions with broken builds, followed by a lack of animation altogether.

This was corrected by r16828, which is also the first revision showing the issue with Blunt i.e. :
------------------------------------------------------------------------
r16828 | lavosspawn | 2005-02-20 18:33:36 +0000 (Sun, 20 Feb 2005) | 2 lines

fix missing speech animations. I introduced that bug when I moved the compacts out of the scummvm executable.
------------------------------------------------------------------------

Am attaching build script and a couple of minor patches, which are necessary (at least for me) to build revisions of this age under newer GCC (4.4.3 in my case), for reference.

Will now start looking for the exact difference in the engine / compact data which is causing this.

by digitall, 9 years ago

Build Scripts to build ScummVM ~r16000-17000 with GCC-4

comment:10 by digitall, 9 years ago

Have traced the fault by adding debugging output to code in sky/logic.cpp for both r16052 and r16074 (with r16828 patch applied) i.e. without and with bug.

The following output occurs while speaking to Officer Blunt while crouched :
Logic::fnSpeakWait(id=16601, message=17168, animation=158)
Logic::fnSpeakMe(targetId=16601, mesgNum=17168, animNum=158)
Logic::stdSpeak(textNum=17168, animNum=158, base=0)

However, Animation 158 maps to target->grafixProgId = 16696 in the correct case and 16607 in the buggy case.

Looking these up in sky/skydefs.h shows that these are:
ID_SC31_GUARD_TALK (standing) 16607
ID_SC31_GUARD2_TALK (crouching) 16696

In the older correct code, sky/talks.h holds a table animTalkTableVal[] mapping these and offset 158 in this table is the correct ID_SC31_GUARD2_TALK, but the newer code returns ID_SC31_GUARD_TALK instead from the external compact sky.cpt. By tracing the code in compact.cpp, I have determined that the errorneous value is present in the SKY.CPT file at byte offset 0x42F6. Changing this to16696 corrects the issue.

Attaching a corrected CPT file.

If this is not a desirable solution, this fix could also be accomplished by adding a code workaround to sky/logic.cpp to check for Animation 158 and force 16696.

by digitall, 9 years ago

Attachment: corrected-sky-cpt.zip added

Corrected SKY.CPT

comment:11 by digitall, 5 years ago

As this bug has not been updated for some time, I am adding this comment to record why and the current status.

We now have a solution which fixes this bug i.e. a binary patch to the SKY.CPT external data file and/or a code workaround to the sky engine which overrides the grafixProgId value for Animation 158 and "forces" / uses a hard coded value.

However, this has not been committed and the bug closed as this is not an acceptable solution to several team members. The reason for this is as follows:

We have the source code for the tool to generate sky.cpt in the tree as devtools/skycpt, but this was not designed for general use and was only committed to comply with Debian licensing as this has issues with "binary blobs" and requires the source code of any tool used to generate them:
https://github.com/scummvm/scummvm/blob/master/devtools/skycpt/README#L13

This tool uses savegame states from all the game versions saved prior to the removal of the static data from the sky engine as input data to generate the sky.cpt file. As such, it is not easy to modify and without knowing the full format of the external SKY.CPT datafile, any further impact of the binary patch proposed is unclear i.e. it may solve this bug and cause others :/

Thus for this bug to be closed, we need to either patch or replace the skycpt tool to ensure that the Debian licensing issues are satisfied i.e. that the binary external datafile, sky.cpt is accurately and reliably produced from an open source tool.

comment:12 by eriktorbjorn, 4 years ago

Would it help if, in addition to the tool, we also had these save games archived somewhere? I don't have the necessary versions of the game, so I couldn't provide them even if I wanted to, so I'm just asking out of curiosity.

comment:13 by digitall, 3 years ago

Fixes for this bug have now been committed to the master tree by lavosspawn as three commits:
https://github.com/scummvm/scummvm/commit/7874f517f79da796e480e014d72f536d3abe2af8

https://github.com/scummvm/scummvm/commit/e40b4850f7ac94766c90873a9ac3e598f31bba40

https://github.com/scummvm/scummvm/commit/4d94bdc0648f19b41be6d6bc313244165a3448a5

Marked bug as fixed.

comment:14 by digitall, 3 years ago

Resolution: fixed
Status: newpending

comment:15 by sev-, 3 years ago

Owner: set to digitall
Status: pendingclosed
Note: See TracTickets for help on using tickets.