Opened 3 months ago

Last modified 3 months ago

#15303 new defect

SCI: LSL5: Timed message skipped during LSL5 coffee scene (symptom of a bigger problem?)

Reported by: eriktorbjorn Owned by:
Priority: normal Component: Engine: SCI
Version: Keywords:
Cc: Game: Leisure Suit Larry 5

Description (last modified by eriktorbjorn)

(Description updated after discussion below.)

During the coffee delivery scene in Leisure Suit Larry 5 (the optional tutorial after the intro), I noticed that one of the lines was skipped, or rather the text box for it was dismissed so quickly that I could barely see it. The skipped message was supposed to be:

"Yes, I know" you offer proudly, "I'm the Chief Tape Rewinder and Sterilizer on this project!"

This glitch is quite timing sensitive, so I have include a savegame during the scene itself. Don't skip any text boxes, just let the scene play out with default settings for everything.

The problem seems to be the way the Talker class times out text boxes. First the doit method calculates a duration based on string length and, I assume, text speed, and assigns it to the ticks property. Then say adds another 60 ticks for good measure, and adds the current game time as reported by GetTime. The doit method then polls GetTime to see if the text box should be dismissed:

		(if (> (GetTime) ticks)
			(self dispose: disposeWhenDone)
			(return)
		)

What seems to happen is that if GetTime is close enough to 32,767, adding the duration causes ticks to become negative. Which means that GetTime is immediately greater than ticks.

I believe this means that the glitch could happen at any point in the game, and I was just lucky (?) to catch it here. And the same mechanism seems to be present in several other games, but no one (?) ever noticed. I blame the lengthy cutscenes in LSL5 for triggering it.

I imagine it should be possible to fix, at least on a case-by-case basis. The arithmetic workarounds seem like a reasonable place to start, but it might need some extending to call a custom C++ function to do the comparison? We know that ticks should stay constant until the text box is dismissed.

I'm more worried about what other things may potentially be affected, though.

Edit: On a closer look, it seems possible (but far from certain) that the issue has been fixed in the localized versions of LSL5. Unfortunately, I do not own any such version myself. See comment further down. Much, much further down.

Attachments (5)

lsl5.003 (32.4 KB ) - added by eriktorbjorn 3 months ago.
lsl5.001 (39.3 KB ) - added by eriktorbjorn 3 months ago.
lsl5-patch.txt (2.4 KB ) - added by eriktorbjorn 3 months ago.
lsl5-patch-v2.txt (3.0 KB ) - added by eriktorbjorn 3 months ago.
lsl5-patch-v3.txt (2.5 KB ) - added by eriktorbjorn 3 months ago.

Download all attachments as: .zip

Change History (45)

by eriktorbjorn, 3 months ago

Attachment: lsl5.003 added

comment:1 by eriktorbjorn, 3 months ago

Description: modified (diff)

comment:2 by eriktorbjorn, 3 months ago

Description: modified (diff)

comment:3 by eriktorbjorn, 3 months ago

Description: modified (diff)

comment:4 by eriktorbjorn, 3 months ago

The 30 cycles may be the delay between two messages. But I still think it's possible that it uses ticks for the message timeout?

The actual timeout handler is probably this piece of code in System.sc. But that's probably as far as my debugging goes.

			((and ticks (<= (-= ticks (Abs (- gGameTime lastTicks))) 0))
				(= ticks 0)
				(self cue:)
			)

comment:5 by m-kiewitz, 3 months ago

Description: modified (diff)

30 ticks really means 30 ticks
And it seems these 30 ticks are actually the duration between dialog boxes, so it's actually part of the NEXT state that follows after the message box has been removed.

Can you please check what the original interpreter is doing?
Also that's not just the follow-up dialog when you bring coffee, but it's like the 30th dialog box. Please also include which script you are posting from.

This is script 150, sCartoon::changeState, and the state number would be useful too.
I just checked my multilingual Larry 5 copy. I skipped the dialog boxes, but this specific text stays on screen for seconds.

I think I read something that there indeed is a time overflow problem with ticks, but it happens not that often.

Last edited 3 months ago by m-kiewitz (previous) (diff)

comment:6 by m-kiewitz, 3 months ago

