SCI Companion V3 - alpha build notes/bugs/feature requests

[troflip]

Hmm... I was able to get LB1 script 414 to compile by manually editing the vocab 996 file (attached) by putting invalid scripts in place of the valid 999 ones.

I did exported vocab.996 as a patch file, edited it manually in a hex editor, saved, then re-imported it (make sure to not to save the patch file in the game's directory).

Other entries probably need the same attention, but at least this let me compile that particular script with a fresh decompile of LB. Dunno if the game barfs on it or not...

[edit:] also, the game re-writes the class table on compile, so this might not be a good permanent solution.

[Charles]

Ok, it looks like the LB1 issue (and presumably KQ4) only happens right after a decompile, right?

No, I should have been clearer. That's how I first got the crash. I could then immediately re-open SCICompanion, hover over the lines I specified, and watch it crash again.

So let me see if I understand this: each script keeps a lookup table of index numbers for each procedure/method in it. SCICompanion will automatically generate that lookup table based on the order each procedure appears (a perfectly logical approach). The Classic Sierra compiler generated that lookup table based on some other reference (likely some sort of text file), the consequence of which is that lookup indexes will not necessarily be in order.

Any script (A) that includes a reference to another script's (B) procedures does so by referencing (B)'s lookup table index. So my recompiled 414 script failed to work because it was referencing procedure index 5 in SCICompanion's compiled 999.scr, when it needed to reference index 6 in Classic Sierra compiled 999.scr.

I'm happy for you to add automagic anywhere you want, but I'm not one for waiting. I like your idea of adding in a dummy procedure in the 3rd spot so the other indexes realign. I suspect I wouldn't need to add the other two dummy's to padd out the end of the table, but 'ellifiknow. As for compatibility with SCI Studio... I'm new to this scene, so my opinion shouldn't have that much weight, but honestly my impression has always been that there was minimal serious development on SCI Studio anyway due to the whole chicken/egg thing with VGA support.
 
I wonder how prevalent missing procedures are in Sierra games in general... I've already seen Colonel's Bequest has a couple. Heck, there's a whole missing script file referenced in there (984).

Soon as I get the chance I'll try the dummy filler method. I'll have to compile 999 for SCICompanion's benefit, but I shouldn't have to include that if I wanted to patch a vanilla copy of LB1, should I?

[troflip]

No, I should have been clearer. That's how I first got the crash. I could then immediately re-open SCICompanion, hover over the lines I specified, and watch it crash again.

Hmm... maybe I haven't fixed the issue then. Did you decompile all the scripts in the game? I can see the issue still happening if User.sc wasn't decompiled, but if you decompiled everything and it still is happening, then I might not have the right repro... hmm...

So let me see if I understand this: each script keeps a lookup table of index numbers for each procedure/method in it. SCICompanion will automatically generate that lookup table based on the order each procedure appears (a perfectly logical approach). The Classic Sierra compiler generated that lookup table based on some other reference (likely some sort of text file), the consequence of which is that lookup indexes will not necessarily be in order.

Any script (A) that includes a reference to another script's (B) procedures does so by referencing (B)'s lookup table index. So my recompiled 414 script failed to work because it was referencing procedure index 5 in SCICompanion's compiled 999.scr, when it needed to reference index 6 in Classic Sierra compiled 999.scr.

That's the general gist of it, but this is specific to species indices - it has nothing to do with procedures or methods. Each class (or species) has a number associated with it.

When SCI Companion encounters this line:

Code: [Select]

(identify state: 4 seconds: 0 cycles: 0)

It looks and sees that (defined in the same file, script 414) "identify" is an instance of the "Script" class. It then tries to validate that state, seconds and cycles are all properties or methods on Script.

To do so, it needs to find the species number of the class. It looks at the included .sco files (which are generated by the compiler or decompiler when compiling or decompiling a script). It sees that "Script" is the 5th class in Obj.sco (for script resource 999). It then looks at vocab.996, which tells us that the 5th class defined in script 999 has species index 5. So the compiler goes "ok, I need to look up properties on species index 5".

Then it goes back to the .sco file and says "ok, give me info about the class that says it's species 5". And in the .sco file, that's EventHandler that says it's species 5 (which is correct). So we compare against the methods and properties of EventHandler (instead of "Script") and fail.

Listing the details of it out like this make me think I can fix how we obtain the species index (italicized above). The .sco file actually has information about which species each class is already inside it - I don't need to touch the vocab.996 resource. Need to think about this a bit more. At any rate, this would require a fix in SCI Companion, so it's of no use to you now.

I wonder how prevalent missing procedures are in Sierra games in general... I've already seen Colonel's Bequest has a couple. Heck, there's a whole missing script file referenced in there (984).

Well I've decompiled a lot of games, and this is the first I've seen of it that I can remember. Although I think there are still some mysterious compile errors in decompiled games that I haven't had time to look into - so perhaps this was the cause of those too.

Soon as I get the chance I'll try the dummy filler method. I'll have to compile 999 for SCICompanion's benefit, but I shouldn't have to include that if I wanted to patch a vanilla copy of LB1, should I?

Correct, it shouldn't need to be included. Compiling 999 should generate an 999.sco and a vocab.996 resource that makes everything consistent for the compiler - i.e. compiling other scripts that reference 999 will now reference valid species numbers.


btw, I hope you're not making a patch to remove the fingerprint "copy protection".

[MusicallyInspired]

I've bypassed copy protection in my own Sierra games for my own personal use (because I just don't want to dig out my manuals) by a fairly simple means, but when I went to attempt the same with LB1, I was surprised to see just how complex they made it. Now, I didn't have the Troflip's decompiler like we do now, so I was just looking at disassemblies and the copy protection, from what I remember, was so convoluted between different scripts that it was impossible to do it they way I had easily done it in past games. I never bothered to look at it decompiled to see just how it was done, but mad props to Sierra for convoluting it like that. Reminds me of the store Scott Murphy told once on how Ken Williams added the "kiss sarien" bonus point to SQ1 and he couldn't figure out how to remove it from the game because Ken had tied it into a bunch of other things in the game and removing it broke everything. So he had to leave it in lol. He said it as a compliment to his programming skills.

