libretro / mame2003-plus-libretro

Updated 2018 version of MAME (0.78) for libretro. with added game support plus many fixes and improvements
Other
189 stars 108 forks source link

Add Option to Eneble\Disable Cyclone and DRz80 #1087

Closed apison closed 3 years ago

apison commented 3 years ago

I think that will be very usefull an option that let to Enable\Disable 68000 cyclone and drz80 when the core is compiled with option USE_CYCLONE := 1 USE_DRZ80 := 1 like in fbneo core.

Many games doesn't work at least on my underpowered linux board if i compile with cyclone option,for example cps3 games , but other games runs more smoothly, cyclone give theme 10 fps that make the difference. So it will be great if cyclone and drz80 should be disable on incompatible games.

mahoneyt944 commented 3 years ago

@grant2258 @markwkidd @arcadez2003 Implementing this seems fairly straightforward as a selectable core option.... Though like many other options would require a mandatory core reset to toggle it. This maybe thinking a step ahead, but a more intuitive or even automated reset feature would be neat in this regard. Any thoughts about this?

ghost commented 3 years ago

These cpu cores are only for arm all you need to do is set a define. It already has a list you can select from AFAIK and enables games that need it on slower systems

apison commented 3 years ago

I've done the option myself and it work quite good its a simple task, as said by mahoney it require core reset. I've seen the define used to set the cyclone option, but the problem is that we must compile everytime we change something, moreover , with the core option its possible to select the use of cyclone, drz80 or both, some games works better with cyclone enabled and drz80 disabled.

The core option should be hided if the compile has not setted the option USE_CYCLONE := 1 USE_DRZ80 := 1

ghost commented 3 years ago

well you will need to undo all the other that was done here https://github.com/libretro/mame2003-plus-libretro/blob/master/src/mame2003/frontend_list.h

apison commented 3 years ago

No in reality the struct fe_drivers is used as default value, if nothing is setted, the change i've done are these :

1) Added to GameOptions in MAme.h enable_cyclone

MAME.h

struct GameOptions { int enable_cyclone; };

2) Added the core option in init_core_options

static void init_core_options(void)

/ ASM cores: 0=None,1=Cyclone,2=DrZ80,3=Cyclone+DrZ80,4=DrZ80(snd),5=Cyclone+DrZ80(snd) / init_default(&default_options[OPT_ENABLE_CYCLONE], APPNAME"_enable_cyclone", "Enable cyclone; None|Default|Cyclone|DrZ80|Cyclone+DrZ80|DrZ80(snd)|Cyclone+DrZ80(snd)");

3)Update the enable_cyclone variable

static void update_variables(bool first_time)

/ ASM cores: 0=None,1=Default,2=Cyclone,3=DrZ80,4=Cyclone+DrZ80,5=DrZ80(snd),6=Cyclone+DrZ80(snd) / // "Enable cyclone; None|Default|Cyclone|DrZ80|Cyclone+DrZ80|DrZ80(snd)|Cyclone+DrZ80(snd)"); log_cb(RETRO_LOG_ERROR, LOGPRE "Entrato in OPT_ENABLE_CYCLONE VarValue=%s \n",var.value); if(strcmp(var.value, "None") == 0) options.enable_cyclone = 0; else if(strcmp(var.value, "Default") == 0) options.enable_cyclone = 1; else if(strcmp(var.value, "Cyclone") == 0) options.enable_cyclone = 2; else if(strcmp(var.value, "DrZ80") == 0) options.enable_cyclone = 3; else if(strcmp(var.value, "Cyclone+DrZ80") == 0) options.enable_cyclone = 4; else if(strcmp(var.value, "DrZ80(snd)") == 0) options.enable_cyclone = 5; else if(strcmp(var.value, "Cyclone+DrZ80(snd)") == 0) options.enable_cyclone = 6;
else options.enable_cyclone = 0;

     log_cb(RETRO_LOG_ERROR, LOGPRE "Letta enable_cyclone: %d. \n", options.enable_cyclone);
     break; 

4) Apply the cyclone option selected in function retro_load_game, id default option is selected, if uses fe_drivers settings otherwise the selected one.

bool retro_load_game(const struct retro_game_info *game)