If this is indeed a tick overflow, then the same should happen in the original interpreter, and there is not much we can do about it. It wouldn't be a ScummVM bug.

And trying to fix this would be a major thing, because you would have to patch basically all games, because they all have the same overflow problem.

Version 0, edited 3 months ago by m-kiewitz (next)

comment:7 by eriktorbjorn, 3 months ago

Description: modified (diff)

comment:8 by eriktorbjorn, 3 months ago

Can you please check what the original interpreter is doing?
Also that's not just the follow-up dialog when you bring coffee, but it's like the 30th dialog box. Please also include which script you are posting from.

I thought I had updated the description before I went to bed, but I must have forgot to press the save button. I hope it's a little clearer now?

The problem is that even if the problem happens with the original, I probably couldn't reproduce it. First I had a savegame outside the door where it seemed to happen every time when I tried it on my phone. Then I moved it to my computer for debugging, and found that it only happened some of the time, so it presumably depends on hitting a very specific window in time. It may be a wide window, but it still probably only happened because I waited for the intro to run its course, rather than skipping it.

That's why I made the second, attached savegame during the cutscene instead. But I assume that's not compatible with the original interpreter.

I also assumed that this could be a pretty nightmarish thing to fix, if it's a script shortcoming. But I've been surprised in the past by what could be fixed so I figured I'd better write a bug report anyway.

comment:9 by m-kiewitz, 3 months ago

It should in theory happen every ca. 18 minutes.
So you could for example pause the intro at the start for this amount of time, and then continue with it.

comment:10 by eriktorbjorn, 3 months ago

But if I'm even a few seconds off, it probably won't happen. I tried a few times in ScummVM to no avail before filing this bug report, but had to resort to extracting the savegame from my phone, so that I could work with that until I had a savegame that reproduces the problem every time for me.

This seems to be the part that updates ticks, retrieved with "disasm Script doit bc":

0004:06dd: 63 16          pToa 	ticks[16]
0004:06df: 30 1f 00       bnt  	001f  [0701]
0004:06e2: 36             push 
0004:06e3: 78             push1			; superClass
0004:06e4: 89 58          lsg  	58		; 88, 'X'
0004:06e6: 63 18          pToa 	lastTicks[18]
0004:06e8: 04             sub  
0004:06e9: 36             push 
0004:06ea: 43 3d 02       callk	Abs[3d],	02
0004:06ed: 04             sub  
0004:06ee: 65 16          aTop 	ticks[16]
0004:06f0: 36             push 
0004:06f1: 35 00          ldi  	00
0004:06f3: 24             le?  
0004:06f4: 30 0a 00       bnt  	000a  [0701]
0004:06f7: 35 00          ldi  	00
0004:06f9: 65 16          aTop 	ticks[16]
0004:06fb: 38 8d 00       pushi	008d		; 141, cue
0004:06fe: 76             push0			; species
0004:06ff: 54 04          self 	04

But I'm still not sure what happens here. I thought it was that integer overflow caused some large negative value to be passed to kAbs but that doesn't seem to be the case. There were a couple of large - but not that large? - positive values, but I don't know if they came from this script or not.

Last edited 3 months ago by eriktorbjorn (previous) (diff)

comment:11 by m-kiewitz, 3 months ago

I think sluicebox wrote something somewhere regarding a tick overflow problem.
Wasn't it King's Quest 5? Or maybe it was a youtube video where sluicebox mentioned that.

It was some weird speedrun glitch that happened by pure chance.

Oh and right, if we fix this, speedrunners will possibly get mad at us lol

comment:12 by m-kiewitz, 3 months ago

Of course we could make this an option like "keep speed runner relevant bugs"

comment:13 by eriktorbjorn, 3 months ago

Well, we'd have to figure out why it happens first. And when I say "we", I really mean "not me" because I'm at my wits' end.

I thought for sure it would have to be signed integer overflow, and where I've inserted debug messages it has been so tantalizingly close to that point. But if so, it must be happening where I'm not looking.

Of course we could make this an option like "keep speed runner relevant bugs"

Speedrunners are a strange lot, and I've probably already annoyed them by fixing the cracked Maniac Mansion GOG and Steam are selling. There's even a feature request - https://bugs.scummvm.org/ticket/14815 - to make that optional, but I'm ignoring it. :-)

