Revert "add memory hooks for cheat/rumble"

[leiradel]

Reverts libretro/81-libretro#8

The amount of system memory reported is wrong.

[RetroSven]

@leiradel please double check that - I don't think there is anything wrong here

[leiradel]

The ZX81 comes with either 2, 16, or 48 Kb of memory depending on the model, but never 1 Mb. Maybe this was done out of laziness, or maybe the excess memory is used some other way by the emulator, I don't know I only ported it, but it can change in the future. We should never report to the frontend memory that doesn't exist in the emulated system.

Also, your code returns 16 Kb of ROM as if it was system RAM.

[RetroSven]

The motivation for getting this data back to RA is for the cheat system. It does not matter what the contents of the memory pointer are (e.g. ROM, etc) as long as it is a superset of the memory that can be searched and poked. Some people may actually make novel use of the fact that ROM is included.

The frontend is only receiving information that does actually exist per the code. There won't be any buffer-related errors.

wrt changing in the future - there has been no real movement on this branch for over a year. Is change really a concern? If we go by that logic, then anything that's coded for returning memory could be considered invalid because it could change. If something in the emu related to memory does change in the future, then whatever is in here (regardless of what it is) will likely have to change. That's not a big deal. In the meantime the end-users can enjoy the feature.

[leiradel]

I'm strongly against hacking the memory API for other uses. If you need to access memory, report it correctly. ROM is not system RAM and should not be reported as if it were, and the ZX81 does not have 1 Mb of RAM.

If the retro_get_memory_data/size interface is not the correct one for this functionality. Maybe the cheating system should use the RETRO_ENVIRONMENT_SET_MEMORY_MAPS interface.

I don't think a feature is an excuse to implement the API incorrectly.

[RetroSven]

Regardless of how much memory the ZX81 actually has, that's the size of the buffer variable that's named "memory". It is a buffer for the entire emu system's RAM - whether it's a zx81 or some other machine or a combination of the emulated system's RAM and other storage that the emulator itself needs. The word "system" in "system RAM" could mean the emulated system or the emulator system.

What's the harm here? My takeaway from this is that you would prefer to deny users a feature because of some principle that has no actual repercussions.

[leiradel]

Regardless of how much memory the ZX81 actually has, that's the size of the buffer variable that's named "memory".

You're letting a core implementation detail trick you into a wrong memory hook implementation.

The word "system" in "system RAM" could mean the emulated system or the emulator system.

While it's true that the memory API has limitations, hence the memory map API, the intent has always been to give the frontend access to the emulated memory, and not to the cores' internals.

What's the harm here?

Imagine in the future someone needs access to the System RAM to implement some functionality. Imagine the time spent figuring out why things don't work and the surprise to find that the System RAM actually starts with the ROM. Imagine the time spent looking for interesting values in the System RAM after address 65535 up to 1 Mb only to find out that there's nothing valuable there. Those are not hypothetical scenarios, the RetroAchievements tooling already looks into the cores' memory regions to implement the achievements and leaderboards.

Again, it's because of correctness. The memory hooks don't exist so we can hack around the cores, they exist to give the frontend access to the different types of emulated memory, nothing more. If this interface doesn't suit some requirement, there's the memory map interface. If that's also not enough, a different API must be pitched to the team and implemented.

There are other things being cooked that will also require access to the emulated memory. Lets just do what's right and avoid having to work around previous wrong implementations that will be difficult to fix because other stuff already use them.

[RetroSven]

You're letting a core implementation detail trick you into a wrong memory hook implementation.

I'm not being "tricked". I'm addressing what's actually available in reality - not in theory.

the intent has always been to give the frontend access to the emulated memory, and not to the cores' internals.

That is your opinion. It is not a statement of fact. You may want it to work that way, but there are legitimate use cases for accessing emulator system memory.

I appreciate you trying to come up with an example, but you're going to have to do better than that.

Imagine in the future someone needs access to the System RAM to implement some functionality.

(emphasis mine) You need to provide a real example - not ambiguous "some functionality".

Yes I'm aware that system ram is used for the leaderboards. What's being returned is a superset and the leaderboards will function just fine with the original code. Everything you've mentioned either is hypothetical or not applicable (i.e. not an actual problem).

There are other things being cooked that will also require access to the emulated memory.

What are those things? How would any of those things (in reality) produce any issues with the original code?

[leiradel]

That is your opinion.

Again, learn about the API you want to implement before implementing it. Don't work with assumptions.

there are legitimate use cases for accessing emulator system memory.

I'm sure there are, but this API is not the correct one for that. Pitch a new API to hack into the cores' memories and I'll gladly merge a PR that implements it.

What's being returned is a superset and the leaderboards will function just fine with the original code.

No because the addresses would be all wrong.

What are those things? How would any of those things (in reality) produce any issues with the original code?

Sure, this thing here: https://gist.github.com/leiradel/a04e85df397d65a6ae12617c23339b13

It's still very draft, but it'll need access to the emulated memory, and I'd rather have the cores implement the correct behavior than to have to write workarounds because they return what they're not supposed to.

You know that an API is a contract with its user, right? If the API doesn't work according to the spec, the user will be frustrated, and will have to work around wrong implementations.

[RetroSven]

Thank you - that's a legitimate use case and I actually agree with you now wrt addressing.

In the future, however, may I suggest discussing things with new submitters before just wiping out commits? No one is disputing that you don't have the authority to do so, but if we had just talked about it first it would have gone over a lot smoother. Honey vs vinegar and all that.

[inactive123]

Thankfully this conversation seems to have gone down much better than the other one. I hope this is a sign of better communication going on between you two so that we can actually arrive at a solution that both camps are happy with.