libretro / Genesis-Plus-GX

An enhanced port of Genesis Plus - accurate & portable Sega 8/16 bit emulator
Other
75 stars 71 forks source link

Games that doesn't work on RPI2 #34

Closed LodanZark closed 7 years ago

LodanZark commented 8 years ago

According to Sega Retro (http://segaretro.org/Genesis_Plus_GX): "ROM images up to 10 MB (Mortal Kombat II hack Mortal Kombat II Unlimited) (Ultimate Mortal Kombat 3 hacks such as Ultimate Mortal Kombat Trilogy, Mortal Kombat Revelations, Ultimate Mortal Kombat [SMD])"

Games that i've tested over Raspberry Pi Model 2 and haven't worked:

Mortal Kombat II Unlimited (http://www.romhacking.net/hacks/596/) - Fails to load

Mortal Kombat Revelations (http://www.romhacking.net/hacks/1826/) - Loads but it crash in middle of a match.

Ultimate Mortal Kombat Trilogy version 5125 (http://www.romhacking.net/hacks/1059/) - Fails to Load

Sonic 2 Long version 1.8.1 (https://info.sonicretro.org/Sonic_2_Long_Version) - It works if the name file of the ROM doesn't contain "()" or "[]", for example "Sonic 2 Long Version 1.8.1 (hack)", otherwise it fails to load...

ekeeke commented 8 years ago

All those hacks should work fine if you disable "68k Address Error" (and eventually enable "Force DTACK) in core settings.

For the record, those are badly "programmed" hack that fail to work on real hardware because they were only tested on inaccurate emulators.

As for filename issues, there is no restriction in the emulator about that so it's an issue with the frontend you are using.

LodanZark commented 8 years ago

Thanks, i will give a try ;)

EDIT: I've checked the retroarch-core-options.cfg file and the "genesis_plus_gx_force_dtack" is set to "enabled" and the problems persist

ekeeke commented 8 years ago

68k address error is what generally causes these hacks to crash and, as said above, it's the core setting that you should try to disable first. The other one was only mentionned as an additional possible solution.

I've tried those hacks on my standalone port for Wii and they all load fine so, at this point, any games that fail to load on your side is likely because of some limitation of RPi frontend unrelated to the emulator core itself or wrongly patched ROM.

LodanZark commented 8 years ago

I am using the Retropie distro version 3.2.1 (latest version at this date) for RPi2 with latest version of lr-Genesis-Plus-GX installed, I will try follow your instructions to see if i can solve this. Thanks very much for your reply

LodanZark commented 8 years ago

By editing the the retroarch-core-options.cfg file and changing the value of "genesis_plus_gx_addr_error" to "disabled" fixed all issues with: Sonic 2 Long version 1.8.1., Mortal Kombat II Unlimited and Mortal Kombat Revelations. Yet the Ultimate Mortal Kombat Trilogy version 5125 and the same version with extra tracks didn't start up.

ekeeke commented 8 years ago

There is indeed a bug accidentally introduced when adding support for cartridge games using audio CD tracks in https://github.com/ekeeke/Genesis-Plus-GX/commit/a07f7a3d24e77d7a6ebdcbae6ca51bc19c539edb.

This is now fixed in https://github.com/ekeeke/Genesis-Plus-GX/commit/8a5cce4e8d69a886177390a242f3a6079c478d04, thanks for the report. Since you are using it on RPi via retroarch though, this still need to be synced back in libretro repository and recompiled by libretro devs.

As for the 13MB version with extra tracks, it can not work because the max supported ROM size is 10 MB. I think it only works on hacked emulators and not on real hardware either because it uses memory range that is also used by Mega Drive internal hardware (Z80 & I/O area, VDP area) and is normally not usable for cartridge.

LodanZark commented 8 years ago

Thank you very much! Sonic 2 Long 1.81 and mortal kombat 2 unlimited works very well now with genesis_plus_gx_addr_error enabled, and the Mortal Kombat Revelations works fine yet gets often the blue screen with address error by accessing the 00FF233 address which is easily fixed by disable the genesis_plus_gx_addr_error. I suppose this issue can be closed.

ekeeke commented 8 years ago

You should be able to close it yourself, I don't have admin power on this repository myself and it does not seem anyone from libretro is visiting much here.

LodanZark commented 8 years ago

I will close it myself then ;)

inactive123 commented 8 years ago

@ekeeke I still visit here. Telll me the issues that should be closed on the fork and I'll do it.

Alternatively, if you don't like the fork repo having an issues section at all, then we could close it if that is what you want. Let me know about that.

Also, on another subject. To be honest, it would really help me and numerous users if you could help out with the Wii version of RetroArch sometime (I think I asked this once before and I didn't get much help then either). There are numerous issues we are still having with libogc since Day One and nobody ever bothered to help us. I still care about having RA run well on Wii but I am quite disappointed that no single Wii dev ever bothered to help us when we have asked time and time again to help fix up some of the rough spots.The issues are input bugs where sometimes the input can repeat itself for one frame, and also a better way to handle resolutions. We sometimes have issues with PAL resolutions.

We also have USB HID support in RetroArch Wii now where we want to attempt to do low-level USB HID for gamepads. Which works, but the issue is that we haven't figured out hotplugging for it. I could point you to a branch if you'd like to investigate this with us.

ekeeke commented 8 years ago

Telll me the issues that should be closed on the fork and I'll do it.

I think all the issue reports except #33 (which should probably be moved to my repository) can be closed.

Alternatively, if you don't like the fork repo having an issues section at all, then we could close it if that is what you want. Let me know about that.

Yes, it might be a better idea to have all core issues centralized in my repo.

Also, on another subject. To be honest, it would really help me and numerous users if you could help out with the Wii version of RetroArch sometime (I think I asked this once before and I didn't get much help then either).

I'm not sure what kind of help you are expecting, I think I already did some code review and gave some advices t but I'm afraid I don't have the time for deeper bug investigation on Wii or submitting code patches. To be honest, I do not even have much free time to work on my own version or for testing things on Wii recently and this is not going to improve this year due to some important family "update"coming in a few weeks ;-)

I still read and answer issues reports from time to time or make quick fixes when I can but I don't have much time for "serious" coding anymore.

I am quite disappointed that no single Wii dev ever bothered to help us when we have asked time and time again to help fix up some of the rough spots.

I understand that Retroarch is very important to you but you have to understand that most of the devs who were working on Wii homebrew left the "scene" long time ago. It's not that nobody wants to help you, it's more that people have their own life priorities and have moved to other things. You also have to realize that debugging other people's code can be sometime very difficult, especially when you are not familiar with the initial project.

The issues are input bugs where sometimes the input can repeat itself for one frame, and also a better way to handle resolutions. We sometimes have issues with PAL resolutions.

See, this is also a problem, because these are quite vague requests or hard to reproduce issues, which would require someone working full-time on it with precise directions about what you want or need, not something that someone external to the project could come up with a simple "hey, here is the solution to your problem" answer.

Like I said, I already had reviewed your code and did not see any problem that could cause the input issues and honestly never noticed any the few times I tried Retroarch on my Wii so it's going to be hard to propose a fix for that without knowing what causes it.

This was some time ago so if the code changed a lot since, I could possibly do another review to see if anything seem wrong to me in regard to resolution handling but, like I said, this is as much as I can do for now.

inactive123 commented 8 years ago

"Like I said, I already had reviewed your code and did not see any problem that could cause the input issues and honestly never noticed any the few times I tried Retroarch on my Wii so it's going to be hard to propose a fix for that without knowing what causes it."

The buttons get "stuck" for like half a frame at specific intervals. You can easily notice this with any CPS2 game in FB Alpha if you play it for some time with any fighting game. For instance, if you're controlling a character and pressing to the left and then downwards, it could get 'stuck' for like half a frame with 'down' being pressed during that time. Pretty much game-breaking.

"I understand that Retroarch is very important to you"

I hope you didn't mean this in a patronising way. Because it very much reads to me like that. It's like you say 'oh well you are ridiculous for caring so much about this' and I don't really respect or appreciate that kind of response. If you didn't mean it like that then I apologize for assuming it like that but that is how most of the rest of the post reads like 'not worth the time', 'those guys moved on', 'blablabla'.

If this is truly how it was meant though and this is the attitude that comes with it, then alright, I'll know better than to reach out to these 'console scenes' anymore in the future if people care about it this much. Makes no real difference to me.

ekeeke commented 8 years ago

It was not meant to ridicule anyone. As for myself, I can perfectly understand caring a lot for the project I 'm working on and being concerned by its quality issues. I was only trying to explain that what you are asking would require much more work than a few programming tips from fellow developers and that people that are unrelated to the project or "retired" from Wii homebrew likely do not have the motivation or time to debug and refactor Retroarch Wii driver for you.

I'm sorry you feel this is "blabla" , I was just trying to be honest regarding your request. As I said, I'm always open for doing code reviews or answering technical questions about Wii programming if I can.

inactive123 commented 8 years ago

OK thanks for clarifying that. I felt a bit bad about it if that was honestly the way you meant it. Good to hear it was not meant like that.

Anyway, me and another guy (@netux79) could definitely need your input here - you can read up in that thread about the issues we're dealing with -

https://github.com/libretro/RetroArch/pull/2616#issuecomment-171319999

We have a low-level USB HID driver written for Wii that supports the DualShock3, DualShock4, Wii U Pro pad, etc. We need it to support hotplugging so that we can hotplug pads and have them automatically configured in RA. That is the part where we are having no success at right now. ToadKing couldn't figure it out and right now we are stuck there.

This USB HID hotplugging is on a separate branch. The code is compartmentalized to one file (input/drivers_hid/wiiusb_hid.c).

ekeeke commented 8 years ago

I had a quick look to https://github.com/libretro/RetroArch/blob/wiiusb_hid_hotplugging/input/drivers_hid/wiiusb_hid.c

The code is quite complex and I don't have much experience with libogc USB driver so I could not comment on USB_ function calls but I have noticed at least two things that seem wrong to me in the code regarding hotplug:

1) "wiiusb_hid_in_device_ids" is used in "wiiusb_hid_change_thread" to search registered device ID in "current_device_ids" array which can have up to MAX_USER (16) connected devices.

s32 current_device_ids[MAX_USERS] = {0};

if (USB_GetDeviceList(dev_entries, MAX_USERS, USB_CLASS_HID, &count) < 0)
    continue;
// add all device ids to list
for (i = 0; i < count; i++)
    current_device_ids[i] = dev_entries[i].device_id;
while (current_dev)
{
        int offset = wiiusb_hid_in_device_ids(current_device_ids, current_dev->device.device_id);
        struct wiiusb_adapter *next_dev = current_dev->next;
        if (offset >= 0)
        {
            // exists, remove it from the list
            current_device_ids[offset] = 0;
        }
        else
        {
            // device is gone, remove it
            remove_adapter(hid, &current_dev->device);
        }
        current_dev = next_dev;
}

but this function only searchs for MAX_PADS (4) entries

static int wiiusb_hid_in_device_ids(s32 *device_ids, s32 search)
{
    int i;
    for (i = 0; i < MAX_PADS; i++)
        if (device_ids[i] == search)
            return i;
    return -1;
}

2) at the end of "wiiusb_hid_change_thread", you are adding new devices only if their device ID is equal to current_dev->device.device_id

if (wiiusb_hid_in_device_ids(current_device_ids, current_dev->device.device_id) >= 0)
    add_adapter(hid, &dev_entries[i]);

This makes no sense since :

I think the code should be:

if (current_device_ids[i]!= 0 && dev_entries[i].vid > 0 && dev_entries[i].pid > 0)
    add_adapter(hid, &dev_entries[i]);
netux79 commented 8 years ago

Thanks @ekeeke, I will check your comments out !

ekeeke commented 8 years ago

I took some time to have a look at https://github.com/libretro/RetroArch/blob/master/gfx/drivers/gx_gfx.c for potential issues:

1) in "gx_set_video_mode", I don't think it's a good idea to disable interrupts while reconfiguring all the video stuff, this should only be done normally for a short time period and it might be the cause of video corruption issue when switching mode. Therefore, I would remove calls to

 _CPU_ISR_Disable(level);

 _CPU_ISR_Restore(level);

in this function and instead add a call to VIDEO_WaitVSync() after the FIRST call to VIFlush() to ensure all the changes are done outside the display period.

2) When changing from interlaced to non-interlaced or the opposite, it is generally required to wait for at least two frame before timings are stabilized so you should all TWO calls to VIDEO_WaitVSync() after the LAST call to VIFlush() in "gx_set_video_mode"

3) In "gx_set_video_mode", the following code is unnecessary

if (modetype == VI_NON_INTERLACE && lines > max_height / 2)
    gx_mode.xfbHeight = max_height / 2;
else if (modetype != VI_NON_INTERLACE && lines > max_height)
    gx_mode.xfbHeight = max_height;

since the code just above:

if (lines <= max_height / 2)
{
    modetype = VI_NON_INTERLACE;
    viHeightMultiplier = 2;
}
else
{
    modetype = (progressive) ? VI_PROGRESSIVE : VI_INTERLACE;
}

if (lines > max_height)
    lines = max_height;

already take care of limiting the "lines" value so these two conditions will never happen.

4) in "setup_video_mode", the following condition test:

if (!gx->framebuf[0])
{
    unsigned i;
    for (i = 0; i < 2; i++)
        gx->framebuf[i] = MEM_K0_TO_K1(
        memalign(32, 640 * 576 * VI_DISPLAY_PIX_SZ));
}

is unnecessary since this function is only called once by "gx_init" on driver init which beforehand allocate a new "gx" structure so gx->framebuf[i] will always be unallocated (NULL) at this point and that condition always occurs.

5) in "setup_video_mode", the call to

VIDEO_GetPreferredMode(&gx_mode)

is unnecessary since this is also called by "gx_set_video_mode" when lines and width are set to zero which is done here just after.

6) in "init_texture", the GX limitation that texture width and height must be multiple of 8

width &= ~3;
height &= ~3;

is only when pixels use 16-bit format (RGB565 or RGB5A3). If RGB32 is used (RGBA8), the limitation is different I think. I am not sure if any core use RGB32 format while using uncommon video buffer size but you never know.

7) in "init_vtx" function, the following condition test:

 if (gx->scale != video->input_scale || gx->rgb32 != video->rgb32)
 {
      RARCH_LOG("[GX] reallocate texture\n");
      free(g_tex.data);
      g_tex.data = memalign(32,
      RARCH_SCALE_BASE * RARCH_SCALE_BASE * video->input_scale *
      video->input_scale * (video->rgb32 ? 4 : 2));
      g_tex.width = g_tex.height = RARCH_SCALE_BASE * video->input_scale;
      if (!g_tex.data)
      {
           RARCH_ERR("[GX] Error allocating video texture\n");
           exit(1);
      }
 }