comment:14 by m-kiewitz, 3 months ago

They may call that "glitchless", which means some bugs are allowed for some reason, but some others aren't. I don't understand that either.

And in your case yes it's even more weird because it's a cracked game. How can a cracked game be a valid copy that is accepted for speed running? If modifications are fine, then I can easily mod SCI+AGI games to make me complete the game faster.

Of course in this case one can actually save tons of time in King's Quest 5 and it's not a modification, but a quirk of the engine and it actually requires perfect execution and planning.

I think ticks should be an unsigned integer, but I'm not sure. I think the problem is actually that it returns to 0, which is why a comparison is valid immediately (no proper match was done for checking additional left-over ticks). It's a classic overflow.

comment:15 by eriktorbjorn, 3 months ago

And in your case yes it's even more weird because it's a cracked game. How can a cracked game be a valid copy that is accepted for speed running?

Getting off-topic, but the excuse I've heard is that since it's sold that way it means it is an official release, and meant to be that way. (My interpretation is... different, with lots of name-calling.)

It's a classic overflow.

I think you're right, and I've been barking up the wrong tree. I've been looking at doit in Script, when I should have been looking at doit in Talker.

In Script, ticks is a countdown that's never particularly large. But in Talker... First startText calculates a delay based on string length and a global variable that's I assume is the text speed setting, or something like that:

(= ticks (Max 180 (/ (* global155 (StrLen @temp0)) 2)))

But then say goes and adds the current time, plus another 60 ticks for good measure.

(+= ticks (+ 60 (GetTime)))

Which doit then compares against GetTime to see if it's time to auto-dismiss the dialog.

At least that's how I read it. At the point where the text box skips, GetTime is still below 32,767, but the calculated delay is enough to push it beyond. From what I understand, arithmetics on reg_t are signed, but even if they weren't the problem would probably remain once ticks gets pushed past 65,535.

And from a quick-and-naive search, that (+= ticks (+ 60 (GetTime))) line is present in a lot of games, supporting the theory I expressed on Discord earlier: @sluicebox is either going to love me or hate me for this.

(Though I also wouldn't be the least surprised if he was already perfectly aware of it.)

comment:16 by m-kiewitz, 3 months ago

Of course in theory we could hack something together that whenever something touches GetTime we could mark that reg_t as timer related and expand it to a DWORD, which would then make it roll over after ages, but basically everything would have to get adjusted.
Pointers were WORDs in original SCI, that also was a hard memory limit (64k), but our SCI doesn't do that, instead it uses 2 WORDs, that's why we have reg_t.
This alone already caused tons of issues, that's also why I introduced type checking 10+ years ago, because some scripts do math against an offset and expect something useful to get returned.
Of course even that were bugs in original SCI, and sometimes it just happens to work, or it worked sometimes, and sometimes it didn't.

I don't think it's really worth it, but maybe there is a way that would be easier, idk.

Last edited 3 months ago by m-kiewitz (previous) (diff)

comment:17 by eriktorbjorn, 3 months ago

I don't think it's really worth it, but maybe there is a way that would be easier, idk.

I'll happily stay out of that decision. I won't lose any sleep if the bug report is closed. :-)

When I decided to report it, I thought it was more serious because it seemed to happen every time, at least from the savegame I had. (Kind of like a bug that was fixed some time ago, where the King's Quest IV intro would apparently hang every time if you allowed it to play out in its entirety.) But now it seems that it just happens during a short recurring window in possibly a lot of the games, and no one noticed.

You'd think that I wouldn't be the one to stumble over these things. Of the Sierra games I own, I would estimate that about 40% of them I've never played past the first few rooms. And there are a couple I don't own at all. Of the ones I did play, many were long before ScummVM supported them and I've forgotten almost everything about them.

Which is why I've been slowly making my way through Leisure Suit Larry now. Emulation has gotten so much better since last time, for which I'm grateful!

comment:18 by eriktorbjorn, 3 months ago

I don't think it's really worth it, but maybe there is a way that would be easier, idk.

Rather than trying to fix everything, I guess it would also be possible to target them on a case-by-case basis, e.g. by adding a script workaround for the comparison opcode. That might require a new type of workaround for reg_t::lookForWorkaround() I guess, since the result can't be a hard-coded value? (Or is there already a mechanism for calling custom functions instead? I couldn't find one.)

