barbudreadmon / fbalpha-backup-dontuse-ty

Deprecated port of Final Burn Alpha to Libretro (v0.2.97.43).
61 stars 43 forks source link

Cheevos support #182

Closed leiradel closed 5 years ago

leiradel commented 6 years ago

Hi @barbudreadmon

I took a look in the source code and this core doesn't support achievements. Is it possible to implement the RETRO_ENVIRONMENT_SET_MEMORY_MAPS environment call, or the retro_get_memory_data/size functions?

barbudreadmon commented 6 years ago

After looking at other cores, it seems to me like it's somehow relying on the same data than savestates. It could be a serious issue considering this data can change a lot from a release to another. Someone can confirm ?

andres-asm commented 6 years ago

It's not really the savestate data, but the SRAM data.

retro_get_memory_data/size aren't a good fit because those force SRAM saving to work via retroarch. Also for either case it would be a pita to map everything properly to all the supported boards. It could be done for a subset I guess, like CPS1, CPS2, CPS3, NeoGeo, MD.

For instance I know to handle saving in CPS2 we hacked the eeprom file so it saves that file in RETRO_GET_SAVE_DIRECTORY

With those functions we would instead get a pointer to the memory that should be saved and RetroArch would handle saving.

barbudreadmon commented 6 years ago

I think "BurnAreaScan(ACB_MEMORY_RAM)" handle saving only the sram data, perhaps @dinkc64 could confirm ?

@fr500 i removed the hack in the eeprom file a few months ago, it wasn't needed anymore.

leiradel commented 6 years ago

WRT achievements, most of them rely on RAM alone, but some may need SRAM when available. However, they never depend on savestates, just on being able to access RAM and SRAM.

Also, all the RAM must be accessible, i.e. some systems have some "quick" or "page 0" RAM that is easier/faster to access, and more RAM available on a different address. Both have to be mapped to enable achievements.

On systems where there's just one contiguous block of RAM, implementing retro_get_memory_data/size for RAM and SRAM is enough to support achievements. On systems with more complex memory layouts, RETRO_ENVIRONMENT_SET_MEMORY_MAPS must be used to map all the necessary block of memory.

Also, it's mandatory that the pointers returned and/or registered are stable between retro_load_game and retro_unload_game, since they are cached by the achievements code to speed up things.

leiradel commented 6 years ago

Forgot to say that, event for simple memory layouts where retro_get_memory_data/size can be used, it's preferable to use RETRO_ENVIRONMENT_SET_MEMORY_MAPS because they give more information about the memory than just it's pointer in the host memory and its size.

andres-asm commented 6 years ago

@barbudreadmon well thing is, if we use retro_get_memory_data that enables the frontend's mandatory handling of sram, which could be good I guess, as long as BurnAreaScan(ACB_MEMORY_RAM has all the data we need. Then we can remove the I/O based save handling.

The RETRO_ENVIRONMENT_SET_MEMORY_MAPS API is better for cheevos imho.

leiradel commented 6 years ago

We can have both.

andres-asm commented 6 years ago

Yes but we don't want to have two methods of saving persistent game data (we already save without retro_get_memory_data)

leiradel commented 6 years ago

I don't know anything about that, my request is to add achievements support

barbudreadmon commented 6 years ago

Answer from dink :

ACB_MEMORY_RAM is just for the ram, ACB_NVRAM is for nvram If you're lucky, the main ram will be the first block at the beginning of ACB_MEMORY_RAM. some drivers might save the ram in the driver data (ACB_VOLATILE). due to the fact that none of this was ever standardized..

leiradel commented 6 years ago

If you're lucky, the main ram will be the first block at the beginning of ACB_MEMORY_RAM

Does it mean there could be more than one block of RAM, and that there's no way to tell if that's the case?

barbudreadmon commented 6 years ago

Well, an arcade machine has several block of ram, check the cps2 specs : https://en.wikipedia.org/wiki/CP_System_II In the best case scenario, the *scan functions of the cpus and devices related to a driver will handle BurnAreaScan(ACB_MEMORY_RAM) properly and it will save all the rams and only the rams.

