Add Input Recording/Playback Functionality

[xTVaser]

First off, the vast majority of the credit for this work goes out to @pocokhc https://github.com/pocokhc/pcsx2-1.4.0-rr and @DocSkellington https://github.com/DocSkellington/pcsx2-1.4.0-rr I've just simply merged with upstream pcsx2 and made a few small bug fixes and improvements to the recording features, not pcsx2 itself.

These changes are not without issues but the core recording functionality is better than anything I've previously seen or worked with on pcsx2 and in my experience is quite reliable (I've never lost a recording yet, desyncs are comparable to Dolphin's recording, etc).

I will continue to work on polishing up these new features but I would only want to address major-blocking issues before getting this merged (otherwise, it never will lol). In addition, we also have Lua scripting tools and features to contribute, but as this PR is already getting fairly large, I will PR those separately after getting this through. Said code is currently in this branch https://github.com/xTVaser/pcsx2-rr/tree/add-lua-scripting

Changes

PCSX2 Unimplemented / "Hidden" Features

• We have implemented the Save/LoadStateOther_Click which lets you arbitrarily pick a file to save or load to. These have been refactored from using Other -> To/FromFile

  pcsx2/pcsx2/gui/MainMenuClicks.cpp

  Lines 531 to 539 in f6e4a30

  	void MainEmuFrame::Menu_LoadStateOther_Click(wxCommandEvent &event)
  	{
  	Console.WriteLn("If this were hooked up, it would load a savestate file.");
  	}
  	void MainEmuFrame::Menu_SaveStateOther_Click(wxCommandEvent &event)
  	{
  	Console.WriteLn("If this were hooked up, it would save a savestate file.");
  	}

• We have exposed the F12 video/audio recording capabilities of GSdx through a menu option
• We have also added a menu option to save a screenshot to a particular path rather than always the default snaps/ folder

Preprocessor Defintions

• In order to facilitate the development story of testing whether or not these additions are causing problems there is a new preprocessor definition that removes all modifications within the pcsx2 core code (anything outside of the Recording/ directory) - DISABLE_RECORDING

GUI Modifications

• Two new top level menu's -
  • Capture
    • This menu's submenu contains the options to capture video or take a screenshot, it is always displayed
  • Recording
    • This menu's submenu contains everything related to starting a new input recording, opening the virtual controller GUI panels, or the recording editor panel.
    • This menu is not displayed by default and must be toggled on through the new menu option in the System submenu called Enable Recording Tools. This toggle is also preceded by a prompt warning the user about the potential performance / instability / work-in-progress nature.
• A simplified window title when recording:
  • During recording the window title will display relevant recording information, such as the current frame, whether they are replaying or recording, etc. It will also update far faster than the default title, every 50ms

New Keybindings

• During recording or playback Shift-R is used to switch between replay or recording mode
• Shift-P to pause the emulator while still maintaining the game window
• Space bar used to individually frame advance
  • These three keybindings are effectively disabled when Recording tools are toggled off - https://github.com/PCSX2/pcsx2/pull/2412/files#diff-eeaa6cdb49cae7c414d70071d24ab079R469
• Added keybindings for saving / loading savestates - 1-9 on the numberpad to load, Shift+1-9 on the number pad to save
  • These are not disabled by default, I figure they are out of the way from normal defaults (unlike space) and are something anyone could benefit from having.

Console Logging Additions

• Added 2 new console log sources
  • Recording Console All feedback from recording goes through here
  • Controller Info disabled by default, outputs the raw byte data per frame for the controller in port 1.
    • This was/is useful for debugging recording problems.
  • These new sources are disabled by default, and the Recording Console automatically enables once Recording tools are toggled on.

Recording Features

• The recording can start from a power-on state, or from the current frame.
  • If the current frame is chosen, then the movie file will include the save state for Frame 0 - This means that it is not necessary to backup and save the starting save-state like many emulator recording features require.