comment:19 by eriktorbjorn, 3 months ago

Description: modified (diff)
Summary: SCI: LSL5: Timed message skipped during LSL5 coffee sceneSCI: LSL5: Timed message skipped during LSL5 coffee scene (symptom of a bigger problem?)

comment:20 by eriktorbjorn, 3 months ago

Description: modified (diff)

comment:21 by eriktorbjorn, 3 months ago

Searching through sluicebox's scripts - https://github.com/sluicebox/sci-scripts - I saw the following games check if (> (GetTime) ticks):

Game Version Where
King's Quest 5 All Amiga, All Macintosh, cd-dos-1.000.052 Talker.sc
Police Quest 3 All Talker.sc
Space Quest 1 All Talker.sc
Leisure Suit Larry 5 dos-1.000 Talker.sc
The Castle of Dr. Brain All Talker.sc and rm280.sc

So that's perhaps not as bad as I first thought? Of course the problem could still be there in different forms, or with different variable names.

comment:22 by eriktorbjorn, 3 months ago

Here's how the English version of LSL5 does it:

	(method (doit)
		(if (> (GetTime) ticks)
			(self dispose: disposeWhenDone)
			(return)
		)
		(if eyes
			(self cycle: eyes)
		)
		(if mouth
			(self cycle: mouth)
		)
	)

Here's how the German, French, Spanish, and Italian versions do it (pun not intended):

	(method (doit)
		(if (> (- gGameTime ticks) 0)
			(self dispose: disposeWhenDone)
			(return)
		)
		(if eyes
			(self cycle: eyes)
		)
		(if mouth
			(self cycle: mouth)
		)
	)

Could that be something they did to fix the problem? The gGameTime variable is also present in the English version, so it wasn't added specifically for these. But mathematics on overflowing integers makes my head hurt. Here's how the EGA DOS version does it:

	(method (doit)
		(if (and (> gGameTime ticks) (< (- gGameTime ticks) 28672))
			(self dispose: disposeWhenDone)
			(return)
		)
		(if eyes
			(self cycle: eyes)
		)
		(if mouth
			(self cycle: mouth)
		)
	)

Which seems just bizarre to me. It's as if someone stumbled over the bug once and added a quick-and-dirty hack for it, which was then rejected by whoever made the VGA version.

comment:23 by eriktorbjorn, 3 months ago

If I search for (> gGameTime ticks) instead, I find it in the following games:

Game Version Where
Leisure Suit Larry 5 amiga-1.000, dos-ega-1.000, All Macintosh Talker.sc
EcoQuest 1 All except cd-dos-1.1 Talker.sc