Actually the same can be said about console : some of them (if not all ?) had main ram + video ram + sound ram. Cheevos only needs main ram or it also needs video/sound ram ?

andres-asm commented 6 years ago

I don't know anything about that, my request is to add achievements support @leiradel I understand, but what I'm saying here is that the implementation should use the MEMORY_MAPS API, not the other one because it would introduce saving issues otherwise.

leiradel commented 6 years ago

Cheevos only needs main ram or it also needs video/sound ram ?

Only main and save RAM, although all memory visible to the main CPU could be mapped via RETRO_ENVIRONMENT_SET_MEMORY_MAPS, including ROM.

Memory visible only for other CPUs can't be made visible because libretro doesn't have an API for that.

leiradel commented 6 years ago

I understand, but what I'm saying here is that the implementation should use the MEMORY_MAPS API, not the other one because it would introduce saving issues otherwise.

Ah I see. Sure, I'm not saying what API should be used. If retro_memory_get_data/size will introduce issues, RETRO_ENVIRONMENT_SET_MEMORY_MAPS is the way to go.

barbudreadmon commented 6 years ago

Ok, let's take an example : the neogeo driver.

At line 278 of src/burn/drv/neogeo/neo_run.cpp i think there are all the things you need :

NB : lines 275 and 283 are the 3 video rams => Unneeded ?

You can get Neo68KRAM by getting the first 65536 bytes of BurnAreaScan(ACB_MEMORY_RAM), and you can get NeoNVRAM (+ NeoNVRAM2 if present) with BurnAreaScan(ACB_NVRAM), it should be 65536 bytes (+ 8192 bytes for the second one if present)

Also, the proper way to detect if you are running a neogeo game should be this boolean : (BurnDrvGetHardwareCode() & (HARDWARE_PUBLIC_MASK - HARDWARE_PREFIX_CARTRIDGE)) == HARDWARE_SNK_NEOGEO

I hope this is enough information if someone want to try and implement this on neogeo as a first step, feel free to ask if you need more information about the codebase.

leiradel commented 6 years ago

Neo68KRAM is main processor ram => Needed ?

Needed.

NeoZ80RAM is co-processor ram (should be sound) => Unneeded ?

Not needed, though I wonder if they could enable some achievements that would be otherwise impossible to make without it. Anyway, there's no way to report memory for CPUs other than the main CPU, unless this memory is mapped to the address space of the main CPU. In this case, they could be reported via the RETRO_ENVIRONMENT_SET_MEMORY_MAPS interface.

NeoNVRAM is the non volatile ram so i guess it is the "save ram" you mentioned => Needed ?

It depends. If it just holds a real time clock or settings, no, it's not needed for achievements, but could be reported via RETRO_MEMORY_RTC so its state is maintained in disk.

NeoNVRAM2 is the second non volatile ram which is used only by gambling games => Needed ?

Are there dumps of gambling games ROMs? If yes, maybe it's needed, depending on what's in it, but we can only report one NVM via RETRO_MEMORY_RTC. Everything can be reported using RETRO_ENVIRONMENT_SET_MEMORY_MAPS though. However, only the one reported via RETRO_MEMORY_RTC will be preserved by RetroArch.

NeoMemoryCard is the memory card => Unneeded ?

Is this where savegames are stored? If so, then it's needed.

NB : lines 275 and 283 are the 3 video rams => Unneeded ?

Same thing as the Z80 memory, considering this memory belongs to a VDP.

you can get NeoNVRAM (+ NeoNVRAM2 if present)

Ah, so they contiguous in memory when NVRAM2 is present? Good.

it should be 65536 bytes (+ 8192 bytes for the second one if present)

How can I be sure? RetroArch needs a pointer to the beginning of the memory, and its size.

Thanks for the help.

barbudreadmon commented 6 years ago

