libretro / snes9x2002

Snes9x 2002. Port of SNES9x 1.39 for libretro (was previously called PocketSNES). Heavily optimized for ARM.
37 stars 46 forks source link

Various Improvements #52

Closed jdgleaver closed 2 years ago

jdgleaver commented 2 years ago

This PR makes the following improvements to the core:

jSTE0 commented 2 years ago

@jdgleaver Would you consider making the max frameskip based on the frameskip interval:

diff --git a/libretro/libretro.c b/libretro/libretro.c
index 3d863eb..426b45a 100644
--- a/libretro/libretro.c
+++ b/libretro/libretro.c
@@ -674,7 +670,7 @@ void retro_run (void)
                   retro_audio_buff_underrun;

             if (!skip_frame ||
-                (frameskip_counter >= FRAMESKIP_MAX))
+                (frameskip_counter >= frameskip_interval))
             {
                skip_frame        = false;
                frameskip_counter = 0;
@@ -688,7 +684,7 @@ void retro_run (void)
                   (retro_audio_buff_occupancy < frameskip_threshold);

             if (!skip_frame ||
-                (frameskip_counter >= FRAMESKIP_MAX))
+                (frameskip_counter >= frameskip_interval))
             {
                skip_frame        = false;
                frameskip_counter = 0;

similar to how it's implemented in the PCSX ReARMed core and picoarch's patch to this core? At the max frameskip interval of 10, this is already 5.45fps whereas FRAMESKIP_MAX of 30 means just under 2fps and not really usable at that point. Also, the help text is incorrect, a frameskip interval of 2 results in 20fps, not 15. If you're happy with these proposals, I'll raise a PR. Thanks.

jdgleaver commented 2 years ago

@jSTE0

Would you consider making the max frameskip based on the frameskip interval:

No, I believe it is very wrong to use frameskip_interval in this fashion, and it bothers me that it is used like this in PCSX ReARMed. frameskip_interval has a specific meaning, and should not be mixed up with automatic frame skipping.

Now if on the other hand you wanted to add a new, separate option for setting the maximum number of skipped frames in 'auto' mode, I'd say that would be a good idea and I'd certainly approve a PR to that effect :)

Moreover, these options could then be selectively shown in the menu based on the frameskip type, using the RETRO_ENVIRONMENT_SET_CORE_OPTIONS_UPDATE_DISPLAY_CALLBACK and RETRO_ENVIRONMENT_SET_CORE_OPTIONS_DISPLAY environment variables (e.g. https://github.com/libretro/gambatte-libretro/blob/15536214cdce31894d374b2ffa2494543057082b/libgambatte/libretro/libretro.cpp#L1670-L1677, https://github.com/libretro/gambatte-libretro/blob/15536214cdce31894d374b2ffa2494543057082b/libgambatte/libretro/libretro.cpp#L1171). This would be very tidy.

Also, the help text is incorrect, a frameskip interval of 2 results in 20fps, not 15.

Good catch!

jSTE0 commented 2 years ago

@jdgleaver Thanks for the pointers, I'll try and knock something up in a few days time. Out of curiosity, how was the value of 30 chosen for FRAMESKIP_MAX? As hinted at earlier, that's a lot of frames to be skipping.

jdgleaver commented 2 years ago

The FRAMESKIP_MAX value is mostly an arbitrary choice. Ideally there would be no limit - it's just a last ditch fail-safe in the event that the hardware is simply too slow to run the core, and we just want to prevent the display from 'freezing' completely.

Remember that we absolutely never want to reach this limit (and so it should necessarily be high) - if we do, then the frame skipping has failed, and the core cannot run at full speed, and the audio will always crackle. It this happens, then the user experience will be poor no matter what we do (and I would argue that the core is not really worth running at all in this case...)