Open russellquinn opened 2 years ago
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...
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.
I'm glad it worked for your use case, but I think I'd prefer not to accept this PR in this instance, sorry!
Oops, meant to close PR. I still accept that this is a valid issue.
~Hello ! is there a workaround for this issue ?~ EDIT : @russellquinn thanks for your solution, just grabbed it and recompiled ink-engine-runtime.dll and it works 🎉
Yeah, my fix has been rock solid for me in my development for the last 2+ years.
If I have a global
LIST
, e.g.:I can get this list from within Unity with:
And when I do
unityList.origins
, it is populated and usable. I can also use.all
and other properties that depend onorigins
being populated. All good so far.If I then do
SaveJson()
andLoadJson()
I can get the list fromvariablesState
again, andorigins
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.:And then I
SaveJson()
andLoadJson()
, when I get the list fromvariablesState
this time,.origins
isnull
..origins
remainsnull
until the Ink script encounters some code that evaluates this list. (It can be assigning a new value or just checking its contents.) Thenorigins
gets populated once again.From stepping through the Ink-runtime code, it looks like
StoryState::PushEvaluationStack(Runtime.Object obj)
is responsible for populatingorigins
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 populatingorigins
in all the lists. If you then do aLoadJson()
, 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:
PushEvaluationStack
is called on all global Ink vars at the start.LoadJson()
, it looks likeLIST
s that no longer have their default value are re-created in memory.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