[Charles]

Did you decompile all the scripts in the game?

Yup. Decompiled them all a couple times each.  Which script number is generally User.sc?
(And just to double-check, SCICompanion's About Box says it's version 3.0.1.7, which I believe is still the latest official release...)

That's the general gist of it, but this is specific to species indices - it has nothing to do with procedures or methods. Each class (or species) has a number associated with it.

Ah, I was mixing up classes and procedures. So the fact that 999 itself wasn't compiling was a bit of a red herring.

I wonder how prevalent missing procedures are in Sierra games in general... I've already seen Colonel's Bequest has a couple. Heck, there's a whole missing script file referenced in there (984).

Well I've decompiled a lot of games, and this is the first I've seen of it that I can remember. Although I think there are still some mysterious compile errors in decompiled games that I haven't had time to look into - so perhaps this was the cause of those too.

I don't know if it decompiled properly or not, but it sure looked like it decompiled properly. Script 996 has the reference to phantom script 984, and even references a specific procedure from it:

Code: [Select]

(proc984_0 alterEgo gSFeatures gCast gFeatures)It's funny, LB1's showing several areas that I would consider to be bad coding practice.  This if statement that leads to a procedure call that doesn't exist, for one. For another I believe there's a 1 in 600 chance of the game crashing on startup at the copy protection screen.

Correct, it shouldn't need to be included.

And it did. The compiled script now behaves exactly as expected.

btw, I hope you're not making a patch to remove the fingerprint "copy protection".

