Reimplement Cheat Search.

[AdmiralCurtiss]

Reimplements the Cheat Search functionality from scratch. The previous variant had all kinds of issues ranging from results randomly disappearing to only working in MEM1. This obsoletes the old #8311 PR.

Fixes:

• https://bugs.dolphin-emu.org/issues/10524
• https://bugs.dolphin-emu.org/issues/11176
• https://bugs.dolphin-emu.org/issues/11434

Note: I removed the value freezing feature because it was completely broken, even when reimplemented in this PR, due to PowerPC::debug_interface.SetPatch() only applying the value once instead of continuously. It probably needs to be implemented in terms of Gecko codes to actually work as you'd want it to.

[Dentomologist]

The basic functionality looks good here, minus a couple crashing issues that should be easy to fix.

There's a lot of complexity in the code in this PR that stems from the need to pass around and validate parameters repeatedly, as well as performing similar operations on different data types. Given that each CheatSearchWidget has a fixed data type, alignment, and address range, I'd like to propose the following changes for discussion:

• Convert CheatSearch into a class templated with the data type it's searching for (the actual data type, not Cheats::DataType), and pass the validated address range and data alignment into the constructor. Unlike most templated classes, declare them in CheatSearch.h but define them in CheatSearch.cpp. To avoid linker errors, at the bottom of CheatSearch.cpp put template class CheatSearch<u8>; and so on for each of the 10 data types. This ensures CheatSearch can only be created with an expected data type.
• Ideally we could add a CheatSearch<data_type>* to CheatSearchWidget but Qt classes that use signals or slots can't be templated and we'll have use for those. Instead we could create a CheatSearchBase class from which CheatSearch<data_type> derives, then take a Cheats::DataType parameter in CheatSearchWidget's constructor and switch on it to initialize CheatSearchBase* m_cheat_search with the appropriate CheatSearch type (Or validate and create the CheatSearch in CheatSearchNewWidget and pass that to the CheatSearchWidget instead).
• Once CheatSearchWidget constructs m_cheat_search it should no longer need to know what data type CheatSearch is using, so any data values CheatSearchWidget deals with should be in string form with CheatSearch doing the converting from and to strings as needed. CheatSearchWidget can still handle addresses numerically since those have a fixed type.

This has multiple benefits:

• Further separates search logic out from CheatSearchWidget, which no longer needs to store and pass the search parameters and full list of results and instead only keeps the displayed address table.
• Removes a lot of extra type checking and validation.
• Makes it simple to use functors like std::less when filtering values
• Reduces the amount of code in the inner scan loop, which should improve performance to some degree.
• Allows SearchResult to not need to store the DataType, reducing memory usage.

Other code comments:

• CheatSearchNewWidget is easy to get mixed up while reading code, especially when considering signals generated by a new CheatSearchWidget. Maybe rename CheatSearchNewWidget to CheatSearchWidgetFactory?
• If you have cheat search open, stop the game, create a new cheat search, then start a scan Dolphin crashes.
• Unless I've overlooked something variables tracking the translation state are ultimately only used to see if the result is valid, which means you could replace SearchValueWithTranslatedFlag with an optional and remove SearchResultValueState from SearchResult.

UI comments:

• The Last Value column in the address table shows values from the most recent filtering and Current Value shows values from the most recent click of Refresh Current Values, with no connection between them. My initial expectation was that Current Value would show the most recent result of either operation and Last Value would show whatever had previously been in Current Value; the actual behavior feels unintuitive to me.
• In the ValueRefType pulldown "unknown value" confused me at first; I thought it was going to refer to some type of unreadable memory state. I would use "any value" instead.
• In the ValueRefType pulldown the capitalization is inconsistent.
• Once a CheatSearchWidget is created it doesn't say what address range, data type, and alignment it's using. If you haven't changed them in the meantime you could check the factory tab, or you could infer them from the address table, but having a small listing of the settings would be nice.
• For someone who's not very familiar with Dolphin it won't be obvious that the Cheats Manager can only be opened when the Enable Cheats box is checked and a game is currently running. I think you should always be able to open it, and if the requirements aren't met have a blank widget describing what the user needs to do. A second "Enable Cheats" checkbox (synched with the config one) right there would be even more convenient, but would be inconsistent enough with other settings that it's probably not worth it.
• This is easier to see in debug mode, but the AR and Gecko code tabs can be missing momentarily when you first start up the Cheats Manager. Having them always be present but disabled when appropriate would be better.
• Refresh Current Values doesn't show how many addresses remain.
• It would be nice to be able to view values in hex.

