Community

SCI Programming => SCI Syntax Help => Topic started by: Doan Sephim on July 05, 2020, 08:41:08 PM

Title: Old SCI0 Template Save Bug
Post by: Doan Sephim on July 05, 2020, 08:41:08 PM
While patching the old project (Betrayed Alliance book 1) I've run into a Save issue that I have also found in all the old fan games I've seen. If you run out of save spots, and save over a different file, sometimes I've experienced restoring that file only to discover that the new save did not overwrite the old.

I looked at the code for this, but the problem wasn't obvious to me.
Code: [Select]
(method (save)
(var strDescBuf[20], gameNum, oldCursor, hSound)
        Load(rsFONT gSaveRestoreFont)
Load(rsCURSOR gLoadingCursor)

= oldCursor (self:setCursor(gNormalCursor))
= hSound (Sound:pause(1))
(if(GetSaveDisk(TRUE))
(if(gPrintDlg)
(send gPrintDlg:dispose())
)
= gameNum (Save:doit(@strDescBuf))
(if(<> gameNum -1)
= oldCursor (self:setCursor(gLoadingCursor 1))
(if(not SaveGame(objectName gameNum @strDescBuf gVersion))
Print(
"Your save game disk is full. You must either "+
"use another disk or save over an existing saved game."
#font 0
#button "OK" 1
)
)
(self:setCursor(oldCursor HaveMouse()))
)
  GetSaveDisk(FALSE)
)
  (Sound:pause(hSound))
)
Any thoughts?
Also, I know no one still uses the old template, and I am moving on from it myself for the next project. So these problems I'm experiencing with the old template will soon be no more!
Title: Re: Old SCI0 Template Save Bug
Post by: lskovlun on July 06, 2020, 06:13:20 AM
I don't think the problem in in this procedure. It just calls save:doit to do the heavy UI lifting, and then SaveGame (a kernel call) to do the actual saving.
Title: Re: Old SCI0 Template Save Bug
Post by: Doan Sephim on July 06, 2020, 01:57:29 PM
I captured the bug on video in a playthrough of jummybummy 2
 here's the clip (https://youtu.be/4WynY7C50cE)
Save is at 5:40 and death at 7:10 then restore doesn't load properly.

Thanks the the help locating the problem Iskovlun. I looked and am still looking. Will update if I find anything
Title: Re: Old SCI0 Template Save Bug
Post by: Doan Sephim on July 08, 2020, 10:26:41 PM
I ran some tests to see if I could replicate this issue and here's what I found:

If you have your max 20 saves anytime you replace a save over a save file that is not the most recent, it seems to fail to save.

If you are replacing the top save file (the most recent), the save seems to work as normal.

So far, I have not been able to locate the the problem in the code, which isn't surprising to me as programming is my weakest area. I've focused my search in the Game Script, but haven't turned up anything except the save method I posted earlier. I suppose there could be a problem in the restore method, not the save...
Title: Re: Old SCI0 Template Save Bug
Post by: lskovlun on July 09, 2020, 06:27:55 AM
@EricOakford had some problems with the saving code when he was doing the SCI01 port a while back. I don't know what came of it, but I think it was some sort of corruption bug. Another corruption bug (or perhaps the same one) came up with his port of sciAudio recently. At least some of that was due to a parsing bug in Companion (didn't like backslashes), which @Kawa fixed.
Title: Re: Old SCI0 Template Save Bug
Post by: Kawa on July 09, 2020, 06:32:52 AM
If the save/load scripts don't have path separators in any of their strings, they're not affected by what I fixed.
Title: Re: Old SCI0 Template Save Bug
Post by: Doan Sephim on July 09, 2020, 01:47:11 PM
If the save/load scripts don't have path separators in any of their strings, they're not affected by what I fixed.

Which fix is this? Will I be able to fit it with the old template?

I'd really like to resolve this as I think it is a big issue for the user. Players are going to want to save over earlier files, not the most recent.
Title: Re: Old SCI0 Template Save Bug
Post by: lskovlun on July 09, 2020, 02:18:31 PM
@Kawa and I were referring to a fix for escape sequences and the use of '\\' for a path separator in particular. sciAudio wants to create a file in a subdirectory of the main game dir, and therefore failed. It manifested differently in SCI0 somehow.

