Save/Load + Rewind oddity

[Simon]

Let's first go over save+load4, because we agree that there is no bug here:

* Dig in frame 80
* Savestate in frame 400
  - Savestate's replay has (80 digger)
* Jump in frame 1006
* Load the savestate.
  - Active replay stays active (Reason: State's replay is initial segment of active)
  - No snipping sound (Reason: We didn't cut anything from active replay)
  - We return to frame 400
* Implode at frame 441
  - Snipping sound plays (Reason: We cut the jumper assignment from frame 1006)
* Load the savestate again.
  - Active replay stays active (Reason: (80 digger) is initial segment of (80 digger, 441 imploder))
  - No snipping sound (Reason: We didn't cut anything from active replay)
  - We return to frame 400
  - If we wait until frame 441, we'll see the imploder play back.

Here's a scenario in which save+load3 is bug-free in 0.9.9 design:

* Dig in frame 100
* Jump in frame 1300
* Framestep back to frame 500
* Savestate in frame 500
  - Savestate's replay has (100 digger, 1300 jumper)
  - R in corner is visible (Reason: future assignment of 1300 jumper)
* Begin video capture
* In frame 550, load state.
  - Active replay stays active (Reason: state's replay is identical to active)
  - No snipping sound (Reason: Active remains as-is)
  - We return to frame 500
  - Active replay is still (100 digger, 1300 jumper)
  - State's replay is still (100 digger, 1300 jumper), same as active
* Implode at frame 700
  - This cuts (1300 jumper) from active replay (Reason: 1300 is in the future)
  - Snipping sound plays (Reason: We cut something from active replay)
  - Active replay is now (100 digger, 700 imploder)
* Load the savestate.
  - State's replay is still (100 digger, 2000 runner)
  - State's replay overwrites active (Reason: State's replay isn't initial segment)
  - Snipping sound plays (Reason: We cut (700 imploder) from active replay)
  - Active replay is now (100 digger, 1300 jumper)
  - If we wait until frame 1300, we'll see the jumper play back
 
-- Simon

[Forestidia86]

Here's a scenario in which save+load3 is bug-free in 0.9.9 design:

* Dig in frame 100
* Jump in frame 1300
* Framestep back to frame 500
* Savestate in frame 500
  - Savestate's replay has (100 digger, 1300 jumper)
  - R in corner is visible (Reason: future assignment of 1300 jumper)
* Begin video capture
* In frame 550, load state.
  - Active replay stays active (Reason: state's replay is identical to active)
  - No snipping sound (Reason: Active remains as-is)
  - We return to frame 500
  - Active replay is still (100 digger, 1300 jumper)
  - State's replay is still (100 digger, 1300 jumper), same as active
* Implode at frame 700
  - This cuts (1300 jumper) from active replay (Reason: 1300 is in the future)
  - Snipping sound plays (Reason: We cut something from active replay)
  - Active replay is now (100 digger, 700 imploder)
* Load the savestate.
  - State's replay is still (100 digger, 2000 runner)
  - State's replay overwrites active (Reason: State's replay isn't initial segment)
  - Snipping sound plays (Reason: We cut (700 imploder) from active replay)
  - Active replay is now (100 digger, 1300 jumper)
  - If we wait until frame 1300, we'll see the jumper play back
 

Simon that's just not what usually happens. Rewinding cuts the jumper out of the replay. I have attached a video.

[Forestidia86]

If you have ticked that the replay is kept upon rewinding then it behaves as you describe.
But I'm not sure if that is good since with loading you usually want to discard some of the later actions?

[Simon]

Ah, yeah, I forgot that, during save+load3, you would have discarded replay actions on framestep-back.

Here's the scenario where save+load3 is still bug-free even when you discard replay actions on framestep-back. Changes to reply #15 are in bold.

* Dig in frame 100
* Savestate in frame 500
* Jump in frame 1300
* Load state
  - State's replay was (100 dig)
  - Active replay was (100 dig, 1300 jump)
  - Therefore, keep active replay because state's replay is initial segment.
  - We're back in frame 500
* Savestate again in frame 500
  - State's replay was (100 digger)
  - State's replay is now (100 digger, 1300 jumper)
  - R in corner is visible (Reason: future assignment of 1300 jumper)
* Begin video capture
* In frame 550, load state.
  - Active replay stays active (Reason: state's replay is identical to active)
  - No snipping sound (Reason: Active remains as-is)
  - We return to frame 500
  - Active replay is still (100 digger, 1300 jumper)
  - State's replay is still (100 digger, 1300 jumper), same as active
* Implode at frame 700
  - This cuts (1300 jumper) from active replay (Reason: 1300 is in the future)
  - Snipping sound plays (Reason: We cut something from active replay)
  - Active replay is now (100 digger, 700 imploder)
* Load the savestate.
  - State's replay is still (100 digger, 2000 runner)
  - State's replay overwrites active (Reason: State's replay isn't initial segment)
  - Snipping sound plays (Reason: We cut (700 imploder) from active replay)
  - Active replay is now (100 digger, 1300 jumper)
  - If we wait until frame 1300, we'll see the jumper play back

It's a very concocted sequence, and I've filed #295: Savestate shouldn't preserve future when !(keep replay after ◀▮) because I don't like this behavior in 0.9.9. Still, it's possible in 0.9.9 and doesn't exhibit the reply-#4 bug.

-- Simon

[Forestidia86]

Yeah, that's what probably happened in my tests.

I'm actually out of ideas how to reproduce the bug especially since savestate/stateload behaves intendedly so peculiar.

[Simon]

Thanks for the efforts still, and this debugging code is good to have around.

If you feel that fixing #295 Savestate shouldn't preserve future when !(keep replay after ◀▮) still won't be a good design, feel free to suggest improvements to the savestate-replay-handling. As it stands, it's confusing, even though it often does what you want. Sometimes it does more than what you want, but then you can cancel.

-- Simon

[Forestidia86]

The only thing which could be counted in is that I have slow machine with low FPS. So especially fast frameskipping back behaves shaky/produces bigger steps back by design. Maybe these kinds of recalculations play a role?

[Simon]

I can't be sure since I don't understand the bug, but I doubt the slow machine affects the correctness of the algorithm.

After reading your reply #4 several times, the biggest oddity remains the extra blocker assignment in the replay. Crazily, after you got stuck and restarted from scratch (which should clear the replay considering your option preserves the replay but you'd still erase it on your first assignment before frame 800), this blocker assignment remained in the replay.

-- Simon

[Forestidia86]

I can't be sure since I don't understand the bug, but I doubt the slow machine affects the correctness of the algorithm.

I meant less the slowness of the machine itself but rather how your code handles it. It tends to do at times huge warps backwards when using fast frameskipping back and it really looks shaky then.
Would it be possible that the recalculations access the savestate rather than the active replay?

[Forestidia86]

Simon, I have it recorded. I used the level "Any Way You Want": It shows a basher asssignment as past actions in the list but none of the bashers is used up.
In this case it doesn't seem to erase the action from the active replay when loading a later point phyu-wise and the save is still part of the replay actionwise.

So basically:

Save.
Rewind a bit (don't cancel anything which would belong to the save).
Do an action (before the phyu-point of saving).
Load.
=> Action still in active replay as past action (although not part of the loaded state).

Edit: Exchanged "Hellfire"-video with a much more to the point video.

[Simon]

Amazing reduction! What a glaring and simple way to have unprocessed assignments in the replay.

It certainly looks like a logical error in the code, should be unrelated to increasing the framestep length on slower machines.

-- Simon

[Forestidia86]

I think it belongs more to this thread:

It doesn't seem to desync anymore with the method I found.
To preserving future: The case mentioned in the issue shouldn't work this way anyways since rewinding before an action cancels if you don't keep replay (, unless you've meant A,C instead of A,B in the end?). But you've mentioned another case in the thread, where you load instead of framestep back. Well loading cuts now all out what doesn't belong to the savestate (even if you have keep replay on and it's after the savestate), so with that the future replay is discarded as well.

Just to clarify if you have keep replay when rewinding on there is still a possibility to have future in the savestate: By framestepping back before an action and then saving.

----
Well and one new desync problem: If you go to a phyu before the save now and cancel an action of the save you have to press load twice to get the actions of the savestate in the replay. If you only press once you get the savestate but the action isn't in the active replay.
Video attached.

Edit: Just for clarification: The new desync is a matter of the unstable build.

[Simon]

if you have (option: keep replay when rewinding) on there is still a possibility to have future in the savestate: By framestepping back before an action and then saving.

Yes, this is desired.

Under that option (keep replay after ◀▮), framestepping backwards shall never erase from the replay. And the savestate is designed to preserve as much as possible from the session.

Well and one new desync problem: If you go to a phyu before the save now and cancel an action of the save you have to press load twice to get the actions of the savestate in the replay. If you only press once you get the savestate but the action isn't in the active replay.
Video attached.
Edit: Just for clarification: The new desync is a matter of the unstable build.

Good catch again!

I should really unittest these behaviors and cover our fixes with regression tests, but I'm scared of testing such high-level UI... I guess I have to look into it.

Clicking twice on load-state should have the same effect as clicking once.

-- Simon

[Simon]

Fix for (2 posts above) pushed to unstable master, and I've rebased debug-replay on top of that. If you'd like, you can test debug-replay (ba6e3794), although I'm reasonably confident this time.

Unless we find egregious new problems, I'd like to release these days. I'll include the more frequent garbage collection, even though it didn't fix the ballooning RAM.

-- Simon

[Simon]

I still have bugs. Every time I fix a bug, another of these savestating bugs come back.

But I have now written automatic tests for these savestating bugs. I already test for 3 scenarios, including the 2 from this thread, under both options (keep replay after ◀▮ or discard), which makes 6 automated tests for the state loading. I haven't gotten them to all pass at the same time, but I'm too tired today to investigate further. Will see what I get these days!

Forestidia: You don't have to test anything. Take a couple days off, you earned it.

-- Simon