Ruin0x11 / OpenNefia

(Archived) Moddable engine reimplementation of the Japanese roguelike Elona.
MIT License
115 stars 18 forks source link

Serializing class instances can mutate their state in damaging ways #320

Open Ruin0x11 opened 3 years ago

Ruin0x11 commented 3 years ago

To avoid allocation, the :serialize() function on classes will return self as the table to be serialized by binser. However, there are some classes that have properties that should not be serialized, like pointers to data prototypes. When :serialize() is called on those objects, the current design is such that those properties should be set to nil by that method. :deserialize() is supposed to restore those properties to their intended values when a save is loaded. However, currently :deserialize() is not called on the objects after serializing to restore the removed properties if the game is not loaded after saving. This is very bad.

If the problem this was trying to solve was avoiding unnecessary allocations, then I have to wonder if the performance benefit is actually worth it. However, :deserialize() will act as if the table is the deserialized class instance, as in the table is not in a special serialized format that gets copied to a fresh instance of that class afterwards - the data deserialized by binser is the class instance itself.

Because of that, maybe it would be best to just call :deserialize() directly after calling :serialize(), and declare that the programmer contract for ISerializable is such that calling :serialize() and :deserialize() in order must be an idempotent operation.