The thread is here (http://sciprogramming.com/community/index.php?topic=1898.0).

The second issue, which may be more relevant, took some digging to find, but it's here (http://sciprogramming.com/community/index.php?topic=1798.30). It seems it was never resolved properly, only worked around.
Title: Re: Old SCI0 Template Save Bug
Post by: Doan Sephim on July 09, 2020, 10:33:21 PM
Thanks for the links. So if I'm understanding correctly, it's not a problem in the template code at all. In that case, I'm even more at a loss! That's frustrating!
Title: Re: Old SCI0 Template Save Bug
Post by: gumby on July 12, 2020, 08:12:01 AM
I've been bitten by this before as well.  EricOakford - you managed a workaround for SCI01, would that (or a similar approach) work for SCI0?

If not, a potential workaround could be to check the number of save games before launching the save dialog and if it's more than 20, delete the oldest one (or better yet, move it to an 'archive' directory).  Admittedly, not a great user experience but it might work.
Title: Re: Old SCI0 Template Save Bug
Post by: Doan Sephim on July 12, 2020, 11:09:11 AM
If not, a potential workaround could be to check the number of save games before launching the save dialog and if it's more than 20, delete the oldest one (or better yet, move it to an 'archive' directory).  Admittedly, not a great user experience but it might work.
It'd be a better user experience than the game not saving at all! And while I like a lot of saves, I can't think of a single time I open one up more than 3 or so down.
Title: Re: Old SCI0 Template Save Bug
Post by: Collector on July 12, 2020, 02:06:09 PM
Or automatically create a new "SAVE" subdirectory appended with an incremental number. I would not like a feature that automatically deleted any save games, even if the oldest.
Title: Re: Old SCI0 Template Save Bug
Post by: EricOakford on July 12, 2020, 09:56:42 PM
I've been bitten by this before as well.  EricOakford - you managed a workaround for SCI01, would that (or a similar approach) work for SCI0?

No workaround here - the old template's save bug may be related to a newer issue I had in the SCI01 template. Specifically, SRDialog:doit. It can't be decompiled and thus falls back to disassembly. Somewhere in there, something didn't disasemble correctly, resulting in the duplicate saves bug.

The newer templates shouldn't have this problem to begin with, as they use the original source scripts as a basis. With SCI01, all that was needed was changes for the new FileIO function.
Title: Re: Old SCI0 Template Save Bug
Post by: Doan Sephim on July 12, 2020, 10:29:22 PM
No workaround here
Sounds like I would need something like Gumby and Collector are suggesting then. When at 20 saves, to either archive the other saves to a new directory, or force new saves to a new directory.
Title: Re: Old SCI0 Template Save Bug
Post by: troflip on July 13, 2020, 02:06:59 PM
I took a quick look at this.

It seems like even if you save over the most recent, it doesn't work properly. For instance,
- I save 20 games, A through T
- Then I try to save over T and give it the same name
- The result is that it deletes game A, and now there are two game T's.


Title: Re: Old SCI0 Template Save Bug
Post by: troflip on July 13, 2020, 02:24:48 PM
Back to the original issue:

After saving 20 games, A-T, the beginning of gamename.dir will look like:
13 00 73 0a 12 00 72 0a      slot 19 (T), slot 18 (S), ...
That's basically, slot number (in hex) followed by string that appears to be 0a-terminated.

Then I save over S (2nd in the list). And I get:

12 00 72 0a 13 00 73 0a      slot 18 (S), slot 19 (T), ...

Looks great, exactly as it should look.
The time stamp on the file shows gamename.dir and gamename.018 written to, which is correct.

But now if I delete gamename.018, I can still load S (but actually it's not S, see below)

However, if I delete gamename.000, I can no longer load S. Which means loading S is actually trying to load the wrong save.

So what's going on: S is being saved properly (to gamename.018), but when we load we are loading gamename.000. This is despite gamename.dir linking S to slot 18. This isn't some kind of memory corruption, it persists even if I close and reopen the game.
If I rename gamename.000 to gamename.018, then I can load the "missing" S save.

So somehow, it appears the save games get into a state where the first slot number in gamename.dir is always getting treated as 0 by the game, despite the data in gamename.dir.

Title: Re: Old SCI0 Template Save Bug
Post by: troflip on July 13, 2020, 02:58:22 PM
It looks like the values returned by the GetSaveFiles kernel are wrong (or at least don't match the files). It always has 0 in the first slot. These are the contents of strPtrs just after the GetSaveFiles call in SRDialog:init.

Title: Re: Old SCI0 Template Save Bug
Post by: Doan Sephim on July 13, 2020, 03:20:40 PM
Troflip, you're investigative skills are top-notch! Very impressive.

I'm just glad the problem is coming into focus!
Title: Re: Old SCI0 Template Save Bug
Post by: Collector on July 13, 2020, 07:41:19 PM
Specifically, SRDialog:doit. It can't be decompiled and thus falls back to disassembly. Somewhere in there, something didn't disasemble correctly, resulting in the duplicate saves bug.

Wonder if Omer might have the original SCI0/SCI01 source scripts for this?
Title: Re: Old SCI0 Template Save Bug
Post by: gumby on July 13, 2020, 08:06:46 PM
Troflip, do you think it might be possible to hack around it with some custom post-processing that would (re)create the .dir file with the correct values?
Title: Re: Old SCI0 Template Save Bug
Post by: Collector on July 13, 2020, 10:13:31 PM
Sierra had a tool create a new catalog file from all of the save games in a folder. http://sierrahelp.com/Utilities/Misc/SierraSavedGameCatalogFixer.html
Title: Re: Old SCI0 Template Save Bug
Post by: troflip on July 14, 2020, 12:14:18 AM
Troflip, do you think it might be possible to hack around it with some custom post-processing that would (re)create the .dir file with the correct values?

I think you could hack around it by limiting the save game count to 19 instead of 20.

I haven't tried it, but if you change this to a 19, it should limit the save game count to 19, and then I think the bug doesn't repro:

Code: [Select]
(procedure (CheckForFreeSpace)
(if (< local2 20) (CheckFreeSpace gSaveDirPtr))
)
Title: Re: Old SCI0 Template Save Bug
Post by: OmerMor on July 14, 2020, 02:12:39 AM
Specifically, SRDialog:doit. It can't be decompiled and thus falls back to disassembly. Somewhere in there, something didn't disasemble correctly, resulting in the duplicate saves bug.

Wonder if Omer might have the original SCI0/SCI01 source scripts for this?

I published my SCI0 scripts here: https://github.com/OmerMor/SCI0 (https://github.com/OmerMor/SCI0)
Unfortunately I don't have any SCI01 scripts.
I do have some SCI1 versions of SAVE.SC which I haven't published yet.
I'll attach all my versions here.
Title: Re: Old SCI0 Template Save Bug
Post by: lskovlun on July 14, 2020, 06:52:25 AM
I've made a test harness for SaveGame/GetSaveFiles. The 'prepare' menu option will make 20 saved games, and the two other options print the information returned by GetSaveFiles. After setting the stage in this manner, you can use the ordinary Save/Restore functionality to stress-test the related code. You just dump the attached file in a newly created game, it should be self-contained.
Title: Re: Old SCI0 Template Save Bug
Post by: EricOakford on July 14, 2020, 06:29:52 PM
I published my SCI0 scripts here: https://github.com/OmerMor/SCI0 (https://github.com/OmerMor/SCI0)
Unfortunately I don't have any SCI01 scripts.
I do have some SCI1 versions of SAVE.SC which I haven't published yet.
I'll attach all my versions here.

Just updated my SCI1.0 decompilations and template. Now saving works in all of my templates, and two disassembly blocks are history!

The SCI01 Save script is pretty much the same as the SCI0 one, just with the file kernel functions merged into one, so we're not missing much there.
Title: Re: Old SCI0 Template Save Bug
Post by: Collector on July 14, 2020, 08:25:41 PM
Are there any other disassembly blocks left in the templates?
Title: Re: Old SCI0 Template Save Bug
Post by: EricOakford on July 14, 2020, 09:47:49 PM
Are there any other disassembly blocks left in the templates?

SCI0, SCI01, and SCI11 have no disassembly blocks at all.

SCI10 only has three - in Logger, PolyEdit, and Talker. I can resolve Talker and thus allow for talking portraits. PolyEdit and Logger? Probably not needed.
Title: Re: Old SCI0 Template Save Bug
Post by: Kawa on July 15, 2020, 06:09:00 AM
Very much not needed, yeah, much like most of the development tools. Now, debug tools...
Title: Re: Old SCI0 Template Save Bug
Post by: OmerMor on July 16, 2020, 11:59:11 AM
Are there any other disassembly blocks left in the templates?

SCI0, SCI01, and SCI11 have no disassembly blocks at all.

SCI10 only has three - in Logger, PolyEdit, and Talker. I can resolve Talker and thus allow for talking portraits. PolyEdit and Logger? Probably not needed.

Attached are all my Logger/PolyEdit/Talker scripts.


Title: Re: Old SCI0 Template Save Bug
Post by: Doan Sephim on July 16, 2020, 10:54:28 PM
I'm a little confused. It got really technical and I got lost. Did we find a work around using 19 saves instead of 20, or did we figure something else out? Or did we figure nothing out?
Title: Re: Old SCI0 Template Save Bug
Post by: troflip on July 17, 2020, 12:15:39 AM
I think the thread took a turn and stopped being about the save bug  :P
Title: Re: Old SCI0 Template Save Bug
Post by: lskovlun on July 18, 2020, 02:37:50 PM
I found the bug! It is a buffer overflow in the script code.
We have this line:
Code: [Select]
(method (doit strDescription &tmp temp0 temp1 temp2 temp3 [temp4 360] [temp365 20] [temp386 20])
Look closely at the temp4 variable (which contains the description strings and comes just before the index numbers). It is sized as 360 (20*18 words), but this is not quite enough. Eric's authentic file has this define:
Code: [Select]
(define BUFFERSIZE 361) ;(define BUFFERSIZE (+ (* MAXGAMES COMMENTBUFF) 1))If you change the size of that temp4 array to be just a bit larger (361 words), you'll get the intended result.
Title: Re: Old SCI0 Template Save Bug
Post by: Doan Sephim on July 18, 2020, 03:07:37 PM
Fantastic! I did the suggested change and it seems to work perfectly!

Awesome catch! I wouldn't have seen that in a million years.

Quick question, is there any potential unintended consequences or is this a fairly clean fix. I get that it was an overflow issue, but my comprehension of how and why is not so good.
Title: Re: Old SCI0 Template Save Bug
Post by: Kawa on July 18, 2020, 03:14:38 PM
It's a clean fix and holy shit that's a sneaky one haha
Title: Re: Old SCI0 Template Save Bug
Post by: lskovlun on July 18, 2020, 03:35:06 PM
It's a clean fix and holy shit that's a sneaky one haha
It's sneaky and goes right back to SCI Studio. That means this is handwritten, not decompiled code. It was rewritten from an ASM block.
Title: Re: Old SCI0 Template Save Bug
Post by: troflip on July 18, 2020, 04:45:24 PM
Wow! You should feel good about finding that one!  :)
Title: Re: Old SCI0 Template Save Bug
Post by: EricOakford on July 18, 2020, 06:13:31 PM
It's a clean fix and holy shit that's a sneaky one haha
It's sneaky and goes right back to SCI Studio. That means this is handwritten, not decompiled code. It was rewritten from an ASM block.

Naturally, when you only have disassembled code for reference, mistakes can be made. With original source for reference, these can be discovered and fixed.
Title: Re: Old SCI0 Template Save Bug
Post by: Doan Sephim on July 18, 2020, 06:20:46 PM
You guys are awesome! Thanks for looking into this for me. I'm looking forward to using SCI01 moving forward, so I'm sure these problems will be a thing of the past.
Title: Re: Old SCI0 Template Save Bug
Post by: lskovlun on July 19, 2020, 11:51:51 AM
I have to thank Phil for coming up with a different way to test this. It was really the discrepancy between the two approaches that led me on the right track.
Title: Re: Old SCI0 Template Save Bug
Post by: MusicallyInspired on July 19, 2020, 04:21:36 PM
Awesome!!