is unnecessary since this function is only called once by "gx_init" on driver init which beforehand allocate a new "gx" structure so gx->scale and gx->rgb32 will always be uninitialized (0) at this point and that condition always occurs.

8) "gx" pointer is unallocated in "gx_free" function but gx->framebuf[i] that were allocated in "setup_video_mode" (cf. 4) are never unallocated, which could cause memory leak in case "gx_init"/"gx_free" is called multiple times by retroarch.

9) a lot of GX functions from libogc have been copied to be inlined/macroized in retroarch codebase (probably for performance reasons) and some VIDEO functions also were apparently renamed to VI_ equivalents (I could not figure the reason for these ) so you should make sure there hasn't been any error made while copypasting libogc code into retroarch and that those functions are strictly identical.

Out of curiosity, was there really a significant performance improvement measured when using macros instead of libogc functions?

netux79 commented 8 years ago

Thanks @ekeeke , I'm aware of the interrupt disabling issue and also about the need of waiting for the vertical retrace while changing resolutions and I also think this is in part the cause of the video corruption issue. The other code you point as unnecessary we'll need to check as the resolution is not changed once at startup in RA but each time you change cores, however these are pretty good catches as I didn't know about the texture width to be multiple of 8.

Regarding the code duplication, that was done since long time ago when the RA Wii version started (I guess) and personally I don't think that's necessary and it would be a better idea to stick with the original libogc functions.

