search

tmedwards / sugarcube-2

SugarCube is a free (gratis and libre) story format for Twine/Twee.
https://www.motoslave.net/sugarcube/2/
BSD 2-Clause "Simplified" License
177 stars 41 forks source link

Diff ignores arrays #168

Closed Kassy2048 closed 2 months ago

Kassy2048 commented 2 years ago

Hi,

When calling Diff.diff() to generate deltas between states, arrays properties are always cloned in the result, even if there is no change in the array. This method seems to support diffing between arrays, so this is probably an unexpected behavior.

Arrays are objects, but Object.prototype.toString.call() returns [object Array] for them, not [object Object], so this check fails and the arrays content is always cloned. Changing the condition from if (origPType !== '[object Object]') to if (origPType !== '[object Object]' && origPType !== '[object Array]') makes it possible to diff between array properties and significantly reduces the deltas size on my tests (up to x10 on some games).

Also, I have a question: why deltas are not used for saves? This could reduce the localStorage space usage, and maybe remove the need for lzstring (or at least speed it up as the data to compress would be smaller).

Kassy2048 commented 2 years ago

Also, I have a question: why deltas are not used for saves? This could reduce the localStorage space usage, and maybe remove the need for lzstring (or at least speed it up as the data to compress would be smaller).

Ignore that part please: deltas are used for saves, but they are created after the call to stateMarshal() instead of during that call.

tmedwards commented 2 years ago

I know that it used to handle arrays. I don't recall at the moment if array handling was purposefully removed for some reason or it was accidentally dropped during a revision. At a guess, it's probably the latter.

I'll look into it when I get my workstation up and running again.

Kassy2048 commented 2 years ago

FYI, it probably stopped working when you added code to filter out unknown non-generic objects (r1617 from the old mercurial repository). The commit message does not mention arrays explicitly, so probably a mistake indeed.