I'm certainly not doing any such thing for distribution purposes.  At some point in the future I want to create my own game in the vein of QFG (There's been a bit of a boon of those types of games lately, Quest for Infamy, Heroine's Quest, Betrayed Alliance), but for now I'm satisfying myself with decompiling/commenting existing games, making minor bug fixes, then making slight mods to existing games. But mostly just soaking in the SCI coding structure.

[troflip]

Yup. Decompiled them all a couple times each.  Which script number is generally User.sc?
(And just to double-check, SCICompanion's About Box says it's version 3.0.1.7, which I believe is still the latest official release...)

Ah, so you didn't do a SetFilenames to get good file names? (don't have src handy at the moment, don't know which one is User.sc). Shouldn't matter though. I can give you a build to test to see if the crash goes away.

Ah, I was mixing up classes and procedures. So the fact that 999 itself wasn't compiling was a bit of a red herring.

Procedures do have a bit of the same issue, but I added the "public" exports section to assign fixed export number for them, so they are stable and don't change when you add/remove public procs. There's no master table to get out of sync with though, since calls to public procs are identified by script number + export index (as opposed to some global index)

It's funny, LB1's showing several areas that I would consider to be bad coding practice.  This if statement that leads to a procedure call that doesn't exist, for one. For another I believe there's a 1 in 600 chance of the game crashing on startup at the copy protection screen.

Well, they probably just removed that script then forgot it was accessed in an if statement that (apparently) never gets hit. Dunno if it's bad coding practice - maybe just the victim of a compiler that didn't treat this as an error.

[OmerMor]

I don't know if it decompiled properly or not, but it sure looked like it decompiled properly. Script 996 has the reference to phantom script 984, and even references a specific procedure from it:

Code: [Select]

(proc984_0 alterEgo gSFeatures gCast gFeatures)It's funny, LB1's showing several areas that I would consider to be bad coding practice.  This if statement that leads to a procedure call that doesn't exist, for one.

If you're interested in this missing script, here some information:
The original name for script #984 was SORTCOPY.SC.
It has one procedure in it:

Code: [Select]

(procedure (SortedAdd theOrigin theLists
      &tmp 
      i toList fromColl
      node dist obj ang
      invisible
     
      objMinFront distMinFront frontList  ;features ego can see on screen
      objMinOut distMinOut outList        ;   "      "   "   "  off   "
      objMinBack distMinBack  backList    ;   "     behind ego
     
     
     
   )
   ; ...
)

The original code that calls this procedure from User::said() is this:

Code: [Select]

    (if useSortedFeatures
      (SortedAdd alterEgo sortedFeatures cast features)
    else
      (sortedFeatures add: cast features)
    )


Here's some documentation on it's usage:

12/12/88 Pablo

   There is a new global variable called "egoBlindSpot" which should contain
   the size of ego's blind spot in degrees, ie. how many degrees from
   "straight behind" are not visible to ego. Default is zero (360 degree
   vision), recommended value is 90 (ego only sees ahead). If
   useSortedFeatures is TRUE then objects in ego's blind spot do not have
   their handleEvent method invoked on Said events.

The useSortedFeatures and egoBlindSpot globals were numbered 54 and 56 respectively.

I saw LB1 doesn't use this feature, so there was no need to include the SORTCOPY.SC script with the game.

[Charles]

Well, they probably just removed that script then forgot it was accessed in an if statement that (apparently) never gets hit. Dunno if it's bad coding practice - maybe just the victim of a compiler that didn't treat this as an error.

You're absolutely right. I'm making wild assumptions without knowing any of the facts.

I saw LB1 doesn't use this feature, so there was no need to include the SORTCOPY.SC script with the game.

And there ya go.

If you're interested in this missing script, here some information:

I'm very interested in this sort of stuff. The Sierra games of old have given me thousands of hours of entertainment over the years, at least, and I find it fascinating to peel back the layers or look behind the curtain, so to speak.  Thank you very much for sharing.

