Updated ScottFree

[angstsmurf]

I don't know if there is any interest in this, but I think that with a bit of polish, this could eventually become a nice update to the ScottFree interpreter.

It is a cut-down version of the Spatterlight ScottFree fork, which removes all support for German grammar, graphics, and ZX Spectrum and Commodore 64 versions, although there are traces of them left in the code. The TI-99/4A format support is still in there, but not very well tested.

New features:

• Restart, undo, ram save, ram load, transcript.
• Universal abbreviations: n, s, e, w, u, d for directions both as verbs and nouns, i for inventory, x for examine, l for look, and z for wait.
• Improved TAKE ALL and DROP ALL which are supposed to work in all games without breaking stuff.
• Commands can be chained together by commas or periods.
• Multiple nouns such as TAKE AXE AND FISH work.
• Changes in room descriptions will create pauses where appropriate, so that they are visible to the player. There should probably be a way to switch this off.
• Unicode support for non-ASCII input and output.
• Line wrapping in the upper window.

EDIT: Now also supports IT (refers to the last noun) and TAKE ALL BUT X AND Y

[cspiegel]

An improved ScottFree would be great! It's barely been touched since I ported it to Glk something like 8 years ago, and I think was pretty dormant before that.

A few notes here:

load_TI99_4a.c uses isascii() which is POSIX, not standard C. I made ScottFree purely C99 (well, I tried to) when I added Glk support, for maximum portability. Now, Gargoyle requires POSIX and the reason I added Glk support to ScottFree was to support Gargoyle, so it's not necessarily a bad thing to require POSIX. On the other hand, a custom isascii() would be trivial to write. Have you added other POSIX functionality? I see strings.h included in detectgame.c, but I'm not sure any functions from it are being used. If it's just isascii() then it may be worth keeping C99.

However, if you decide to stick with POSIX, you'll be able to remove bsd.c and switch to POSIX strcasecmp and strncasecmp. I added these from FreeBSD because ScottFree uses them, but they're not C99. If we go POSIX, we can remove the compatibility functions. In this case, you'll want to add POSIX to the definition in terps/CMakeLists.txt (right now it builds with a warning that isascii() is undeclared).

I did find a segfault with Pyramid of Doom (adv08.dat):

(gdb) bt
#0  __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:74
#1  0x00007ffff79e22b1 in __vfprintf_internal (s=s@entry=0x7fffffffb8a0, format=0x40f360 "Location of item %d: %d, \"%s\"\n", ap=0x7fffffffd9d8, mode_flags=0)
    at vfprintf-internal.c:1517
#2  0x00007ffff79e2ad6 in buffered_vfprintf (s=0x7ffff7b656a0 <_IO_2_1_stderr_>, format=format@entry=0x40f360 "Location of item %d: %d, \"%s\"\n",
    args=args@entry=0x7fffffffd9d8, mode_flags=mode_flags@entry=0) at vfprintf-internal.c:2261
#3  0x00007ffff79e1cd4 in __vfprintf_internal (s=<optimized out>, format=0x40f360 "Location of item %d: %d, \"%s\"\n", ap=ap@entry=0x7fffffffd9d8,
    mode_flags=mode_flags@entry=0) at vfprintf-internal.c:1236
#4  0x00007ffff79cdee9 in __fprintf (stream=<optimized out>, format=<optimized out>) at fprintf.c:32
#5  0x0000000000405940 in LoadDatabase (f=0x444e30, loud=1) at /home/chris/github/garglk/terps/scott/scott.c:540
#6  0x00000000004097e0 in detect_game (file_name=0x7fffffffe150 "/home/chris/Programming/scottfree/games/adv08.dat")
    at /home/chris/github/garglk/terps/scott/detectgame.c:143
#7  0x0000000000408d23 in glk_main () at /home/chris/github/garglk/terps/scott/scott.c:2015
#8  0x000000000040e239 in main (argc=2, argv=0x7fffffffdd18) at /home/chris/github/garglk/garglk/main.c:50
(gdb) frame 5
#5  0x0000000000405940 in LoadDatabase (f=0x444e30, loud=1) at /home/chris/github/garglk/terps/scott/scott.c:540
540                 fprintf(stderr, "Location of item %d: %d, \"%s\"\n", ct, ip->Location, Rooms[ip->Location].Text);
(gdb) print ip->Location
$3 = 255 '\377'
(gdb) print Rooms[ip->Location]
$4 = {Text = 0x371 <error: Cannot access memory at address 0x371>, Exits = {-120, 92, 0, 0, -26838, 27077}, Image = 61 '='}

