Altered LISTs do not have origins set after LoadJson()

[russellquinn]

If I have a global LIST, e.g.:

LIST some_list = (none), step_one, step_two, completed

I can get this list from within Unity with:

Ink.Runtime.InkList unityList = story.variablesState["some_list"] as Ink.Runtime.InkList;

And when I do unityList.origins, it is populated and usable. I can also use .all and other properties that depend on origins being populated. All good so far.

If I then do SaveJson() and LoadJson() I can get the list from variablesState again, and origins is populated just like before. Still… all good.

However, if the Ink script has changed the value of some_list before the save, e.g.:

~ some_list = step_one

And then I SaveJson() and LoadJson(), when I get the list from variablesState this time, .origins is null.

.origins remains null until the Ink script encounters some code that evaluates this list. (It can be assigning a new value or just checking its contents.) Then origins gets populated once again.

From stepping through the Ink-runtime code, it looks like StoryState::PushEvaluationStack(Runtime.Object obj) is responsible for populating origins and I think this is where the problem lies.

When an Ink Story is first loaded from C#, PushEvaluationStack gets called on every global var, thus populating origins in all the lists. If you then do a LoadJson(), and the List value hasn't changed, this initial init is still valid and everything works. But, if the value has changed, it looks like the list is re-created(?) and this time, PushEvaluationStack does not get called (until it is used in some Ink code that is run later).

In summary, I think the problem in the Ink runtime might be:

1. PushEvaluationStack is called on all global Ink vars at the start.
2. After a LoadJson(), it looks like LISTs that no longer have their default value are re-created in memory.
3. But, PushEvaluationStack is not called on these, leaving them not fully initialized.

UPDATE: I have opened a PR with a fix, which solves the problem I was having with this. But, I'm pretty sure it should be fixed in a more comprehensive way. Hopefully this PR is a useful start though: https://github.com/inkle/ink/pull/764

[joethephish]

Apologies for the very late reply, and thanks for this analysis, it's really helpful.

I've just been looking into this stuff today, and I wanted to share some (perhaps inconclusive) thoughts for the record!

I think the reason this happens is an unfortunate case that origins was never really intended to be a public API, and could also perhaps be better named as cachedOrigins (though now it is public, it perhaps shouldn't be renamed!) The fact that the origins are refreshed when all the globals are first initialised is basically luck because all globals happen to go through PushEvaluationStack at the start.

The basic assumption I think I made when I wrote this stuff is that list values are "asleep" until they enter the engine to be used, and PushEvalutionStack is a bottleneck that all values have to go through to be used in any kind of internal calculation, hence the refresh being done there. You can think of them as becoming "awake" and refreshed when they go through there, at which point origins is valid and correct. I imagine I simply didn't consider that external game code might want to make use of origins for "asleep" lists 🤦.

The fact that we set origins in other places (such as constructors) is inconsistent and unreliable unfortunately. I probably did it in a bunch of places because it was easy and seemed like a good idea at the time, since having origins be correct more often than not seemed like a good thing. But it was probably a bad idea since it's correct most of the time now but can't be relied upon.

One other factor to consider is that lists are the slowest/heaviest construct we have in the engine, so we need to take care to not do this refreshing too often. I vaguely recollect that doing the origin refresh in PushEvaluationStack was an optimisation - it's lazily updated at the very last minute when we know for sure that the list is about to be used.

Thanks very much for the pull request by the way! I'm still considering what the best way forward is. Maybe it's possible that there are actually a limited number of cases where origins is incorrect and that we can fix it so that it's always correct. But I want to be sure that any change I make doesn't cause a performance regression...

[russellquinn]

Thanks for the backstory! Yeah, balancing performance cost seems like the nut to crack with this one. Centralizing the logic a bit might make the path forward clearer, even if functionality doesn't change immediately?

Anecdotally, I've been running a forked version of Ink with my PR for the last couple of months. I don't have a huge Ink project — certainly not like what you're dealing with — but it's complex and (I think) I'm pushing the runtime hard. I haven't noticed any performance degradation from my changes, but I also haven't run any definitive timing tests.

[joethephish]

I'm glad it worked for your use case, but I think I'd prefer not to accept this PR in this instance, sorry!

[joethephish]

Oops, meant to close PR. I still accept that this is a valid issue.