[AdmiralCurtiss]

You make some very good points and I'll see whether I can get this stuff implemented. Making the backend class template-by-search-type seems like a very good idea in particular if we can get it to work.

Some notes though:

The Last Value column in the address table shows values from the most recent filtering and Current Value shows values from the most recent click of Refresh Current Values, with no connection between them. My initial expectation was that Current Value would show the most recent result of either operation and Last Value would show whatever had previously been in Current Value; the actual behavior feels unintuitive to me.

This last/current value concept is modelled directly after CheatEngine, which I assumed most people would be familiar with. One column shows the result from the last scan, the other shows the current value in memory regardless of when you last scanned. Of course, CheatEngine has the advantage here of being able to constantly read the memory in the background and refresh them automatically, which may be something to strive for in the future as well if we can get it working without affecting the emulation. I suppose at the very least we should refresh the Current values on scan too, yes.

In the ValueRefType pulldown "unknown value" confused me at first; I thought it was going to refer to some type of unreadable memory state. I would use "any value" instead.

This is also from CheatEngine, though I just checked and that specifically calls it 'Unknown initial value'. YMMV on the terminology here, I'd be fine with 'any value' too.

For someone who's not very familiar with Dolphin it won't be obvious that the Cheats Manager can only be opened when the Enable Cheats box is checked and a game is currently running. I think you should always be able to open it, and if the requirements aren't met have a blank widget describing what the user needs to do. A second "Enable Cheats" checkbox (synched with the config one) right there would be even more convenient, but would be inconsistent enough with other settings that it's probably not worth it.
This is easier to see in debug mode, but the AR and Gecko code tabs can be missing momentarily when you first start up the Cheats Manager. Having them always be present but disabled when appropriate would be better.

Both of these are pre-existing issues that aren't really related to the Cheat Search feature directly. I agree they are bad UX and should be fixed, but a separate PR (or two) is likely the better place for them.

Regarding the signal handling and close button and stuff, frankly I wasn't quite sure how to handle that sensibly, Qt kinda didn't want to cooperate with me here likely because I'm just not very familiar with how Qt expects it to be handled, so this is how it ended up. Definitely some room for improvement there.

[Dentomologist]

This last/current value concept is modelled directly after CheatEngine, which I assumed most people would be familiar with. One column shows the result from the last scan, the other shows the current value in memory regardless of when you last scanned. Of course, CheatEngine has the advantage here of being able to constantly read the memory in the background and refresh them automatically, which may be something to strive for in the future as well if we can get it working without affecting the emulation. I suppose at the very least we should refresh the Current values on scan too, yes.

Really we would only need to update the current values for whichever rows of the table are currently visible in the scroll frame (at most a few dozen), so I'm sure it's possible to get real-time updates without a noticeable effect on framerates. That said it's not a blocker for this PR, so if it's not something you can add easily then refreshing current values on scan would be enough.

Both of these are pre-existing issues that aren't really related to the Cheat Search feature directly. I agree they are bad UX and should be fixed, but a separate PR (or two) is likely the better place for them.

Works for me.

[AdmiralCurtiss]

@Dentomologist: Haven't fully updated the GUI code yet, but I've rewritten the backend based on your suggestions. I think it turned out quite nicely!

[AdmiralCurtiss]

Alright, I hopefully addressed almost everything now, and indeed, I think the code is way cleaner now. Maybe I'll add a few more comments here and there but otherwise I consider this done.

Stuff not addressed that I would like to push to follow-up PRs:

• The GUI weirdness where the window is inaccessible if Cheats are disabled.
• The AR/Gecko tabs appearing and disappearing based on emulation state.
• Auto-updating the Current values.

[JMC47]

This is a HUGE improvement even at a glance, with the old functionality being so broken that there's little reason to delay this.