Prior RAM search workflow is broken in 2.9.2 dev

[RetroEdit]

I don't know when this was broken, but I was trying to test an unrelated bug, and RAM search doesn't seem to be working properly in the latest dev build ( ab7f379e745bb241e537ee6fe0212f98ab421dad ).

Specifically, Compare To: Previous Value with Comparison Operator: Not Equal To dismissed all the addresses even if you visibly observe ones that have changed in the hex editor.

Tested with the game Libbet and the Magic Floor v0.07 for GB, which is my go-to testing game because it's homebrew. I moved the character, which should change some address's values (specifically: 0400, 0404, 0408, 04EB, 056F, 0594), but all the addresses were removed from the search as if none of them had changed.

I would venture to guess it's just broken in general, but that's the extent of my testing so far.

---

Okay, I think I've figured out the deal: 1a9e5e52f1f12cab55348c67eca8a9bb20d49329 (#3954) says the following:

• removes PreviousType.LastSearch due to its questionable use case

I think this was the prior default? @Morilli care to explain the rationale here? To me, an intuitive search methodology is to anchor the list of addresses when you start RAM search by pressing new, then doing an action and searching for addresses that change as a result of that action. And then do another action that changes the state and iteratively do that until you've narrowed down to the most relevant addresses.

I don't think the other RAM search options supply the relevant functionality for effective repeated searches of this nature, so removing this option seems to reduce RAM search's usability. At the very least, Previous Frame seems like a bad default to me because it's only useful in a frame-by-frame context, not real-time.

[YoshiRulz]

PreviousType.LastChange should be a suitable replacement.

[RetroEdit]

I tried that, and I don't believe it actually serves as a proper replacement: it doesn't discern the vital detail that the value is actually different. Values can merely change coincidentally during the time you do an action (or be relevant during, but not after), making the search less discerning when you're looking for what's different at the end of an action.

[Morilli]

What you can do is set the PreviousType to original, and update the previous values manually with the UI button after every search. How often do you expect to need to search the value? I feel like it's not necessary to have this functionality builtin.

[RetroEdit]

Using the <- "Copy Value to Previous" button in combination with setting PreviousType to "Original" sort of works. It's less convenient, because it's easy to accidentally forget to press the button, and now it's split what was previously one synchronized action of updating search into two discrete operations that you can't do simultaneously unless you pause.

Is getting rid of PreviousType.LastSearch a notable performance improvement, then? I guess performance can matter for larger memory domains in certain systems, but usually that's mitigated after the first search. For me personally, I value LastSearch greatly and have made repeated use of it.

Also, to me Previous Frame still seems like a bad default. In fact, by the same logic you just put forth, previous frame doesn't seem like a necessary option -- I actually struggle to come up with a use-case where you would want to check the previous frame, but don't know you're checking the previous frame in advance. If you're just inferring address changes by visual changes, that address could have changed a few frames ago because of graphics delay. Technically maybe it's useful if you're searching for an address that you think is dependent on existing addresses you're observing change, but that requires a lot of assumptions.

[YoshiRulz]

I'm in favour of a revert or partial revert.

[Morilli]

The main reason I didn't like the previous search option is because it made the search button inconsistent: Pressing it twice could (in many cases) remove all addressess, which is never useful.

What's the actual use case for finding many addresses that may change at different points? I understand not knowing the exact frame a change happens, but wouldn't that be covered by the Original previous type? You start a search at a certain point, advance some frames and then check what addresses have changed.

[YoshiRulz]

What you're describing is really the quintessential RAM Search workflow: "Not Equal To" "Previous", make some change in-game, press the cull button, GOTO 20.

[Morilli]

I don't know, maybe I've never had to search that much to find repeating this process often useful. I just dislike that you lose information with this method: once you click search, all watches have the same previous and current value, even though you just searched for all addresses that have differing previous and current values!

PreviousType of last search is effectively either the same as last frame (when auto-searching) or as original + clicking the button. And as I've just explained I feel like it's wrong or at least unexpected when the current->previous button is "implicitly" pressed.

[RetroEdit]

I can appreciate additional elegance and simplicity in the design. RAM search is a powerful and versatile tool, but I do remember being confused at various points trying to use it. And I didn't even remember auto-search being a thing until you mentioned it; not sure if I've ever used it.

That said, I would still appreciate some kind of toggle along the lines of "Update Previous value after each search" if Last Search is being removed as a PreviousType (which may defeat the point or be worse, so I don't know).

[YoshiRulz]

Whether it's the "correct design" doesn't really matter when it's been the default for years, I believe dating back to FCEUX' implementation. You can't just remove the option without fanfare. Reverted.

I've also reverted 542e043261012f6271cadbcd2251f073a31cbd3e since it was more convenient and I don't like the idea of uint for signed quantities.

Per https://github.com/TASEmulators/BizHawk/pull/3954#issuecomment-2178168159 I'd like to review the whole changeset. Please rebase it into separate commits.

[Morilli]

Not sure if it was necessary to revert the full commit including refactor, I could have just added the functionality back...

I still don't really understand the convenience of that option, and I have explained how I as a first-time user found its behavior unintuitive.

Whether it's the "correct design" doesn't really matter when it's been the default for years, I believe dating back to FCEUX' implementation.

This sounds a lot like "don't bother correcting incorrect stuff, it's been this way for years", which I disagree with. If this is a default for good reason, fine, but I find a previoustype of last frame to be much more natural, especially as it won't show different behavior between auto-search and manual search.

You can't just remove the option without fanfare

I did make a PR specifically because I wanted to allow for feedback and I put the main changes in the PR description. You even commented on it and seemed to approve, so I merged it. Everything in that change was very inter-connected, so splitting it up into multiple commits would have generated much more work, like moving stuff and making it work with some changes just to remove all of it in the following commit.

I don't wanna break anyone's workflow, just make improvements. I feel like this can be resolved in a better way than by just blindly reverting commits.

[YoshiRulz]

You can't just remove the option without fanfare.

I did make a PR specifically because I wanted to allow for feedback and I put the main changes in the PR description. You even commented on it and seemed to approve, so I merged it.

By "without fanfare" I mean without indicating to the user in some way that the default has changed.

Miscommunications are inevitable so let's not worry about my PR comment.

Everything in that change was very inter-connected, so splitting it up into multiple commits would have generated much more work, like moving stuff and making it work with some changes just to remove all of it in the following commit.

This is the real problem. Consider that more, smaller commits also means smaller (and easier) reverts.

I still don't really understand the convenience of that option, and I have explained how I as a first-time user found its behavior unintuitive. [...] If this is a default for good reason, fine, but I find a previoustype of last frame to be much more natural, especially as it won't show different behavior between auto-search and manual search.

I feel like I say this too often, but: you seem to be alone in that.

Whether it's the "correct design" doesn't really matter when it's been the default for years, I believe dating back to FCEUX' implementation.

This sounds a lot like "don't bother correcting incorrect stuff, it's been this way for years", which I disagree with. [...] I don't wanna break anyone's workflow, just make improvements.

something something xkcd

Of course you can change things, the Hawk really needs change. Just be more careful when removing features which were the default. I only brought up the feature's age as a way of saying that people are used to it.