Yes i think nvram is for dipswitch settings, and probably hiscores and rtc. There are a few gambling game on neogeo (V-Liner, Jockey GrandPrix) The role of the memory card is kinda unclear to me, you can read about it there : http://www.neogeoprotos.com/memcardfaq.htm You can check if the game is a gambling game with ((BurnDrvGetHardwareCode() & HARDWARE_SNK_CONTROLMASK) == HARDWARE_SNK_GAMBLING)

barbudreadmon commented 6 years ago

Also, BurnAreaScan won't do since you need a pointer. Actually, if you can write some callback function which can be called with the right args (pointer, size) from the driver, and handle reporting to RETRO_ENVIRONMENT_SET_MEMORY_MAPS, i think it would help a lot.

barbudreadmon commented 6 years ago

Not sure i made myself clear so i pushed some code, let me know if you think we can work out something from this

salvadorc17 commented 6 years ago

BurnAreaScan() Dont seems to work for other system like Cps. Is not able to return the memory used, any way to get that one?

leiradel commented 6 years ago

@salvadorc17 this seems to be a different problem, I believe you should open a different issue.

andres-asm commented 6 years ago

AFAICT the callback was only added for NeoGEO for testing. Once a solution is in place it could be implemented in other drivers.

leiradel commented 6 years ago

Hey guys, achievements are working with FBAlpha, thank you very much!

Just one more request: can the RETRO_ACHIEVEMENTS macro be removed and achievements support be always on?

Also, we can have achievements for any FBA driver now, it's just a matter of implementing the callback :+1:

barbudreadmon commented 6 years ago

I enabled it. Also, i'll probably do some refactoring on the way the callback is called, dink gave me some good ideas to make things in a less hectic way.

salvadorc17 commented 6 years ago

Im interested in the method that can be used for calling retroachievements callback without touching drivers, im doing test in standalone fba.

leiradel commented 6 years ago

I believe the callback and macro should be changed to better reflect what this really is. Despite the motivation being to enable achievements, what this does is to report the System RAM to the libretro frontend, thats all. I.e. this could also be used for cheats.

barbudreadmon commented 6 years ago

Someone can confirm neogeo and cps1 RA are still working after my last commit ?

salvadorc17 commented 6 years ago

Seems working fine for those, but now cps2 games does crash, will need extra code before was working with same memory size

barbudreadmon commented 6 years ago

Probably because i didn't reimplement cps2, you told me they weren't working properly in your last reply

salvadorc17 commented 6 years ago

My mistake they were working, just seemed some memory areas was not properly aligned.

meleu commented 6 years ago

@barbudreadmon @leiradel

I've just tested Metal Slug X.

The cheevos feature works on commit 8ad726a96c322a2742aa3e61d46121ebb54290ef mslugx-cheevo-58546

It doesn't work on commit 5a19dd2a33a0986aad5ee2050bdced823115240e

Do you guys need the git bisect thing or you already know where the problem was inserted?

barbudreadmon commented 6 years ago

What about the other neogeo games ? @salvadorc17 said they were working.

meleu commented 6 years ago

@barbudreadmon I'm not sure what he meant by "working". The games are working, but the cheevos feature doesn't. And Metal Slug X is the only arcade game with an achievement set on our site, as you can see here.

meleu commented 6 years ago

Also, there's another thing that can lead to confusion: The RetroArch by itself is the piece of code responsible to load the achievements list. But the core needs to support cheevos correctly in order to make the achievements trigger.

When people load a game and open the Achievment List, they can believe that the cheevos feature is working fine, but they need to actually get a cheevo to confirm. That's what I did on my tests.

barbudreadmon commented 6 years ago

Do you mind telling me what changed in the data you are receiving ? Is the new data unusable or something ? Looks to me like what i'm sending after the refactoring from https://github.com/libretro/fbalpha/commit/3978cdeae2ab7cc8dd62c960102edd1da5ed6a64 should be the same as before, but perhaps i'm wrong.

meleu commented 6 years ago

are you on discord? if yes, look for meleu (me) at libretro server. I think we can talk better there

