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

[sluicebox]

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.

[m-kiewitz]

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)

[m-kiewitz]

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.

[bluegr]

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

Thanks for your work on this! Closing