search

CaravelGames / drod

The official public repository of Deadly Rooms of Death and DROD RPG
http://caravelgames.com
56 stars 17 forks source link

Add support for array script variables #587

Closed Hypexion closed 4 months ago

Hypexion commented 5 months ago

Adds new functionality to scripts by allowing arrays. A script variable starting with a # becomes a global array, and a variable starting with a @ becomes a local variable. To avoid weird issues with serialization, once a variable is created, it can't be renamed in a way that changes its "arrayness".

Internally, arrays are technically maps, as they can be non-continuous. Negative indexes are also supported, although this is not unheard of in some languages.

Global arrays are serialized back to the packed stats object when saving. The number of active items in the array is saved as the first value, followed by a sequence of key/value pairs. The readBpUINT and writeBpUINT functions have been moved to CDbSavedGame to allow them to reused for this. To save space, values of zero are not packed when serializing (as getting an int value from a map will return zero is no value is assigned to that key).

New script commands have been added to write to arrays. Reading from arrays is handled by expression parsing functions.

Thread: http://forum.caravelgames.com/viewtopic.php?TopicID=46113

mrimer commented 5 months ago

@Hypexion Thanks for the PR! This is cool functionality that would have probably been useful to have a long time ago.

I'm wondering whether we need to be concerned about RAM usage. Considering that game snapshots will store full NPC var copies, what would it take for a player to use up all available RAM and crash the app?

I'm also concerned about address fragmentation in the 32-bit Windows binary in particular. We've had issues with this in the past and have had to be very careful to avoid a lot of random-access map allocs and de-allocs, e.g., in DB acceleration structures. If players go nuts with this functionality, or there's a bad actor, it would be easy to crash the game.

What guards do we want to put in place here? Maybe not allow over a certain high cardinality of entries in a map (i.e., check for map size before a new index assignment)? Even if it's something relatively large like ten thousand, 100k, or a million, I think we need some guardrails.

Hypexion commented 5 months ago

I'm wondering whether we need to be concerned about RAM usage. Considering that game snapshots will store full NPC var copies, what would it take for a player to use up all available RAM and crash the app?

I'm also concerned about address fragmentation in the 32-bit Windows binary in particular. We've had issues with this in the past and have had to be very careful to avoid a lot of random-access map allocs and de-allocs, e.g., in DB acceleration structures. If players go nuts with this functionality, or there's a bad actor, it would be easy to crash the game.

What guards do we want to put in place here? Maybe not allow over a certain high cardinality of entries in a map (i.e., check for map size before a new index assignment)? Even if it's something relatively large like ten thousand, 100k, or a million, I think we need some guardrails.

I've pushed up some changes to smooth things out, but I definitely see the concerns here. It might be worth restricting arrays to a specific range of indexes. Five or six digit range in positive and negative directions is probably enough for anyone, I would think.