Opened 14 months ago
Closed 13 months ago
#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:
- Create a file named CLASSES in the LSL7 game directory (enables debugger)
- Start a new game
- Teleport to room 301 (ALT+T, or use the scummvm debugger)
- Give yourself many inventory items with LSL7 debugger (ALT+I)
- Wait for people to appear on the screen
- Click on the red pants and select "Use"
- Mouse around the other menu items
- 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)
Change History (5)
by , 14 months ago
Attachment: | lsl7-gog.006 added |
---|
comment:1 by , 14 months ago
comment:2 by , 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 , 14 months ago
comment:3 by , 13 months ago
Owner: | set to |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Fixed by sluicebox in the following PR:
https://github.com/scummvm/scummvm/pull/5340
Thanks for your work on this! Closing
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)