• Virtual controller GUIs for accurate input entry
  - Recording editor to modify / view the inputs on a per frame basis
  • Removed from this PR, not critical and large gui refactor.
    - Conversions from old movie file formats:
  • Also removed, I now see this spike as a chance to "reset" back to v1 of the file format

Demo

• Walkthrough of recording usage - using pocokhc's version on 1.4.0 so this is quite dated at this point
• Quick example TAS

Outstanding Questions

• Where should the documentation live? The current docs need a bit of polishing but I'm unsure whether to just add the pages to pcsx2's repo wiki, or create new articles on the official wiki (or both/neither)...let me know.

[sudonim1]

I should actually come back and get my own todos done before my opinion's worth anything but... do we really want such extensive changes in mainline? Input recording and replay's definitely something that would be nice to have (although very much preferably in a plugin independent way, which would mean changing the way the plugins and core work), but... lua scripting? Um. Some of this really seems like material that should stay in a fork to me.

[mirh]

+1 for plugin independenecy
But Lua scripting is the best solution I can think of for "more intelligent" cheats/patches #1923

[ramapcsx2]

I think it's a cool feature, often requested as well.
Most of the work is in separate files, but where it connects with PCSX2, it is quite invasive (SIO, counters, many new hotkeys, it changes some old constants, etc).

Is there an effect on performance or stability?
Is there a way to completely turn this off, via build system or some user option?
Will this be maintained in the future?
Can this work without modifying plugins?

[xTVaser]

Pulling this out into a plugin is a great idea and something that I thought of after making this PR. But to be completely honest I don't have the time to dedicate to both doing that work and understanding how to intercept controller inputs without being invasive. But if thats a deal breaker and unless someone else wants to take on that work it will have to remain in it's fork.

There aren't that many new hotkeys, there are 3 important ones and I guess I could remove the savestate ones on the numpad if that is deemed too much. From what I understood that wouldn't be a breaking change as a user's existing ini would overwrite the new keybindings if applicable.

I've seen no issues myself on performance or stability but of course this is a huge testing surface so no guarantees.

I don't think you could turn these features "off", as mentioned, they are invasive in some spots. However it requires a user action to begin recording / using the lua engine. So problems should only occur once that happens

From what I understand, the only plugin that these changes touch is the snapshot features in GSdx, which from what I can tell is completely unrelated to the recording/lua components and could be reversed if thats an issue?

I will try to maintain these changes time permitted, my main priority is the recording functionality and the tooling for it. It's a bit rough in some spots and I have a list of TODOs in mind to make it better. It seems like the Lua scripting is what got the attention for the merge, but it definitely needs much more work and testing than the recording features. I would hope that means that other people would be willing to pitch into that work but if not, I'd have no problems gutting the lua portion from this as the recording features are really all I use and would like to see in mainline pcsx2.

[mirh]

I don't think plugin independent meant "making a specific plugin for it" (about what, even?)

But having the thing inside core, and adjusting plugin interfaces if really required.

"Turning off" the feature wouldn't be impossible then, with enough #ifdefs and a config setting (like gsdx sse4 or portaudio noasio).

Said this anyway, imo all of these features would be appreciated by all the devs here, at least in theory. The only actual blocker is polishedness. ^^

[ramapcsx2]

In my opinion, it can be "a little" invasive, if it's useful.

Let's see about GSDX:
It looks like the changes there are entirely for screenshots. What are those used for?
If it's just a nice feature for TAS stuff, then I would not put it into this PR, but maybe add it at a later time.

