Fixes to seeking and auto restore

[SuuperW]

The current implementation of TAStudio is a gigantic mess and has various pesky bugs. This is an attempt to resolve some of these issues.

Changes that are not bug fixes:

1. Show seek progress bar even for short seeks.
2. Include hotkey to go to green arrow, default is R.
3. Middle-click resumes a seek if a seek was in progress. (was this a bug? not sure)
4. Changed Lua API method tastudio.setplayback to return as soon as the seek begins. This is basically a side-effect of fixing bugs. See comments below for reasoning. (Fixes to seeking and auto restore #4347 (comment))
5. Removed Lua API method client.seekframe. It would freeze the user interface for the duration of the seek, same as setplayback.

List of bugs fixed (also in commit message):

1. Auto-restore did not work when painting "axis" inputs.
2. If unpaused in recording mode, manual seek to frame A (click cursor column) then before that seek finishes seek to frame B. It would not unpause after reaching frame B to resume recording.
3. TAStudio would fail to pause with auto-restore if a second edit was made to a non-greenzoned frame while auto-restore seek was in progress.
4. Canceling seek would not remove the seek progress bar.
5. Mouse drag seeking was broken while seeking. (It would not change the target seek frame until prior seek had completed.)
6. Multiple issues with seeking to frames past the current movie length.
7. Modifier key + right click would jump to the last edited frame (even if this right click made no edits).
8. Clicking on an axis value while in axis editing mode would disable recording mode, regardless of whether an edit was made.
9. Clicking on an axis value while in axis editing mode would trigger auto-restore, even if no edit was made (seek to last edit frame).
   EDIT: I'm not listing them all here anymore, see commit comments for more details. But, somewhere along the line I accidentally fixed these:
10. It used to be possible to invalidate greenzone without rewinding by pressing escape after mouse editing an axis value.
11. Right-click + scroll up (to seek backwards) immediately after opening TAStudio used to break the emulator pause functionality; it would be stuck unpaused.

I've done my best to avoid changing any behavior that isn't a bug. If you see any, please let me know.

Check if completed:

• I have run any relevant test suites
• I, the commit author, have read the licensing terms for contributors (last updated 2024-06-22) and am compliant

[Morilli]

2. If unpaused in recording mode, manual seek to frame A (click cursor column) then before that seek finishes seek to frame B. It would not unpause after reaching frame B to resume recording.

Does that even make sense though? Maybe seeking while unpaused and in recording mode actually should pause once seeking finishes? I generally do not really see a situation in which you would want to seek somewhere while unpaused and in recording mode, that sounds like a recipe to lose inputs.

[SuuperW]

If we don't know what workflow would do that, then I think the best thing to do is provide consistent behavior. Making an exception for a special case we don't understand seems like a bad idea.
Anyway, I do it sometimes when I am messing around/exploring in-game. Never when I care about my precise inputs, but still I'd personally be annoyed if it paused unexpectedly or failed to re-enable recording mode.

As for losing inputs, the undo feature exists so I don't see this as being an issue. (It's currently totally broken in recording mode, but that's another issue and a bug I intend to fix.)

[SuuperW]

Most recent commit (fix for Lua) changes the behavior of the Lua API. Previously, tastudio.setplayback would not return until seeking had completed and no Lua events/callbacks would happen during the seek. Now it returns when seeking begins (after load state, if that happened).
Note that seeks caused by tastudio.applyinputchanges were not broken in this way.

Although the old behavior is very bad, some Lua scripts could potentially no longer work without modifications. Is there significant value in making the old behavior available?

[vadosnaprimer]

Most recent commit (fix for Lua) changes the behavior of the Lua API. Previously, tastudio.setplayback would not return until seeking had completed and no Lua events/callbacks would happen during the seek. Now it returns when seeking begins (after load state, if that happened).

Sounds kinda weird. Callbacks being ignore while it's seeking may or may not be desirable (I never used lua to control tastudio/taseditor behavior in real tasing), but "set playback" implies a concrete event which is defined in that function's description:

Seeks the given frame (a number) or marker (a string)

The goal is getting to that frame before we do something. After it finishes, it is expected that it's now safe to do whatever we wanted to do on that frame (or marker). Especially if there has to be a lot of seeking due to lack of states on those frames: if we say "Hey tastudio, please put playback on frame 9000!" we won't be satisfied if it puts it on frame 1000 and says "Done!!!".

the old behavior is very bad

Why?

[SuuperW]

The goal is getting to that frame before we do something. After it finishes, it is expected that it's now safe to do whatever we wanted to do on that frame (or marker). Especially if there has to be a lot of seeking due to lack of states on those frames: if we say "Hey tastudio, please put playback on frame 9000!" we won't be satisfied if it puts it on frame 1000 and says "Done!!!".

This is at least trivial to fix in Lua: while emu.framecount() < targetFrame do emu.frameadvance() end

Why?

1. It freezes the user interface. If TAStudio has to seek 8000 frames and the user can only emulate 200fps, it will be 40 seconds until ANYTHING is displayed and the user can take any further action. This would appear to be BizHawk crashing. This also means that it is impossible to cancel the seek, for example if the user didn't realize they were about to re-emulate thousands of frames.
2. During this time no Lua events can run. This means that any Lua functionality that depends on monitoring playback or keeping track of values over time can never work in a script that uses tastudio.setplayback. For example a script that I regularly use requires data from the previous frame (sometimes two) in order to display correct information on the current frame, and that is impossible if I use tastudio.setplayback(frame_i_want_to_go_to).

[vadosnaprimer]

This is at least trivial to fix in Lua: while emu.framecount() < targetFrame do emu.frameadvance() end

It's not something that needs fixing because it's not a bug but an explicit function. We told it to get there, it got there, we're done.

It freezes the user interface. If TAStudio has to seek 8000 frames and the user can only emulate 200fps, it will be 40 seconds until ANYTHING is displayed and the user can take any further action. This would appear to be BizHawk crashing. This also means that it is impossible to cancel the seek, for example if the user didn't realize they were about to re-emulate thousands of frames.

Didn't know about that. Does it depend on any option or is the freeze forced no matter what?

During this time no Lua events can run. This means that any Lua functionality that depends on monitoring playback or keeping track of values over time can never work in a script that uses tastudio.setplayback. For example a script that I regularly use requires data from the previous frame (sometimes two) in order to display correct information on the current frame, and that is impossible if I use tastudio.setplayback(frame_i_want_to_go_to).

This sounds like you have turbo seeking enabled and scripts during turbo disabled. If that's not the case, then I agree that it's also a problem.

However it doesn't mean that when we tell it to frame 1000 it should start seeking from frame 10 and immediately say "done!" There needs to be some proper solution to it doing this deep freeze.

[SuuperW]

The freeze is forced no matter what.

This sounds like you have turbo seeking enabled and scripts during turbo disabled. If that's not the case, then I agree that it's also a problem.

It freezes without turbo seek. I rarely have turbo seek enabled, and never turn off scripts during turbo.

I don't know what we could do to allow the script to run during the seek, without returning from tastudio.setplayback. It would not be too hard if the script uses the after-frame event, but since the "default" way to run some Lua code every frame is to run an infinite loop with emu.frameadvance() in it, ... well, I don't know how to make that work. The script will obviously be at the call to tastudio.setplayback, and I don't think there's any sensible way to move execution to wherever in the script emu.frameadvance() is.

[vadosnaprimer]

Maybe I'm seeing it now. So what actually happens when I do setplayback(9000) from frame 10, given the most recent changes? Will it return from that function but actually keep seeking to 9000 while running all the tools normally?

What about client.seekframe()?

[SuuperW]

Also, if the UI did not freeze then the user would be able to pause emulation and interrupt the seek. This will be much easier to handle if tastudio.setplayback has already returned. If it hasn't, and Lua is still waiting for the seek to complete, what happens if the seek never completes? We cannot possibly guarantee that emulation will ever reach the frame specified in the call to tastudio.setplayback, and suspending the Lua indefinitely would be very confusing behavior ... as would sometimes returning on the wrong frame but not always.

[SuuperW]

Maybe I'm seeing it now. So what actually happens when I do setplayback(9000) from frame 10, given the most recent changes? Will it return from that function but actually keep seeking to 9000 while running all the tools normally?

What about client.seekframe()?

There are two possibilities here.
First, if the movie does not have 9000 frames, setplayback will return immediately, doing nothing. (I do not know why this check exists.)
If the movie does have 9000 frames, the new setplayback behavior is to return right after the seek begins. It will keep seeking while all the tools run normally including the Lua script, which must explicitly check the current frame to know when frame 9000 is reached.

In either situation, seekframe will freeze the UI and seek, the same as old setplayback behavior (for when the movie did have 9000 frames) and just as it would in the current master branch since I haven't modified that code

[vadosnaprimer]

Yeah okay. As long as it keeps seeking (until you disable it) returning right away is better.

[SuuperW]

My list of things to change in this branch is now empty. Please review.

[SuuperW]

Fixed issues brought up in @YoshiRulz review along with one or two other cleanups. Also fixed regression and accidental behavior change: Lua's GreenzoneInvalidatedCallback was getting called at a different point and sometimes not at all.
And a few more fixes to tastudio.setplayback.

[YoshiRulz]

Suggested change

	if (TasView.IsPartiallyVisible(frame) || frame < TasView.FirstVisibleRow)
	if (frame <= TasView.LastVisibleRow || TasView.IsPartiallyVisible(frame))

May be faster?

[SuuperW]

Probably, but there are more significant performance optimizations in the implementation of ListView if you want to go there. In this particular case it's insignificant since (1) this isn't performance-sensitive logic and (2) the vast majority of edits will be in visible rows anyway.

[YoshiRulz]

Is bug fix 2) in OP the same as #2329?

