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

[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.

[eriktorbjorn]

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:)
			)

[m-kiewitz]

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.

[m-kiewitz]

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.

Also I think I remember that games typically calculated the duration by themselves, depending on the length of the text.

[eriktorbjorn]

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.

[m-kiewitz]

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.

[eriktorbjorn]

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.

[m-kiewitz]

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

[m-kiewitz]

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

[eriktorbjorn]

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. :-)

[m-kiewitz]

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.

[eriktorbjorn]

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.)

[m-kiewitz]

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.

[eriktorbjorn]

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!

[eriktorbjorn]

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.)