I will work on the controls stuff first and then when I have time I will try these last comments from you for the gfx stuff.

Thanks a lot for the input!

ekeeke commented 8 years ago

The other code you point as unnecessary we'll need to check as the resolution is not changed once at startup in RA but each time you change cores

If you mean the part about "VIDEO_GetPreferredMode(&gx_mode)", it's just that this function is already called in function "gx_set_video_mode" that is called just after it so basically it's being called twice for nothing.

If you mean the part about gx parameter condition checks, it does not matter if or when resolution is changed, the point is that those bits of code are only called from "gx_init" after it allocates a new gx structure pointer so any gx->... parameter will always be reseted at this point. The fact gx->framebuffer[i] is never unallocated but the gx "parent" is (within gx_free) is also a problem that will likely cause memory issues after a few times reloading cores (as I assume this is what calls gx_free / gx_init).

To finish my "code review", here are some comments on https://github.com/libretro/RetroArch/blob/master/input/drivers_joypad/gx_joypad.c

Nothing much but here you go for completeness...

1) The following code in "gx_joypad_poll"

ls_y = (int16_t)PAD_StickY(port) * -256;
rs_y = (int16_t)PAD_SubStickY(port) * -256;

will cause an overflow if the value returned from one of the sticks is -128. The ls_y or rs_y value being an int16_t, they would become max negative value instead of desired (inverted) max positive value. This probably never occurs since the values returned from GX analog sticks are likely limited and never reach int8_t limit but you never know.

2) PAD polling is usually done just before VSYNC so you should add a call to "VIDEO_WaitVSync()" before the first call to "gx_joypad_poll()" in " gx_joypad_init" function

3) Try initializing the data format after having initialized WPAD in " gx_joypad_init" with

 WPAD_SetDataFormat(WPAD_CHAN_ALL,WPAD_FMT_BTNS_ACC_IR);

even if you are not using IR or motion because it will set the controller report mode to "CONTINUOUS" instead of the controller only reporting when a button is pressed. It should not matter in theory but you might want to try that to see if this fixes the "button sticking" bug as all homebrew emulator that I know use that mode.

4) in "gx_joypad_query_pad", pad is checked against MAX_USERS instead of MAX_PADS. On that matter, one problem I see with that driver is that it won't allow using a wii controller if a gamecube controller is connected to the same port number. It could be useful to increase MAX_PADS to 8 on Wii and use pads entries 4-7 for Wii controller ports.