Crash when operating pictures with high ID (High memory usage? PicPointerPatch?)

[elsemieni]

When someone wants to operate Pictures with very high ID, the game crashes. I know this is not a common use case (as using high picture ID means the game developer has a poor picture organization), but it's important to mention it as it's a corner case that Player is not handling i guess (maybe it's not too importante, but I think reporting it anyways counts).

Looks like PicPointerPatch feature (https://github.com/EasyRPG/Player/pull/1035) is involved in this. I will explain with a test case:

@> Control Variables: [0010] = 9999999 @> Show Picture: Variable [0010], 'test', (160,120), 50%, 50%, M6 (The variable operation is for reaching that value. Putting them directly via hacking the editor, or by editing the event by hand is also possible).

Then the following happen:

• 9999999 > 50000 , so PicPointerPatch feature enters to act.
• It tries to find the value of variable 9999999 - 50000 -> 9949999.
• Also it tries to find the value of 9949999 + 1 (for getting the suffix number for getting what image needs to load).

Expected behaivour: A Invalid read var[9949999] and Invalid read var[9950000] warning.

Current behaivour: It crashes.

It's important to mention that I tested it in Windows Player (master) in x86 and x64. In 32 it crashes, but in 64 the behaivour is just the expected one, but with a considerable memory consumption and lag (near 3GiB of RAM, so you will guess something is going out of control).

Stack trace:

std::bad_alloc Exception

>   KERNELBASE.dll!76bf9ab2()   Unknown
    KERNELBASE.dll! Unknown
    [External code] 
    Player.exe!Game_Interpreter::Update(bool reset_loop_count) Línea 406    C++
    Player.exe!Game_Map::UpdateForegroundEvents(MapUpdateAsyncContext & actx) Línea 1113    C++
    Player.exe!Game_Map::Update(MapUpdateAsyncContext & actx, bool is_preupdate) Línea 960  C++
    Player.exe!Scene_Map::UpdateStage1(MapUpdateAsyncContext actx) Línea 239    C++
    Player.exe!Scene_Map::Update() Línea 235    C++
    Player.exe!Player::Update(bool update_scene) Línea 375  C++

Dump: Player_2020-10-31-22-22-47.zip

[fmatthew5876]

At least part of this is definitely memory usage related: https://github.com/EasyRPG/Player/blob/master/src/game_pictures.h#L120

We should probably change the picture array to a sorted sparse array, and use binary search (or an intermediate hash table?) to go from picture id -> picture instead of a direct array lookup.

Having a sparse array here has other benefits even in normal cases it will save memory as games rarely use all 1000 pictures.

Doing this will make a commands like ShowPicture, ErasePicture and MovePicture slower as a lookup will now be required to get the picture. This would negatively affect games like Frozen Triggers which do a lot of ShowPicture every frame. The tradeoff will be less memory usage over all and a speedup for Game_Pictures::Update() as that function won't have to traverse and update a bunch of empty pictures.

[elsemieni]

@fmatthew5876 Anyways, this corner case is very rare (as until now it's not in any game, I just discovered it trying to solve a bug in a friend's game), considering it is out of memory crash rather than a illegal access one, maybe changing speedy structures for smaller ones will have pros and cons. Just to mentions ones:

• As pro, several games will act better in low-ram platforms? (for example 3DS).
• As you said, picture expensive-games like Frozen Triggers and some VIRPG can be negatively perfomance affected.

[Ghabry]

Maybe there should be also an upper picture limit. Don't think you ever want to allocate picture 1000000. This also creates gigantic save files :(

[fmatthew5876]

I didn't think about save files, but you're right. Also the whole idea is kind of dumb, why would you even want to have 1000000 pictures. It's not like you can draw that many on the screen.

I can think of 2 solutions:

1. Pick an upper limit, and throw an error if a game tries to create pictures above this limit.
2. Do a hybrid approach of both the dense and sparse array.

For example, we could always allocate pictures 1 to 1000. If you try to ShowPicture in this range it's a direct array lookup. Above 1000, we do the sparse array approach both in memory and in the save game. These big picture ids would require a binary search to lookup and have a performance penalty.

Since pictures above 1000 aren't allowed in RPG_RT anyway, we could store the spare upper half of the array in the save game file.

[CherryDT]

This doesn't sound like an issue with the pictures array though, more like an issue with the variables array, or am I misunderstanding something here? Trying to read a variable that doesn't exist should just return 0 without allocating that variable, but it sounds like this is not what happens.

Pictures above 1000: Note that it was possible to get up to 9999 pictures before with Hyper Patcher 2, and DynRPG uses 2000 pictures with 1001-2000 not being deleted on map change.