[SuuperW]

Probably not, since he doesn't mention recording mode. It's more likely he was experiencing bug 3, but none of his posts indicate the steps taken to reproduce the bug so I can't tell.

EDIT: No, not 3, since he said he was clicking on the left column not doing edits.

[vadosnaprimer]

I missed that this breaks a script we ship (and one we want to start shipping). Please fix.

BizHawk/Assets/Lua/GBA/SonicAdvance_CamHack.lua

Lines 24 to 26 in e5817d9

	client.seekframe(emu.framecount()+1)
	client.invisibleemulation(false)
	client.seekframe(emu.framecount()+1)

[Morilli]

Is there any reason you changed this to be int.MaxValue instead of frame? This breaks numerous (reasonable) assumptions in the code that PauseOnFrame actually contains the correct frame number value and leads to incorrect behavior with turbo seeking because the frame advance code does not know when the turbo seek ends anymore.

[SuuperW]

What specific incorrect behavior are you seeing? The only reference to PauseOnFrame that care about the value other than checking if it is null or <= Emulator.Frame is MainForm line 3018. That might be an oversight by me (looks like it might cause the core to not render the last frame, for cores that support skipping rendering). But it's not related to frame advance.

For everything else, TAStudio handles things by setting PauseOnFrame to null at the correct time.

I would guess that the reason I set it to MaxValue was to avoid potential issues with having it get out of sync with _seekingTo, e.g. when using mouse wheel to change the seek frame. But the correct solution to that is probably to change _seekingTo to a property and use MainForm.PauseOnFrame as a backing "field". EDIT: Actually that would make it so that MainForm's pause-after-seek logic applies for turbo seeks, which is different from TAStudio's.

[Morilli]

Well it kinda breaks the tooltips in SetPauseStatusBarIcon although that's a really minor issue.

But yes, it also directly regresses #2081. Additionally this will now call UpdateAfter twice for lua scripts, once in StepRunLoop_Core because atTurboSeekEnd isn't set, and once in the StopSeeking call, which causes everything to be rendered twice which is very noticable with transparent drawings.

[SuuperW]

See #4743