For the hotkeys:
Maybe the added functionality can be disabled by default. A menu option could then be used to enable TAS features, and activate the new hotkeys.
This helps separate legacy behaviour from new stuff, so guides / readme's all stay valid.
(And people won't accidentally dump hundreds of gigabytes of GS screenshots ;p)

@sudonim1:
Are you entirely against adding this stuff?
It is a feature some people wanted badly enough that they made their own fork.
It looks like this fork can be merged with PCSX2 proper, with the new stuff being mostly its own thing.
Having a new 3rd party library may bring in some new issues though.
Is there anything that can't be easily integrated?

[xTVaser]

I believe the screenshot changes were added just to add some additional convenience. A menu option was added to easily dump frames/audio instead of pressing F12 so I guess at the same time the previous author figured they would implement the same for screenshots (save as with file dialog option) and ran into some bugs. I would have to double-check to see if there was anything beyond that, but that could be broken out into it's own PR if so.

I can explore adding some kind of toggle for TAS/Lua features but it will take some time as that is a significant amount of code to wrap in config setting checks, also I'd have to look into how this is even accomplished. Looking at the examples mirh mentioned should help but I have zero familiarity at the moment.

I will wait and let this PR sit for a while before doing that work since it's only purpose would be to fit well into mainline pcsx2, but the possibility of these large changes being rejected is still there. As more people look at this PR, I imagine there will be a clearer direction towards getting this merged or a clear rejection (which I completely understand the hesitation to merge such large/unrelated changes), at which point I can move forward.

[willkuer]

Maybe it's just my c# background but for me these //tas comments in the code are smelly. It looks to me like a virtual call with inheritance is what you want to do. Just subclass all pcsx2 classes (sio/counter) and overwrite TAS-specific methods with your own implementations including the TAS-calls.

Like that you basically need a single ifdef/if per pcsx2 class in each consumer to clearly separate TAS and non-TAS code. Not sure how performance critical the code is and how much it will suffer from virtual method calls though.

[bositman]

I think it's a great feature to have on PCSX2. If people think its too intrusive on the GUI you can just add an option inside emulation settings to show or hide the TAS stuff.
About the movie recording menu, I'm all for it since it's hidden behind F12 almost no one knows it exists and it's a great built in feature for making gameplay videos with PCSX2.

About code standards and being invasive in PCSX2 code, I too would also like it to be as contained as possible as long as it is doable and doesn't need huge amounts of work.

Worst case scenario it could remain as a fork although personally I'd love the extra functionality in PCSX2 :)

[gocha]

I am welcome for having TAS-capable abilities to PCSX2 mainline, since I am a TASer. But I also agree for the opinion that the code is too smelly and invasive. In my opinion, it needs code cleaning. (I tried to make the code cleaner and stable but I was busy and lost motivation at last.)

I also planned to shrink the granularity of the pull request, in other words, to make some individual pull requests such as scripting core, input injection mechanism, movie support, etc. Isn't this change just too big and hard to review?

I wish PCSX2 will have a stable and sophisticated functionality for TASing in future anyways. Thank you devs for trying to make the first step for that.

P.S. I would be more happy if the movie format would be changed to text-based format, which is more extensible and readable.

[xTVaser]

I will clean up the code at the same time as making the changes to make it acceptable for merging, but in regards to having a smaller PR, you can't really make it any smaller other than removing the Lua code or vise-versa.

In my opinion it wouldn't make much sense to make a PR to upstream which adds TAS support that did not actually add TAS support, all of the underlying TAS code is required to make that happen. Maybe I'm flawed in that mentality but I see this as the initial spike that can be worked on in the future with reasonably sized PRs. Of course you could make an individual PR for just the input recording for example and stub everything else out...but that adds no value to PCSX2 on it's own

There is already an included tool that can be used to view (and I believe edit) the movie files in a human readable format. I would prefer to see this be worked on instead of making another breaking change to the movie file format. I'm not sure what you mean by extensible as the movie format has to always be tightly coupled with the underlying TAS code.

[xTVaser]

Went through and wrapped the invasive code that interfaces with the Recording/Lua tools in new EmuOptions settings that can be disabled / enabled via the System menu. These options are also wrapped in warning prompts explaining that these are experimental/works-in-progress features. Also cleaned up a fair amount of bugs and such along the way, more details are in the integration PR on the fork - xTVaser#15

In the next few days I will do a pass of the diff in this PR and see if there is anything I've missed, I'll update the PR description, and most of all I will split the Lua portion into it's own PR to make reviewing easier. While the +7K line diff looks intimidating for reviewing, keep in mind that 1/3rd of that is the addition of the standard lua C library and additional standard CMake scripts.

Hopefully the improvements I've made makes these large changes more acceptable for merging, and if changes still need to be made it should just be simpler refactors from here on out 🤞

[xTVaser]

The crashes that @bositman brought to my attention appear to have gone away with the removal of the Lua code, which I will look into before PRing those changes, at one point I got a stacktrace and it seemed related to initializing vif0.

This is ready for review

[mirh]

You may want to squash/annihilate the commits where you add lua in the first place then.

[xTVaser]

@mirh Maybe I'm just not confident enough with rebasing, but it looks like a terrible mess as this PR has merges with integration branches as well as pcsx2/master. The PR could just be squashed to a single commit upon merge.

If removing the first few commits which just append files is really desired then I guess I could remake the PR with a new branch and cherry-pick the correct commits. If all lua code wants to be removed from the commit history then I might as well just squash it to a single commit myself since it's a part of basically every commit here....although I'd really like to preserve my commit history as I have to maintain this lol

[avih]

The PR could just be squashed to a single commit upon merge.

It could, but it really shouldn't. As is, it's already 46 files and 3300 LOC, which will be a nightmare to review.

The best you can do is split it into meaningful and relatively independent and incremental commits to make it easier to chew - for whomever reviews it and later for anyone who looks at the code and wants to understand it and its evolution. If it gets in, it still has to be maintained and understood by others, and meaningful commits are essential, especially in such a big PR.

[xTVaser]

@avih I completely agree with you but if the intent is to rely on individual commits to review it, then the entire PR has to be rebased anyway as old commits do not reflect the current diff. I also agree with you that having contextual commit messages is important (why I don't want to squash away my recent changes).

Relying on individual commits for review purposes makes things very difficult from the contributor's standpoint....once reviewers suggest something and I change it, I then have to rebase the entire PR again to remove the original bad code to not confuse other reviewers (which also re-writes the entire commit history which will remove all comments and suggestions lol).

[avih]

I'll attempt to do the rebase on a test PR

You don't need a new PR for that. You should do that locally on your own system, in a branch, and once you're happy with the branch and its commits history, put it back at your master branch (I'm assuming that's where you're working on it right now) and force push - this will update this PR with the new history.

It may or may not be simple to edit the current commits - it depends how far they are from your final history setup you want to create. Depending on your git knowledge, if editing the commits is hard, your safest bet would be to backup your current branch, start working from scratch on PCSX2 master, and apply/create the commits from scratch.

This could be tedious, but it's important. I suggest though to leave it till you're happy with the overall commit.

Actually, I think you have it in a pcsx2-rr-squashed branch, so create a new branch, work on that, copy the work back to pcsx2-rr-squashed and force push. But again, leave it as the last step.

[xTVaser]

@avih Done, I hope this is separated and isolated enough to make this acceptable for reviews.

I edited my previous message, I am still concerned about the following:

Relying on individual commits for review purposes makes things very difficult from the contributor's standpoint....once reviewers suggest something and I change it, I then have to rebase the entire PR again to remove the original bad code to not confuse other reviewers.

I would hope that the individual commits make things easier to digest, but that comments are placed on the overall diff, so that I do not have to continually force-push away old code and automatically dismiss reviews.

Simple example to illustrate my concern: reviewer suggests to change the name of a variable...all previous commits use the old variable name which causes confusion / other reviewers to make the same suggestion because they aren't cross-referencing the overall changeset.

[mirh]

If rebasing gives you shivers (understandably), it's not like you couldn't just work in a new branch... And then make a new PR altogether.
It may sound.. rude, but there's really no shame in presenting as best as you'd think a PR this big (probably in the top 3 since we switched to github?)

Not necessary anymore.

[MrCK1]

I'm ready for this to be merged unless anybody else has other comments/suggestions. We'll just have to bump the savestate version after merging.

[lightningterror]

Num pad will probably be used for input bindings by people that don't use a controller so it's not a good idea to bind these, same goes for space.

Do we need to have hotkey bindings for saveslots ?

[xTVaser]

For the purposes of recording, yes they are definitely required as saving and loading save-states is done incredibly frequently (as is frame advance). These are just the default settings, if someone were to bind numpad/space wouldn't they already have duplicated and modified the PCSX2_keys.ini.default file? I'm guessing the controller keybindings do not take precedence / detect conflicts and that is the bigger issue.

I can remove the defaults and add documentation somewhere on how to bind the recording keybindings, or leave them commented out in the same file?

As an aside, is the only way to modify non-controller keybindings in PCSX2 via the ini file? I actually never noticed but looking around the UI right now I see no other way. Would adding some sort of UI menu to change the keybindings be considered a valuable contribution?

[lightningterror]

As an aside, is the only way to modify non-controller keybindings in PCSX2 via the ini file? I actually never noticed but looking around the UI right now I see no other way. Would adding some sort of UI menu to change the keybindings be considered a valuable contribution?

That would be a great feature, it's just finding someone willing to add it :)

These are just the default settings, if someone were to bind numpad/space wouldn't they already have duplicated and modified the PCSX2_keys.ini.default file?

Lilypad has it's own ini config so hotkeys can actually become duplicated.

[mirh]

If somebody's doing a TAS, I would assume it's not really any more interested into normal gaming
So... How about "enable recording tool" making lilypad use a separate .ini?

[xTVaser]

A separate .ini file is a good idea, although I believe you meant make pcsx2 use a separate file? We aren't overwriting any controller keybindings, just emulator ones (savestates and new custom ones).

In the meantime and to keep things moving, the latest commit will enable / revert the recording keybindings while preserving whatever custom overrides a user has setup in the .ini file (tested and everything appears to work as expected).

Setting the bindings conditionally is pretty easy, the tricky part I found was reverting and restoring the original bindings if they disabled recording. Right now everything is centralized in the PCSX2_Keys.ini file or in code so to pull a portion of the bindings out into their own config file, might be a larger change (case in point, Lilypad's ini will overwrite). So I'm sure there's a more elegant solution than mine, but I think I'd be best to sit on it and think about it, currently low on ideas.

[MrCK1]

Is there anything else we're waiting for on this PR? I'd like to see it get merged soon :) @lightningterror @xTVaser

[xTVaser]

@MrCK1 From my perspective there's nothing blocking it from being merged. The sooner it gets merged the sooner I can start to make things better with much more reasonably sized PRs. I imagine that it will need to be tested again before merging, and if anything comes out from that testing I'll try my best to address it as soon as possible.

[lightningterror]

Can you add a commented out define for DISABLE_RECORDING just so we know where to change it if we want to disable the functionality. Bonus for adding a comment next or above the define explaining what it does.
Edit: the above can be done on a later PR since it's not a big issue and it's more of a nitpick.

Anyway was linux behavior tested? Linux has gsdx options for recording and may need to be updated according to this pr.

pcsx2/plugins/GSdx/Window/GSLinuxDialog.cpp

Line 548 in 5c23a9f

void populate_record_table(GtkWidget* record_table)

[refractionpcsx2]

After that change has been made, can we merge this please, I want this over and done with. It has been tested quite extensively and it's fine, these changes being asked are pretty petty

[lightningterror]

Ok let's merge the initial pr, it's been delayed enough as it is :P
Other improvements can be done after.

[ramapcsx2]

Wee! :)