barbudreadmon commented 6 years ago

I'm not, and my oral english is lacking, so please explain here what you think the difference is compared to before https://github.com/libretro/fbalpha/commit/3978cdeae2ab7cc8dd62c960102edd1da5ed6a64 .

salvadorc17 commented 6 years ago

Dont want to be contradictive but meleu is right, seems memory have changed his alignement for neogeo and new changes are not making cps2 work properly, so to be clear the previous first changes was working as best.

barbudreadmon commented 6 years ago

Can we find a workaround for the new implementation ? If we stay with the new code, i don't mind having support for the 500+ drivers in the long term since all the code will be in the same place, if we go back to the old one where i have to hack every driver file, support will be limited to only a select few drivers. I leave the choice up to you.

andres-asm commented 6 years ago

I agree with @barbudreadmon

Also @leiradel said achievements aren't working on RA because the RA rom detection algorithm needs a new method for this.

Can you guys at least get the initial address and sizes logged before and after?

meleu commented 6 years ago

@barbudreadmon

If we stay with the new code, i don't mind having support for the 500+ drivers in the long term since all the code will be in the same place

Oh yeah! This is a pretty strong argument! And I agree. As this is a very new feature I think we can rework what we did at RetroAchievements and from now on use the new implementation.

@leiradel do you have something to comment about it?


@fr500

The hash used for arcade on RALibretro is just the ROM name with no extension, then mslugx is:

$ echo -n mslugx | md5sum
320a36c48c06dce51438108df9defb90  -

I think we can do it on RetroArch's cheevos.c.

EDIT: as a workaround I linked the hash calculated by RetroArch to Metal Slug X in our database.

leiradel commented 6 years ago

This is not something we can adapt to in Retro Achievements or RetroArch, this is something that just works or not. I didn't have the chance to test it yet.

meleu commented 6 years ago

I thought it was just a matter of memory addresses being shifted. I'll ask the cheevos developers community to hold on with NeoGeo/Arcade games until we can solve/test this.

andres-asm commented 6 years ago

@leiradel I understand that, getting a log of the reported memory addresses and memory sizes before and after the change would be helpful to debug this.

meleu commented 6 years ago

On my tests yesterday, I was testing RetroArch and trying to get cheevos.

Today I tested on RALibretro itself. And when using the fbalpha 43ad83084606e6735adc9dd5db5b157028f92861 RALibretro crashes right after opening the Memory Inspector.

How to reproduce:

  1. get RALibretro pack here.
  2. get the latest fbalpha 32bit here: https://buildbot.libretro.com/nightly/windows/x86/latest/
  3. unzip the fbalpha_libretro.dll.zip and put the file in the RALibretro's folder, inside the Cores folder.
  4. launch RALibretro and load fbalpha
  5. load a Neo Geo game
  6. after checking the game is running fine, go to RetroAchievements -> Memory Inspector
  7. CRASH
salvadorc17 commented 6 years ago

How do you commit each separated build?? Will be needed to get back to the old method, new one was moving the memory in 0x4000 filled with zeros at start for NeoGeo.

barbudreadmon commented 6 years ago

Will be needed to get back to the old method, new one was moving the memory in 0x4000 filled with zeros at start for NeoGeo.

Data from the first implementation is offsetted by 0x4000 or the 0x4000 first bytes are missing and replaced by 0 ? Or something else ? If it's the first, that means there could be some kind of junk inserted at the beginning of the buffer when using the new method, so we could probably get back the data from the old implementation by using an additional offset on the new one.

leiradel commented 6 years ago

@meleu can you attach the RALibretro logs here, one with the latest core and one with a core that was working before?

meleu commented 6 years ago

@leiradel

RALibretro's log when using a "good" fbalpha (bb651e3) good-log.txt

RALibretro's log when using the latest fbalpha (43ad830) and opening the Memory Inspector (CRASH!) crashlog.txt

RALibretro's log when using the latest fbalpha (43ad830) and not opening the Memory Inspector (no crash) no-meminspector-log.txt