if (HAS_CYCLONE || HAS_DRZ80)

//I read only cyclone option
struct retro_variable var;
var.value = NULL;
var.key = default_options[OPT_ENABLE_CYCLONE].key;
environ_cb(RETRO_ENVIRONMENT_GET_VARIABLE, &var) ;
if(strcmp(var.value, "None") == 0)
    options.enable_cyclone = 0;
 else if(strcmp(var.value, "Default") == 0)
    options.enable_cyclone = 1;
 else if(strcmp(var.value, "Cyclone") == 0)
    options.enable_cyclone = 2;
 else if(strcmp(var.value, "DrZ80") == 0)
    options.enable_cyclone = 3;
 else if(strcmp(var.value, "Cyclone+DrZ80") == 0)
    options.enable_cyclone = 4;
 else if(strcmp(var.value, "DrZ80(snd)") == 0)
    options.enable_cyclone = 5;
 else if(strcmp(var.value, "Cyclone+DrZ80(snd)") == 0)
    options.enable_cyclone = 6;    
 else
    options.enable_cyclone = 0;

int i; int use_cyclone = 1; int use_drz80 = 1; int use_drz80_snd = 1; log_cb(RETRO_LOG_ERROR, LOGPRE "enable_cyclone: %d. \n", options.enable_cyclone); //Default cycle
if (options.enable_cyclone==1) {

for (i=0;i<NUMGAMES;i++)
{
    if (strcmp(drivers[driverIndex]->name,fe_drivers[i].name)==0)
    {
        /* ASM cores: 0=None,1=Cyclone,2=DrZ80,3=Cyclone+DrZ80,4=DrZ80(snd),5=Cyclone+DrZ80(snd) */
        switch (fe_drivers[i].cores)
        {
            case 0:
                use_cyclone = 0;
                use_drz80_snd = 0;
                use_drz80 = 0;
                break;
            case 1:
                use_drz80_snd = 0;
                use_drz80 = 0;
                break;
            case 2:
                use_cyclone = 0;
                break;
            case 4:
                use_cyclone = 0;
                use_drz80 = 0;
                break;
            case 5:
                use_drz80 = 0;
                break;
            default:
                break;
        }

        break;
    }
}

} //cyclone disabled else if (options.enable_cyclone==0) { //fe_drivers[i].cores=0; use_cyclone = 0; use_drz80_snd = 0; use_drz80 = 0; }
//Force cyclone type else { int cores=0; cores=options.enable_cyclone-1; log_cb(RETRO_LOG_ERROR, LOGPRE " Set Cyclone enable_cyclone: %d. \n", cores); switch (cores) { case 0: use_cyclone = 0; use_drz80_snd = 0; use_drz80 = 0; break; case 1: use_drz80_snd = 0; use_drz80 = 0; break; case 2: use_cyclone = 0; break; case 4: use_cyclone = 0; use_drz80 = 0; break; case 5: use_drz80 = 0; break; default: use_cyclone = 0; use_drz80_snd = 0; use_drz80 = 0; break; } }

ghost commented 3 years ago

Looks good just need to redo the setting if changed and reset seems like you have it all in place to be honest

mahoneyt944 commented 3 years ago

Go ahead and submit this as a Pull Request. We can test and make edits as needed to the PR.

apison commented 3 years ago

I've made the pull request, I've also added the conditional compilation to eliminate the option if the variabile USE_CYCLONE or USE_DRZ80 are not setted this must be tested.

mahoneyt944 commented 3 years ago

@apison I've merged a preliminary core option for changing the cyclone settings. Note that I've updated the naming of this of this option. Be sure to delete any old core options using the old naming "enable_cyclone" if you loaded this previously to avoid confusion later. Test the lastest commit and let my know if it works how you'd expect. You must reload the core after changing this option for it to take effect.

Cyclone mode: default - uses the default cyclone configurations disabled - does not use cyclone at all

The rest of the options will force the selected cyclone modes.

apison commented 3 years ago

Thank you mahoneyt944, I'll test the latest commit as soon as i can.

apison commented 3 years ago

Hy mahoneyt944,I've compiled the source code but it doesnt work in this way,In function retro_load_game it needs this code 👍

if(!init_game(driverIndex)) return false; init_core_options();

