The VM has been treating the entire area between the frame pointer and
the stack pointer as temp variables for the current function. There are
two problems with this:
1. The VM hasn't been updating the frame pointer correctly when multiple
methods are called within the same send/self/super instruction.
2. The VM has been recalculating the number of temp variables on every
instruction as the difference between the two pointers.
This is incorrect, as this changes with almost every instruction in
ways have nothing to do with the number of temp variables allocated
by the link instruction. Meanwhile, the VM has not been recording
the number of variables allocated by the link instruction.
The first discrepancy caused scripts to behave differently than in SSCI
when reading parameters out of bounds in certain situations.
It also prevented our uninitialized variable detection from detecting
certain reads. The second made the temp-count used for out of bounds
detection too large, made debugger output such as `stack` incorrect,
and made stepping through the link instruction in the debugger appear
to do nothing until stepping through the following instruction.
When multiple methods are called by a send/self/super instruction, each
method's link instruction increases the stack pointer further.
Method B's variables appear after method A's. The VM has been setting
the stack pointer correctly but it kept using the previous frame
pointer, so method B would re-use method A's stack area instead of the
area the VM had just allocated for B. If a script correctly initializes
variables before using them and doesn't use out of bounds parameters or
temp variables then this discrepancy doesn't make a difference.
But a lot of scripts do these bad things and accidentally rely on the
undefined values they read.
Now we update the frame pointer correctly when "carrying over" to
subsequent method calls from the same send/self/super instruction.
This matches SSCI behavior. We also now record the number of temp
variables that have been allocated by the link instruction and use
that instead of incorrectly recalculating on every instruction.
Fixes the KQ6 black widow lockup, and other KQ6 music bugs, where
scripts call Sound:fade without the required fourth parameter.
Sound:fade expects a fourth parameter, so reads it out of bounds,
and passes it as the stop-after-completion boolean to kDoSoundFade.
Scripts that called Sound:fade as the only method in a send got the same
results as in SSCI, but scripts that called Sound:play first didn't.
Fixes bugs #5625#5653#6120#6210#6252#13944
This was introduced as an extra mechanism of patching game scripts.
However, it's completely hardcoded with offsets for specific game
versions, with no endianess handling. Furthermore, the only patch
it's used for at the moment (input prompt for SQ3) has no visible
changes.
The SCI engine's script patcher has pattern matching functionality,
which allows it to be used in a variety of game versions. Furthermore,
it supports endianess handling for BE versions. Thus, it makes no
sense to keep a separate and complex script patching functionality
for a single patch that is hardcoded for a single game version with
no actual change in functionality. Since this is a fan patch, all of
these script changes can be added as part of the patch without any
changes to the engine code. We've discussed this with @sluicebox and
decided to remove all of this code, which should not have been part
of the engine's codebase because it clashes with the existing script
patcher
The current patch mechanism allows only replacing intructions in current
scripts, but not adding new instructions out of nowhere.
Thus, issue like #9646 is about to be closed as WONTFIX.
This new mechanism adds a hook on vm.cpp, before executing opcodes, and
if required, executs a new code.
It solves one of the issues of #9646, the others can be solved as well using that mechanism.
reg32_t was a transitive solution, before reg_t's were
adapted to use 32-bit addresses internally, and before
support for SCI3 was added. It was introduced as another
way to handle large script offsets in SCI3, and was only
used for the program counter (PC). It's no longer
needed, as we now support SCI3 script offsets using
reg_t's, so we can use make_reg32 in all cases where
we need to access offsets over 64KB
This fixes a problem in Lighthouse 2.0a where the mini-sub would
fail to start playing the animation of the shipwreck when clicking
on the throttle.
In SSCI, in SCI3 only, r_acc was (inadvertently?) set to the
superclass object ID whenever a super call was made. This happened
because OP_super would call to get the superclass object ID, the
calling conventions of the compiler put this return value into EAX,
and then the PMachine message processing code put whatever was in
EAX into r_acc before each message was processed.
In the game code, there are a sequence of steps that look like
this:
* First, throttle::doVerb is called when throttle is clicked on;
* Which calls getRobot::doit to tell the shipwreck robot to start
playing;
* Which calls wreckBot::init to reset the Robot for the animation;
* Which calls Hiliter::hotVerbs(0) to remove cursor hotspots;
* Which calls Hiliter::dispose to clean up since it is not used;
* Which causes Hiliter::verbList to get set to 0.
* Later, verbList is loaded into r_acc, and it is still 0;
* Then, Hiliter::dispose makes a super call to Obj::dispose;
* Then, Obj::dispose does nothing except call kDisposeClone,
which does not mutate r_acc, so r_acc is still 0 from verbList;
* Then we return back through 5 calls to throttle::doVerb;
* Then throttle::doVerb checks that r_acc is non-zero, and if so,
adds wreckBot to theDoits global, allowing the animation to
occur.
In ScummVM, without setting r_acc in the super call, the non-zero
check failed and the wreckBot didn't get put into theDoits, so the
entire sequence fell apart. In SSCI, the non-zero check happened
to succeed because the Obj::dispose super call put the Obj class
into the accumulator. So now we do that too, and now Lighthouse
2.0a works here.
Earlier versions of SSCI used EAX for other things in between the
OP_super call and the message processing, so would set r_acc from
different data, so this change does not apply to those versions.
This changes the syntax for bpk and logkernel:
Enable breakpoint on kernel call:
bpk FrameOut
Enable logging for kernel call:
bpk FrameOut log
For backward compatibility this has an alias: logkernel FrameOut
Removing a kernel call breakpoint is done with bp_del/bc now.
This groundwork enables an object to look up its static name
separately from the normal process that is used to populate
Object::_variables when an object is first constructed.
(The static name property needs to be able to be retrieved from
objects inside of earlier save games whose name properties may
have already been modified at runtime, so the code cannot simply
pluck the value out of Object::_variables when they are first
initialised and then persisted into the save game, as nice and
easy as that would have been.)
This commit also helps to clarify the situation with relocation
tables in SCI1 games that start with a zero entry.
Refs Trac#9780.
In SCI2/2.1, variable indexes are used along with a range encoded
in the interpreter executable to determine whether an object
variable is a view-related variable. Operands to aTop, sTop, ipToa,
dpToa, ipTos, and dpTos are byte offsets into an object, which
are divided by two to get the varindex to check against the
interpreter range.
In SCI3, objects in game scripts contain groups of 32 selectors,
and each group has a flag that says whether or not the selectors
in that group are view-related. Operands to aTop, sTop, ipToa,
dpToa, ipTos, and dpTos are selectors.
This patch includes enhancements to the ScummVM integration with
SCI engine, with particular focus on SCI32 support.
1. Fixes audio volumes syncing erroneously to ScummVM in games
that modify the audio volume without user action (e.g. SCI1.1
talkies that reduce music volume during speech playback). Now,
volumes will only be synchronised when the user interacts with
the game's audio settings. This mechanism works by looking for
a known volume control object in the stack, and only syncing
when the control object is present. (Ports and planes were
researched and found unreliable.)
2. Fixes audio syncing in SCI32 games that do not set game
volumes through kDoSoundMasterVolume/kDoAudioVolume, like GK1,
GK2, Phant1, and Torin.
3. Fixes speech/subtitles syncing in SCI32 games that do not use
global 90, like LSL6hires.
4. Fixes in-game volume controls in SCI32 games reflecting
outdated audio volumes when a change is made during the game
from the ScummVM launcher.
5. Fixes SCI32 games that would restore volumes from save games
or reset volumes on startup, which caused game volumes to be
out-of-sync with ScummVM when started.
6. ScummVM integration code for audio sync has been abstracted
into a new GuestAdditions class. This keeps the ScummVM-
specific code all in one place, with only small hooks into the
engine code. ScummVM integrated save/load code should probably
also go here in the future.
Fixes Trac#9700.
Some kernel calls need to be able to call back into game script
code, which requires a valid stack pointer for use with
invokeSelector.
An example of this is the guest additions code that syncs audio
volumes from ScummVM: it needs to be able to call into the game
scripts responsible for managing the in-game audio volume UI.
SCI3 includes four new opcodes:
* op_info[0x26][0] puts -info- flag in accumulator
* op_infoSP[0x26][1] pushes -info- flag to stack
* op_superP[0x27][0] puts -super- reference in accumulator
* op_superPSP[0x27][1] pushes -super- reference to stack
The implementation of these opcodes was correct already, but the
opcode names given were a bit misleading (the value is not always
stored to accumulator), and magic numbers were used for these
opcodes in places.
A review of the opcode table in Phant2 indicates that there are
no other new opcodes for SCI3.
1. pushi opcode now displays decimal value and selector value (if
one exists) in-line
2. lofsa, lofss, and super opcodes now display resolved
object/class names
3. Opcode arguments are visually aligned
Silently returning zero values can cause games to break. e.g.
Shivers 1 room 35170 has a script bug where vJoystick::handleEvent
makes a super call which causes doVerb to be called a second time
with no arguments. In the original game this happened to work
because the value already on the stack happened to be 1. In ScummVM
this silently (unless VM debug messages were enabled) failed
because the uninitialised read value was forced to 0.
Revert "SCI32: Fix KQ7 1.51 video background"
This reverts commit c8affb54cc.
Revert "SCI32: Fix crash when kShowMovie is called but the video cannot be found"
This reverts commit 93b06f4a9e.
Revert "SCI32: Fix KQ7 1.51 basic video playback"
This reverts commit cdab24aa07.
Revert "SCI32: Additional Video32 documentation"
This reverts commit 4ff0924e57.
Revert "SCI32: Implement kShowMovie"
This reverts commit 13297c1929.
Modifying a value above the stack pointer doesn't seem to make much
sense. This was added in FreeSCI back in 2002 in a pair of commits that
did not make clear what the purpose of this was. My guess is that it
attempted to adjust argc, but failed. This wouldn't have been noticed
since argc was always set correctly by make_exec_stack_entry (which is
now the ExecStack constructor).
ExecStack now stores the kernel call number as well as the subfunction.
This allows kStub and backtraces to log the actual subfunction called.
The kernel call number in ExecStack used to be stored in the
debugSelector field. It now has its own field, to avoid confusion.
sluicebox
Hubert Maier
Eugene Sandulenko
Orgad Shaneh
Filippos Karapetis
Zvika Haramaty
Colin Snover
Willem Jan Palenstijn