#14632 closed defect (fixed)

SCI: LSL7 crash at pool entrance; too many screen items

Reported by: sluicebox Owned by: sluicebox
Priority: normal Component: Engine: SCI
Version: Keywords: original
Cc: Game: Leisure Suit Larry 7

Description

LSL7 crashes with an assertion failure at the pool entrance (room 301) when drawing the popup menu while having a lot of inventory items. It also depends on how many things are on the screen, like random people in the background and extra menu items. This is a bug in the original, but it's a hard-coded engine limit that this game's design exceeds.

This bug was posted to the GOG forum in 2019: https://www.gog.com/forum/leisure_suit_larry_series/missing_line

To reproduce this, it's easiest to use LSL7's built-in debugger to get lots of inventory items:

  1. Create a file named CLASSES in the LSL7 game directory (enables debugger)
  2. Start a new game
  3. Teleport to room 301 (ALT+T, or use the scummvm debugger)
  4. Give yourself many inventory items with LSL7 debugger (ALT+I)
  5. Wait for people to appear on the screen
  6. Click on the red pants and select "Use"
  7. Mouse around the other menu items
  8. Assertion fails depending on how many background people there are

SCI32 used a 250 element fixed array of screen items in its drawing code. LSL7 uses a lot of screen items for the popup menu and this room uses over 100. The more inventory items, the more internal screen items, and it can exceed the limit. Our implementation is the same, so it has the same limit.

The class that causes this is Sci::StablePointerArray. We could increase the array size but it's not clear to me what it should be, since I got LSL7 to exceed 300, and I'd rather not increase that buffer everywhere for edge cases. I also suspect this can happen in other rooms. I would rather fix this with a dynamic array, but it's a template class used in several places in the high performance SCI32 drawing code, and it's constantly being constructed and destroyed. There's also another template class that's an array of these. So switching to a dynamic array would take some analysis and restructuring of all the callers to not take a performance hit. (And it would be a noticeable one, because this room already has significant drawing delays in debug builds.)

Attached is a save game from the GOG version: English 1.01. Click on the red pants, mouse over "Use" and mouse over "Other" to crash.

Attachments (2)

lsl7-gog.006 (74.8 KB ) - added by sluicebox 14 months ago.
bug.jpg (384.3 KB ) - added by m-kiewitz 14 months ago.

Download all attachments as: .zip

Change History (5)

by sluicebox, 14 months ago

Attachment: lsl7-gog.006 added

comment:1 by m-kiewitz, 14 months ago

Is there a reason why we keep that fixed limit?
Why not do a dynamic limit instead?
I don't really see a downside making it dynamic (and thus only limited to memory)

comment:2 by m-kiewitz, 14 months ago

for example our VM isn't limited to 64k either, Sierra's was (SCI16, not sure about SCI32) and especially games like Quest for Glory 3 crashed a lot when that limit was reached (memory fragmentation etc.), but that works fine in our VM.

by m-kiewitz, 14 months ago

Attachment: bug.jpg added

comment:3 by bluegr, 14 months ago

Owner: set to sluicebox
Resolution: fixed
Status: newclosed

Fixed by sluicebox in the following PR:
https://github.com/scummvm/scummvm/pull/5340

Thanks for your work on this! Closing

Note: See TracTickets for help on using tickets.