I know nothing about the Scott Adams format, but to a naive eye it looks like 255 is a sentinel value?

[angstsmurf]

Thanks for taking a look!

I was afraid that there would be embarrassing bugs left, but that one was pretty bad. Not only was the "print lots and lots of debug info" switch on, every game that starts with the player carrying something would crash. Should be fixed now.

It sounds like we will eventually have to switch to POSIX for convenience, but let's try to stay C99 for now.

I added lots of header includes trying to make MinGW happy, but strings.h was probably unnecessary.

[cspiegel]

Presumably this would be better as const char *.

[cspiegel]

Well, I know I'm being annoying here, but there are lots of places that ought to be const throughout the code. Anything which is just string literals should be stored in const char*, and when those changes are made, warnings will radiate out to various other functions/variables, but it's simple enough to fix as you find them.

The Glk API, unfortunately, takes some char * values when it should take const char *, so casts will be required there, but throughout ScottFree things ought to be const except where they're modified.

[angstsmurf]

Definitely not annoying! Thanks the review. I hope I got all of them.

[cspiegel]

This is always one of my least-favorite review issues, because it seems so ridiculous, but when passing a char to the ctype functions, you need to cast it to unsigned char. Why? Because the C standard says so, that's why...

But it's really to accommodate systems which use macros plus lookup tables to speed up these functions. Solaris used to do this, and to my recollection NetBSD did as well. I'm not sure any modern systems still do, but they're allowed to. On Solaris, isspace used to look like this:

#define isspace(c) ((__ctype + 1)[c] & _S)

And __ctype is an array mapping unsigned char to a set of bits indicating which properties each character has. The + 1 is because EOF (-1 on Solaris and probably ever other existing system) has to be handled as well. So isspace(x) will index directly into the array, and if you have a signed char type, and the value is negative, you'll index backward into something that's totally unrelated.

So, long story short, all ctype functions are required to take “an int, the value of which shall be representable as an unsigned char or shall equal the macro EOF.” That's backwards compatibility for you.

[cspiegel]

Recommend making this a const char *.

[cspiegel]