if (HAS_CYCLONE || HAS_DRZ80)

  struct retro_variable var;
var.value = NULL;
var.key = default_options[OPT_ENABLE_CYCLONE].key;
environ_cb(RETRO_ENVIRONMENT_GET_VARIABLE, &var) ;
if(strcmp(var.value, "None") == 0)
    options.enable_cyclone = 0;
 else if(strcmp(var.value, "Default") == 0)
    options.enable_cyclone = 1;
 else if(strcmp(var.value, "Cyclone") == 0)
    options.enable_cyclone = 2;
 else if(strcmp(var.value, "DrZ80") == 0)
    options.enable_cyclone = 3;
 else if(strcmp(var.value, "Cyclone+DrZ80") == 0)
    options.enable_cyclone = 4;
 else if(strcmp(var.value, "DrZ80(snd)") == 0)
    options.enable_cyclone = 5;
 else if(strcmp(var.value, "Cyclone+DrZ80(snd)") == 0)
    options.enable_cyclone = 6;    
 else
    options.enable_cyclone = 0;

we must read the enable_cyclone variable before it is used, otherwise cyclone will be always disabled, and we have to init_core_options() in order to make enable_cyclone variable readable. The order of the option in the menu is better now.

mahoneyt944 commented 3 years ago

So basically we want to init_core_options() before the cyclone code is run here. Might be a cleaner way to do this. I'll check it out.

apison commented 3 years ago

Yes and we have to load enable_cycle option.

mahoneyt944 commented 3 years ago

Later in retro_load_game we init the options and update them. I'm thinking we could just read the cyclone code directly after this and it would work. https://github.com/libretro/mame2003-plus-libretro/blob/776740864dbae6d207cab435a2abbf65b1e59b78/src/mame2003/mame2003.c#L1009-L1011

apison commented 3 years ago

I dont think so , because the 68000 core swith is done before init_core_options() and Update_variable(), for this reason I've ported init_core_options() on the top and read update_cyclone variable separetely .

mahoneyt944 commented 3 years ago

I mean to move all the cyclone code after. Like this https://github.com/libretro/mame2003-plus-libretro/compare/Cyclone

apison commented 3 years ago

Ok, I've have not understood this make sense i'll try it.

mahoneyt944 commented 3 years ago

Just committed it to the build c6768404c7fdf53e0851dad393d46dcea4c4d5ed Let me know if it works better.

apison commented 3 years ago

I've tried to compile the new code, it has an error , you have made the function "configure_cyclone_mode" that is a very good idea but it miss the variable driverIndex, we can pass it as parameter to the function.

apison commented 3 years ago

Passing driverIndex to configure_cyclone_mode make the code works great, i think that this could be the last change we need.

ghost commented 3 years ago

@mahoneyt944 the variables should really set in retro_init but the content detection probably is why mark moved it to retro_load. Variables really should be getting set before the game_init this did cause issues with samples at the time but it was worked around.

mahoneyt944 commented 3 years ago

Oops. I passed driverIndex to the new function. Just commited.

@grant2258 I haven't really looked at the grand scheme of the cyclone code, though it appears as though it was invasively added into retro_load_game so that the drz80 and cyclone could be swapped out before the game loads. I'm not sure these could be hot swapped with our current implementation without crashing the active running game anyhow.

As far as the variables, I'm not sure why they were added there, but I wanted to get the cyclone option working without altering the current implementation so I'm not introducing more "variables" lol. I'm certainly open to reviewing when and from where they are initialized if your interested in experimenting / testing with me.

apison commented 3 years ago

For me all works good, can i close this issue?

mahoneyt944 commented 3 years ago

Great. Thanks for testing.

mahoneyt944 commented 3 years ago

@apison if you're interested in improving our default cyclone configurations through testing with the new core option, you could create a new issue with game list corrections. For example:

Improved defaults

Or you could always create PR for the changes directly. https://github.com/libretro/mame2003-plus-libretro/blob/a582a5ae042deba4b8dc46ef6983a71c1e61b9b8/src/mame2003/frontend_list.h#L4-L14

apison commented 3 years ago

Yes if i found better config i will do, I'm on holiday for some weeks when i return Im planning to test the romset.