Request: disable cloning the state when Config.history.maxStates is equal to 1

[ezsh]

When a game have a lot of variables and custom class instances in the state, and does not need the history, there is no need to clone and discard the state object.

[HiEv]

That's not actually true, because the history is what gets saved when you save the game, and the save/load system doesn't have any way of indicating that two or more variables refer to the same object.

[ezsh]

I don't believe one has to clone the state each turn in order to be able to serialize it.

[greyelf]

The cloning process results in the breaking of object referential integrity. eg. if two variables referenced the same object instance before the cloning then after it each of those two variables would now reference their own unique copy of that instance.

As I understand things the same behaviour occurs as a result of the serialisation / de-serialisation process used by the Story Save/Load system. If this is true then removing the cloning process (under the specified conditions) would result in an inconsistency between the outcomes of the Passage Transition and Save Loading processes.

Which I believe is what HiEv was alluding too.

[ChapelR]

I believe there are ways to solve this issue but I'm pretty sure any fix would be a breaking change, so unlikely before SugarCube 3.

[ezsh]

Maybe another switch in the Config object, which would imply maxStates = 1 for those who assume they understand what they are doing would solve the problem?

[tmedwards]

Cloning between moments is not the real issue here. While the current cloning scheme does not preserve references between objects, modifying it to do so would not be overly difficult—though it would come with a performance penalty.

The real issue is that SugarCube's current serialization scheme does not support preservation of references, and changing that would be difficult—and also come with various issues. Serialization is used both to maintain the current playthrough state and for saves, which are core features, so disabling it isn't on the menu.

The preservation of references within moments is something I hope to address and is tentatively scheduled for SugarCube v3.

[ezsh]

Cloning is the real issue here, both cloning to extend the history array (just to truncate it in the very next line) and cloning to the state storage. Obviously, there is no need to clone state variables for the history array (src/state.js, historyCreate()), there is no need to clone in momentActivate(), and I doubt there is need to clone before marshaling the state (stateMarshal()). Concluding, there are 3 clone() calls per passage change, and two of them do nothing except imposing performance penalties. All of the above said applies to the case Config.history.maxStates === 1 only.

[tmedwards]

Alright. For the sake of argument, let's say that cloning could be bypassed. How do you propose to fix the various issues with serializations—and in a backwards compatible way? Because without addressing that, either bypassing or updating cloning is pointless.

[HiEv]

The only easy-ish fix would be to also disable saves/loads, but I doubt most people would want to do that.

[ezsh]

What I am saying is 3 clone() calls per passage change is certainly too many. I can understand a need for a single cloning to be able to reload the current passage or load a save. Maybe it is the best solution, but two other clones are here for what? Also, this single cloning (I may be mistaken here) can be done asynchronously (maybe it is already is), after a passage was calculated, when it is read.

[ChapelR]

AFAIK, SugarCube is largely synchronus for legacy and compatibility reasons, but I remember TME mentioning that SugarCube 3 would be largely or completely asynchronus in design.

I don't know about 3 clone calls but HiEv is right that there needs to be at least one per transition to support saving and loading, including session preservation across refreshes and such, at least using the current cloning method, which does not support references.

If TME fixes the cloning issue it will fix it regardless of the history settings. I think it would probably be a breaking change since some (unwise) code out there may rely on passage-to-passage cloning instead of explicitly cloning something.

I don't and can't speak for TME, but while theoretically you could add the change behind a config, you'd then have two different cloning methods to maintain in the same version, you'd have to document what will be to most users a subtle or even meaningless change, and ultimately I don't think working around this is difficult enough to warrant all that. It's the same exact problem, at the end: the references won't behave how users expect, and they'll have to scrounge around to find out why, except instead of the current solution(s) they'd be looking for a specific config.

[ezsh]

Here is what I observe now when a passage navigation link is clicked:

1. Add a new history record cloning the active state.
2. Remove excess records from the history array.
3. Activate required history record, cloning the sate one more time.
4. Save the state to the storage, again cloning it.
5. Render a new passage.

If one wants to keep saving the state and not the rendered passage, I'd use the following sequence:

1. Block on async state clone to the storage, waiting for it to complete (I guess it will take less time than an average passage requires to read it, so most of the times the block will take no time).
2. if maxStates > 1 do a clone and add a new state to the history, otherwise just change the passage name.
3. Render the new passage and display it.
4. Launch asynchronous cloning to the storage. Atomically replace data in the storage.

When loading a save or reloading the page, if the passage name in the active state and the passage to be activated are the same, clone the state from the storage and re-render the passage.

I think the scheme above brings no semantic changes and is fully compatible with the current implementation. When maxStates == 1 passage transition requires no clone() calls, and a single call is issued otherwise.

[greyelf]

Launch asynchronous cloning to the storage. Atomically replace data in the storage.

And what happens if this asynchronous process fails for any reason?

eg. The Local Storage area allocated to that 'domain' is full.

[ezsh]

No, there will be no situation when the asynchronous cloning has started and the state changes. The difference between this asynchronous cloning and the current execution flow is that now all these actions are initiated when readers request a change (clicking a link, for example), while I propose to save state immediately after a passage finished rendering. The state does not begin to change until the saving completes or fails.

[HiEv]

The number of clonings is largely irrelevant if you don't sort out the issue of being able to save and load the history in a compatible way with the current systems, which simply can't be done due to the fact that there's no architecture in place to handle two or more variables referring to the same object.

Without that, it's basically a feature that has to wait for SugarCube v3.

If you want to argue about the efficiency of the current system, then that's a completely different bug report.

[greyelf]

while I propose to save state immediately after a passage finished rendering

A copy of the current state needs to be made before the contents of the 'current' Passage is processed, thus before that Passage's generated HTML elements are rendered. When in the Passage Transition process that copy is persisted to Local Storage is a mute point, as long as that persistence is guaranteed to be successful.

Otherwise the activity of recovering from a web-browser refresh or resisting History from a Save load, both of which involve loading state stored in Local Storage, could result any variable changes made in the current Passage being applied twice.

[ezsh]

immediately after rendering is two steps earlier than it is done now: before clicking a link and before beginning the passage change.