When warnings are enabled with gcc (which they're not, for some reason, but should be: I must have just forgotten to add it), these pragmas cause warnings. They ought to be wrapped in tests to see if the compiler is clang.

[cspiegel]

This can be removed

[cspiegel]

@angstsmurf Do you feel this is in a pretty good state for a full review again? I'm happy to look it over and bring it in to Gargoyle if you're fine with its current state.

[cspiegel]

Since this returns an enum, it ought to use the enum names (ACT_FAILURE here, preusmably).

[angstsmurf]

Done!

[cspiegel]

This should be static since it's only used in this file.

In addition, it looks like action_line can be const.

[angstsmurf]

And done.

[angstsmurf]

I don't think there is a lot left to be done here except testing games, unless you have more comments. I suppose I could add some kind of script reading mechanism to make testing easier.

Also, the Mysterious Adventures (and perhaps others) need the PREHISTORIC_LAMP setting that destroys the lamp when it runs out, and there is no way to set this in Gargoyle. This problem exists in the current ScottFree in master as well. Perhaps I could just calculate checksums and autodetect these games.

[angstsmurf]

Now the lamp should work correctly in the Mysterious Adventures.

I also implemented a way to read command scripts, (#SCRIPT LOAD) and have played through The Golden Baton, Arrow of Death part 1, Ten Little Indians, Waxworks, Secret Mission, Voodoo Castle, Strange Odyssey, Mystery Fun House, Ghost Town and The Golden Voyage to completion.

I'd say this is ready to go, but more testing wouldn't hurt.

[cspiegel]

Sorry for the long delay, but I'd say it's high time to get this in!

A couple considerations:

1. I think it'd probably be good to have this in a separate upstream repository, so it can be properly shared by both Spatterlight and Gargoyle (and anybody else interested). I know that's making a bit more work for you: is it something you're willing to do? If so, we can then merge it in a single commit as with other interpreter updates.
2. Does this have 100% the same behavior as the original scottfree interpreter for games that it (the original) supports? I'm just wondering if there's a reason to keep the old interpreter around (disabled by default) in case there are games it's needed for. If not, then a wholesale replacement is fine.

[angstsmurf]

1. I think it'd probably be good to have this in a separate upstream repository, so it can be properly shared by both Spatterlight and Gargoyle (and anybody else interested).

The current ScottFree code in Spatterlight is very different from this. The major part of it is support for the ZX Spectrum and Commodore 64 cassette versions released by Adventure International UK, with graphics and animations, none of which is included here. The point of this pull request was to start small with a subset that was easier to review, leaving out all the more hacky and controversial stuff, such as the 6502 emulator used to decompress the Commodore 64 versions, and then cleaning up and adding the rest later.

By the way, someone just ported the complete Spatterlight ScottFree interpreter, 6502 emulator, German grammar and all, to C++ and the ScummVM framework. It might even make more sense to port that to Gargoyle instead, as some cruft was removed in the porting process, but that would require a bit of work.

None of this precludes making a separate repository out of this pull request, of course.

2. Does this have 100% the same behavior as the original scottfree interpreter for games that it (the original) supports?

Well, yes and no. The parser works a bit differently, but if you enter the exact same commands the output should be identical.

[cspiegel]

The current ScottFree code in Spatterlight is very different from this. The major part of it is support for the ZX Spectrum and Commodore 64 cassette versions released by Adventure International UK, with graphics and animations, none of which is included here. The point of this pull request was to start small with a subset that was easier to review, leaving out all the more hacky and controversial stuff, such as the 6502 emulator used to decompress the Commodore 64 versions, and then cleaning up and adding the rest later.

Ok, I see.

By the way, someone just ported the complete Spatterlight ScottFree interpreter, 6502 emulator, German grammar and all, to C++ and the ScummVM framework. It might even make more sense to port that to Gargoyle instead, as some cruft was removed in the porting process, but that would require a bit of work.

I've looked at ScummVM's Glk implementation before, and they really hack up the interpreters to fit into their model. To port it over we'd need a compatibility layer, at least providing the common routines they use (Common::Array, etc). Maybe that's worth it, maybe not... and it looks like they at least hook into the Scumm VM save system, which would have to be adapted.

Offhand I think it'd be easier to get the Spatterlight code extracted and usable everywhere, but I'll try to find some time to see just how difficult it'd be to use the ScummVM code with a shim layer. For now, I'm not against a separate "in between" version as represented by this pull request.

None of this precludes making a separate repository out of this pull request, of course.

I do think this is the right approach: I'd prefer for Gargoyle to not be the "official" source for any interpreters. At the moment the Scott Adams interpreter is from https://github.com/cspiegel/scottfree-glk, so it has an official-esque upstream, although it's just my straight port of the original ScottFree to Glk. I'm slowly trying to decouple Gargoyle from the interpreters so that it's easier to see Gargoyle's modifications, and to upgrade interpreters when necessary. A new upstream Scott Adams interpreter would make things a lot cleaner, and I'd rather keep my scottfree-glk repo just the original 1.14 ported to Glk, with no embellishments.

If you'd prefer I can create a repository under the garglk organization to hold this version of the interpreter, and features from Spatterlight can slowly be added to it, then upstreamed here. If you're in favor of that I can do the work of shifting this branch over as the initial commit there.

Does this have 100% the same behavior as the original scottfree interpreter for games that it (the original) supports?

Well, yes and no. The parser works a bit differently, but if you enter the exact same commands the output should be identical.

OK, that's reasonable enough to not need two interpreters.

[angstsmurf]

I'm closing this now. It's too out of sync with the Spatterlight version. I made a fork here with all the three Scott Adams-related interpreters (ScottFree, TaylorMade and Plus) running in Gargoyle:

https://github.com/angstsmurf/garglk/tree/saga

Seems to work fine apart from locking up on certain animations. And I don't know how to get the AppImage workflow to build.