[MusicallyInspired]

Wow! A blind spot script! That'd be something interesting to play with...

[OmerMor]

If you're interested in this missing script, here some information:

I'm very interested in this sort of stuff. The Sierra games of old have given me thousands of hours of entertainment over the years, at least, and I find it fascinating to peel back the layers or look behind the curtain, so to speak.  Thank you very much for sharing.

I think you described pretty much everyone in this little community. 
And you're welcome.

[Charles]

*To do this audit, you can get the species index from the class table, and compare it to the one defined by the object in the script resource (Script->View Script Resource). The species of a class is property 0, the first entry right under the "properties:" header for each class. If you do this for script 999, you'll see the classes have species 0, 1, 3, 4, 5, 6, 7. Hmm, so actually just a single dummy class between Code and Collect should fix that particular issue up. Oh actually wait, no, you'll need to add 2 more dummy classes at the end, since we're trying to preserve the integrity of the class table. ugh, complicated.

I've taken another look at some other LB1 scripts, with an eye to the species order... am I reading this correctly? If I look at the Script Resource for Feature.sc (script 998), I see there are 7 classes and they start at index 24 (in hex). So does this mean using the "dummy" method of correctingit, I would have to insert 36 dummy classes before the "Feature" class?

[troflip]

No. Species are global numbers, not per-script. I don't have LB1 on hand since I'm not at home now, but looking at the SCI0 template game, Feature.sc is script 998. In the global class table (vocab.996), I see the following entries for script 998:

species 35: script 998
species 36: script 998
species 37: script 998
species 38: script 998
species 39: script 998
species 40: script 998
species 41: script 998

If the numbers you see in LB1 match the species numbes in the Features script, then you're fine.

If, though, you looked at the features script, and saw that it had 6 classes with species: 35, 36, 38, 39, 40 and 41 (decimal), then you'd notice that 37 is missing. So you'd need to add a dummy class between the class with species 36 and species 38.




[edit:] Just looked at the LB1 998 script, and it seems to correspond fine with the global class table, don't think you need to do anything.

[OmerMor]

I'd like to report what looks to me like a decompiler bug:
In Quest for Glory 3, script #23, Teller::doChild(), the disassembly is the following:

Code: [Select]

(method (doChild key &tmp index)
(asm
ldi      0
sat      index
code_0713:
+at      index  ; (++ index)
bnt      _end
pushi    2
pTos     keys
push    ; <== BUG HERE
calle    WordAt,  4
push   
lap      key
eq?     
bnt      code_073d
pushi    #stuffArray
pushi    2
pushi    2
pTos     arrays
lst      index
calle    WordAt,  4
push   
pushi    1
self     8
ldi      1
ret     
code_073d:
pushi    2
pTos     keys
lst      index
calle    WordAt,  4
push   
ldi      999
lt?     
jmp      code_0713
_end:
ldi      1
ret     
)
)
And the decompiled source is the following:

Code: [Select]

(method (doChild key &tmp index)
   (= index 0)
   (while (++ index)
      (if (== (WordAt keys (++ index)) key)  ; <== BUG HERE
         (self stuffArray: (WordAt arrays index) 1)
         (return 1)
      )
      (< (WordAt keys index) 999)
   )
   (return 1)
)
In the lines I marked, the script is calling WordAt with 2 parameters. The 2nd parameter is the accumulator (push), which has the value of the previously incremented index (+at index).
However, the decompiler copies the whole (++ index) expression as the 2nd parameter, which is incorrect, as the index is only incremented once.

[troflip]

It's possible I've already fixed this (are you using the official Companion release, or private build?). I'll check tonight. I'm waiting to release an official update until I've tackled one other decompiler bug involving chained operators (e.g. (< 3 4 5 6)).

[edit:] nope, not fixed. I'll add it to my bug list.

[Kawa]

wait what

I can understand (+ 1 2 3) and such but <?