search

TASEmulators / BizHawk

BizHawk is a multi-system emulator written in C#. BizHawk provides nice features for casual gamers such as full screen, and joypad support in addition to full rerecording and debugging tools for all system cores.
http://tasvideos.org/BizHawk.html
Other
2.21k stars 385 forks source link

Refactor RamSearch to increase speed and improve functionality #4004

Closed YoshiRulz closed 2 months ago

YoshiRulz commented 3 months ago

(recreation of #3954, as per discussion around https://github.com/TASEmulators/BizHawk/issues/3999#issuecomment-2289237080)

Morilli commented 3 months ago

Looks generally fine even though the individual commits probably don't compile. From the discussion in #3999 I'm assuming 97adf1a should not be included now?

YoshiRulz commented 2 months ago

Some of this was merged as https://github.com/TASEmulators/BizHawk/commit/3c218c664205785691d1e2c0fc87402fbdfb9384 through https://github.com/TASEmulators/BizHawk/commit/f09e72e7e0397e30cf45a4d76006ef7efdb30ca2, plus https://github.com/TASEmulators/BizHawk/commit/ea1dcf5b22d9602643f9383ff63876950686e074. edit: and https://github.com/TASEmulators/BizHawk/commit/3b1ce44fb505af51b99faab4d5e2ffa4eb22c1e5 through https://github.com/TASEmulators/BizHawk/commit/92724aee0b6adac39f7756b1fcda70e43a3e49b5.

Morilli commented 2 months ago

Looking at it again and I can't seem to be able to properly separate the changes more than in https://github.com/Morilli/BizHawk/tree/xd. Main commit is 3a63ec7da8395837b14eb82fd959df2c83d7a46c, I've tried to add a decently sized explanation to it but I feel like there's no natural way to deconstruct that commit any further while keeping the commits logical.

To be fair the commit looks bigger than it is; lots of GetValue(w.Address) -> w.Current and rewrite of the Mini*WatchDetailed classes.

YoshiRulz commented 2 months ago

It looks like you already have it broken down into smaller conceptual pieces in the commit description?

Morilli commented 2 months ago

cache Current

Actually, that seems to already be done in the detailed watch, the variable is just called _prevFrame and only used internally.

replace GetValue with Current

That can't be done until Current has a meaningful value at all times. And even if that is done, you still need to query the value for the non-detailed watches or update their code to store the current value as well (making them an exact copy of the detailed implementation, just without the ChangeCount++ line).

(this has just come to me: can you make the WatchDetailed classes extend Watch? if so, do that at this point or maybe first)

Yeah, reasonable. But again feels like wasted effort when the classes are to be merged anyways.

move IMiniWatchDetails features up to IMiniWatch

At this point, that is effectively only the change count logic. That is the only remaining difference between the detailed and non-detailed watch.

Morilli commented 2 months ago

The parts currently in master should already resolve the slowdown incurred from MiniWatchDetails, so this change is not really necessary anymore. There's more that could be done, but the main goal with my changes was to make the detailed watch type not as stupidly slow as it was and fix some issues in the process, and I believe all of that is in master already.