(I'll stop now.)

comment:24 by eriktorbjorn, 3 months ago

Description: modified (diff)

comment:25 by eriktorbjorn, 3 months ago

Description: modified (diff)

comment:26 by eriktorbjorn, 3 months ago

Description: modified (diff)

comment:27 by eriktorbjorn, 3 months ago

I've had it happen in another scene, which seems to confirm that it's a recurring problem. You may be able to reproduce it from this new savegame, but you have to wait a little bit before talking to the maitre d'. (Or talk to him multiple times.)

by eriktorbjorn, 3 months ago

Attachment: lsl5.001 added

comment:28 by eriktorbjorn, 3 months ago

Experimenting in C (because I lack the tools and expertise to do it in SCI), it seems that the method in the non-English versions will work.

I used this as my test program:

include <stdio.h>
#include <stdint.h>

int main() {
	int16_t GameTime = 0;

	do {
		int16_t ticks = GameTime + 5;
		int16_t tmp = GameTime;
		int i;

		for (i = 0; i < 10; i++) {
			GameTime++;

			if (GameTime > ticks) {
				if (i < 5)
					printf("%d: Method 1 triggered prematurely\n", tmp);
				if (i > 5)
					printf("%d: Method 1 triggered too late\n", tmp);
				break;
			}
		}

		if (i >= 10)
			printf("%d: Method 1 never triggered\n", tmp);

		GameTime = tmp;

		for (i = 0; i < 10; i++) {
			int16_t diff;

			GameTime++;

			diff = GameTime - ticks;
			if (diff > 0) {
				if (i < 5)
					printf("%d: Method 2 triggered prematurely\n", tmp);
				if (i > 5)
					printf("%d: Method 2 triggered too late\n", tmp);
				break;
			}
		}

		if (i >= 10)
			printf("%d: Method 2 never triggered\n", tmp);

		GameTime = tmp;
		GameTime++;
	} while (GameTime != 0);

	return 0;
}

Which produced the following output:

32762: Method 1 never triggered
32763: Method 1 triggered prematurely
32764: Method 1 triggered prematurely
32765: Method 1 triggered prematurely
32766: Method 1 triggered prematurely
Last edited 3 months ago by eriktorbjorn (previous) (diff)

comment:29 by eriktorbjorn, 3 months ago

Here is the output from "disasm Talker doit bcc" in the English version that I have:

0031:064b: 0x76,                                // push0			; species
0031:064c: 0x43, 0x42, 0x00,                    // callk	GetTime[42],	00
0031:064f: 0x36,                                // push 
0031:0650: 0x63, 0x5c,                          // pToa 	ticks[5c]
0031:0652: 0x1e,                                // gt?  
0031:0653: 0x30, 0x08, 0x00,                    // bnt  	0008  [065e]
0031:0656: 0x39, 0x6c,                          // pushi	6c		; 108, 'l', dispose
0031:0658: 0x78,                                // push1			; superClass
0031:0659: 0x67, 0x5e,                          // pTos 	disposeWhenDone[5e]
0031:065b: 0x54, 0x06,                          // self 	06
0031:065d: 0x48,                                // ret  
0031:065e: 0x63, 0x58,                          // pToa 	eyes[58]
0031:0660: 0x30, 0x07, 0x00,                    // bnt  	0007  [066a]
0031:0663: 0x38, 0xb0, 0x00,                    // pushi	00b0		; 176, cycle
0031:0666: 0x78,                                // push1			; superClass
0031:0667: 0x36,                                // push 
0031:0668: 0x54, 0x06,                          // self 	06
0031:066a: 0x63, 0x5a,                          // pToa 	mouth[5a]
0031:066c: 0x30, 0x07, 0x00,                    // bnt  	0007  [0676]
0031:066f: 0x38, 0xb0, 0x00,                    // pushi	00b0		; 176, cycle
0031:0672: 0x78,                                // push1			; superClass
0031:0673: 0x36,                                // push 
0031:0674: 0x54, 0x06,                          // self 	06
0031:0676: 0x48,                                // ret 

Here is the corresponding output from the German version, provided by Discord user invwar:

0030:0649: 0x89, 0x58,                          // lsg      58			; 88, 'X'
0030:064b: 0x63, 0x5c,                          // pToa     ticks[5c]
0030:064d: 0x04,                                // sub  
0030:064e: 0x36,                                // push 
0030:064f: 0x35, 0x00,                          // ldi      00
0030:0651: 0x1e,                                // gt?  
0030:0652: 0x30, 0x08, 0x00,                    // bnt      0008  [065d]
0030:0655: 0x39, 0x6c,                          // pushi    6c			; 108, 'l', dispose
0030:0657: 0x78,                                // push1			; superClass
0030:0658: 0x67, 0x5e,                          // pTos     disposeWhenDone[5e]
0030:065a: 0x54, 0x06,                          // self     06
0030:065c: 0x48,                                // ret  
0030:065d: 0x63, 0x58,                          // pToa     eyes[58]
0030:065f: 0x30, 0x07, 0x00,                    // bnt      0007  [0669]
0030:0662: 0x38, 0xb0, 0x00,                    // pushi    00b0		; 176, cycle
0030:0665: 0x78,                                // push1			; superClass
0030:0666: 0x36,                                // push 
0030:0667: 0x54, 0x06,                          // self     06
0030:0669: 0x63, 0x5a,                          // pToa     mouth[5a]
0030:066b: 0x30, 0x07, 0x00,                    // bnt      0007  [0675]
0030:066e: 0x38, 0xb0, 0x00,                    // pushi    00b0		; 176, cycle
0030:0671: 0x78,                                // push1			; superClass
0030:0672: 0x36,                                // push 
0030:0673: 0x54, 0x06,                          // self     06
0030:0675: 0x48,                                // ret

comment:30 by eriktorbjorn, 3 months ago

Putting it more concisely, the scripts appear identical to me apart from the first few instructions. English:

0031:064b: 0x76,                                // push0			; species
0031:064c: 0x43, 0x42, 0x00,                    // callk	GetTime[42],	00
0031:064f: 0x36,                                // push 
0031:0650: 0x63, 0x5c,                          // pToa 	ticks[5c]

German:

0030:0649: 0x89, 0x58,                          // lsg      58			; 88, 'X'
0030:064b: 0x63, 0x5c,                          // pToa     ticks[5c]
0030:064d: 0x04,                                // sub  
0030:064e: 0x36,                                // push 
0030:064f: 0x35, 0x00,                          // ldi      00

So the German version uses an additional byte, and I'm not sure yet if the script patcher can insert code or if it has to stay within the original size limitations.

comment:31 by eriktorbjorn, 3 months ago

If the script has to stay within the original size, it should be possible to replace "btn 0008" with "btn 08" to save one byte. But doing so doesn't fix the problem for me, because now the text box never times out. So either I did something wrong, or there's more to this puzzle than I can figure out.

by eriktorbjorn, 3 months ago

Attachment: lsl5-patch.txt added

comment:32 by eriktorbjorn, 3 months ago

I've attached my non-working patch.

comment:33 by dwatteau, 3 months ago

Per the request on Discord, I've added the disasm output from my French release:

0032:0649: 0x89, 0x58,                          // lsg          58              ; 88, 'X'
0032:064b: 0x63, 0x5c,                          // pToa         ticks[5c]
0032:064d: 0x04,                                // sub
0032:064e: 0x36,                                // push
0032:064f: 0x35, 0x00,                          // ldi          00
0032:0651: 0x1e,                                // gt?
0032:0652: 0x30, 0x08, 0x00,                    // bnt          0008  [065d]
0032:0655: 0x39, 0x6c,                          // pushi        6c              ; 108, 'l', dispose
0032:0657: 0x78,                                // push1                        ; superClass
0032:0658: 0x67, 0x5e,                          // pTos         disposeWhenDone[5e]
0032:065a: 0x54, 0x06,                          // self         06
0032:065c: 0x48,                                // ret
0032:065d: 0x63, 0x58,                          // pToa         eyes[58]
0032:065f: 0x30, 0x07, 0x00,                    // bnt          0007  [0669]
0032:0662: 0x38, 0xb0, 0x00,                    // pushi        00b0            ; 176, cycle
0032:0665: 0x78,                                // push1                        ; superClass
0032:0666: 0x36,                                // push
0032:0667: 0x54, 0x06,                          // self         06
0032:0669: 0x63, 0x5a,                          // pToa         mouth[5a]
0032:066b: 0x30, 0x07, 0x00,                    // bnt          0007  [0675]
0032:066e: 0x38, 0xb0, 0x00,                    // pushi        00b0            ; 176, cycle
0032:0671: 0x78,                                // push1                        ; superClass
0032:0672: 0x36,                                // push
0032:0673: 0x54, 0x06,                          // self         06
0032:0675: 0x48,                                // ret

comment:34 by m-kiewitz, 3 months ago

It loads global 58h, so yeah there has to be additional code somewhere else.
That global 58h is probably not used in the original release, but it's used to set ticks.
Therefore you set ticks to 0, therefore the message is not skipped automatically anymore.

comment:35 by eriktorbjorn, 3 months ago

I thought global 58h was what sluicebox's scripts call gGameTime which is present in both English and non-English versions. Though it seems non-English versions do more with it.

But maybe I should keep using the GetTime kernel function instead...

comment:36 by m-kiewitz, 3 months ago

If it's literally gametime, then there has to be another error.

comment:37 by eriktorbjorn, 3 months ago

I could be barking up the wrong tree, but it looks to me like gGameTime doesn't change while it's waiting for the text box. At least for some of the text boxes.

I can't see that the German version updates the variables in any other scripts than the English version does it, but maybe those scripts get invoked in different places?

That could be avoided by calling GameTime but that leads to another problem: Pushing gGameTime onto the stack is done in 2 bytes. Pushing the result of GameTime onto the stack requires 4 bytes, I believe. Well, there are two more branch instructions that could be converted to the shorter form, so maybe that's doable...

But my attempt caused a stack underflow, so I'm clearly messing things up even worse.

comment:38 by m-kiewitz, 3 months ago

It fully depends on how Sierra implemented it. It's just a global variable. They can do whatever they want with it.

Oh wait, I see what it's doing.
lsg 58h -> loads global 58h onto stack
pToa ticks -> puts ticks (parameter) into acc
sub -> subtracts acc from value on stack ( global 58h minus parameter )
push -> pushes acc onto stack again
ldi 00 -> set acc to 0
gt? -> stack is greater than acc? (bigger than 0)
bnt -> if not true, then jump

I think it should be possible to optimize that comparison and use way less bytes.

comment:39 by eriktorbjorn, 3 months ago

Just to clarify, if we don't want to rely on gGameTime working the same way in the English version as in the non-English version, my aim was to rewrite

	(method (doit)
		(if (> (GetTime) ticks)
			(self dispose: disposeWhenDone)
			(return)
		)
		(if eyes
			(self cycle: eyes)
		)
		(if mouth
			(self cycle: mouth)
		)
	)

into

	(method (doit)
		(if (> (- (GetTime) ticks) 0)
			(self dispose: disposeWhenDone)
			(return)
		)
		(if eyes
			(self cycle: eyes)
		)
		(if mouth
			(self cycle: mouth)
		)
	)

making it a hybrid of the two scripts. There's already 3 bytes that can be saved by using the shorter form of the bnt instruction, but m-kiewitz suggested it may be possible to optimize even the earlier parts of the script. If so, that should simplify the patch and I might give that another try. But I read SCI byte code much the same way as I read German: slowly, often incorrectly, and my vocabulary is vanishingly small.

I guess we should be thankful Sierra didn't add an optimization pass to their compiler!

by eriktorbjorn, 3 months ago

Attachment: lsl5-patch-v2.txt added

comment:40 by eriktorbjorn, 3 months ago

I've attached a second attempt at the patch. I thought the initial push0 wasn't related to the subsequent callk, but I guess it is?

So I had to shave off yet another byte by using a jmp to some common code.

It seems to be working, but I should stress that this is very much just intended as a proof-of-concept. It's not intended for merging!

This is what my patched version of the script looks like:

0032:064b: 0x76,                                // push0                       ; species
0032:064c: 0x43, 0x42, 0x00,                    // callk        GetTime[42],   00
0032:064f: 0x36,                                // push
0032:0650: 0x63, 0x5c,                          // pToa         ticks[5c]
0032:0652: 0x04,                                // sub
0032:0653: 0x36,                                // push
0032:0654: 0x35, 0x00,                          // ldi          00
0032:0656: 0x1e,                                // gt?
0032:0657: 0x31, 0x07,                          // bnt          07  [0660]
0032:0659: 0x39, 0x6c,                          // pushi        6c             ; 108, 'l', dispose
0032:065b: 0x78,                                // push1                       ; superClass
0032:065c: 0x67, 0x5e,                          // pTos         disposeWhenDone[5e]
0032:065e: 0x33, 0x14,                          // jmp          14  [0674]
0032:0660: 0x63, 0x58,                          // pToa         eyes[58]
0032:0662: 0x31, 0x07,                          // bnt          07  [066b]
0032:0664: 0x38, 0xb0, 0x00,                    // pushi        00b0           ; 176, cycle
0032:0667: 0x78,                                // push1                       ; superClass
0032:0668: 0x36,                                // push
0032:0669: 0x54, 0x06,                          // self         06
0032:066b: 0x63, 0x5a,                          // pToa         mouth[5a]
0032:066d: 0x31, 0x07,                          // bnt          07  [0676]
0032:066f: 0x38, 0xb0, 0x00,                    // pushi        00b0           ; 176, cycle
0032:0672: 0x78,                                // push1                       ; superClass
0032:0673: 0x36,                                // push
0032:0674: 0x54, 0x06,                          // self         06
0032:0676: 0x48,                                // ret

by eriktorbjorn, 3 months ago

Attachment: lsl5-patch-v3.txt added
Note: See TracTickets for help on using tickets.