Enchanced per-tile vertical scroll

[alexkiri]

This emulation hack allows allows each cell to be vscrolled individually, instead of being limited to 2-cell (16px). The offset of the new, intermediary cell is calculated as an average of the offset of the current 2-cell and the offset of the next 2-cell.

Before / after comparisons:

I still have some questions about the SDL build, the other platforms and the dev workflow. We can discuss here

[LibretroAdmin]

This is a great enhancement feature.

Out of courtesy @ekeeke we will let you review this. We hope you can appreciate the inherent interest users have in enhancement features like this - I think this could make for a very welcome addition to Genesis Plus GX across the board.

[ekeeke]

This is a nice and original hack idea indeed (although i am afraid it will only benefit to very specific scenes in a very limited set of games as the use of 2-cell vscroll + this kind of rotation effect in MD games is quite rare) and the code changes are localized and seem fine at first glance. I will try to make a proper review and share a few suggestions to improve integration with existing codebase in the upcoming days

[ekeeke]

OK, here are my suggestions after reviewing your code changes:

• the new rendering code is slightly less efficient as it recalculates pattern base address, line offset, etc for each tiles even when the hack is disabled so it would be better to leave the original "render_bg_m5_vs" function intact and create a new one that implements the enhanced rendering mode (named "render_bg_m5_vs_enhanced" for example). All you have to do is to modify the following line in vdp_ctrl.c:

/* Vertical Scrolling mode */
      if (d & 0x04)
      {
        render_bg = im2_flag ? render_bg_m5_im2_vs : render_bg_m5_vs;
      }

into

/* Vertical Scrolling mode */
      if (d & 0x04)
      {
        render_bg = im2_flag ? render_bg_m5_im2_vs : (config.enhanced_vscroll ? render_bg_m5_vs_enhanced : render_bg_m5_vs);
      }

Note that this means that you don't need to check if (config.enhanced_vscroll == 0) anymore in that new function

• the DRAW_COLUMN macro that you reimplemented (in order to draw the 2 tiles one by one) can lead to unaligned memory accesses (as the horizontal offset in line buffer is pixel based) which can cause crash on some platforms (mostly ARM based platforms I think), hence why there are two versions of the macro depending if ALIGN_LONG is defined or not. So, in your enhanced rendering function, you will need to similarly replace every occurrences of

    *dst++ = (src[0] | atex);
    *dst++ = (src[1] | atex);

by

#ifdef ALIGN_LONG
    WRITE_LONG(dst, src[0] | atex);
    dst++;
    WRITE_LONG(dst, src[1] | atex);
    dst++;
#else
    *dst++ = (src[0] | atex);
    *dst++ = (src[1] | atex);
#endif

• the use of MIN macro to calculate next_v_line is not necessary as it is only used to detect the case column = end - 1 but in this case, next_v_line is not used anyway as the offset is forced to previous calculated offset so it's simpler to always calculate next_v_line the same way

• the following code

        prev_v_offset = v_offset;
        v_offset = ((int)next_v_line - (int)v_line) / 2;
        v_offset = (abs(v_offset) >= config.enhanced_vscroll_limit) ? 0 : v_offset;
        if (column == end - 1) {
          v_offset = prev_v_offset;

could be optimized into

        if (column != end - 1) {
        {
          v_offset = ((int)next_v_line - (int)v_line) / 2;
          v_offset = (abs(v_offset) >= config.enhanced_vscroll_limit) ? 0 : v_offset;
        }

and prev_v_offset discarded as voffset will simply retain last calculated value for the last column

Note that this could be further optimized by moving the calculation of next_v_line with the calculation of voffset but you could argue that it's cleaner to have it calculated above with v_line

• there are 2 alternate versions of render_bg_m5_vs function in vdp_render.c, depending on ALT_RENDER being defined in Makefile or not so it is necessary to similarly have two sets of render_bg_m5_vs_enhanced function. The alternate version is slightly different in that it renders window layer / layer A first then directly renders+merges layer B into output buffer and therefore uses a different macro to render layer B column but it should be the same principle.

[alexkiri]

@ekeeke thanks for the detailed review. I have added the requested changes. but, as I mentioned earlier, I do have some other questions / uncertainties

1. in core/sound/sound.c there is a #ifdef that breaks the SDL build. with the line commented, it works, but I'm not sure if it breaks other things. what should I do here?
2. should I leave some comments for the render_bg_m5_vs_enhanced functions?
3. in the libretro/libretro_core_options.h, should I add even more description, such as including example games that are affected by the enhancement?
4. is it ok to merge into this libretro/Genesis-Plus-GX repo instead of the ekeeke/Genesis-Plus-GX repo?

[ekeeke]

Here are my answers:

1. this part is specific to this libretro fork repository, I think it was added with some other specific code portions in this file to handle retroarch run-ahead functionality. The fact it breaks compilation in non-libretro ports is probably an oversight from the developer who added run-ahead support, who likely only tested the libretro build. I agree this should be fixed to preserve code portability, even in a libretro fork repository.
2. yes, that's usually a good practice, it helps people not familiar with the codebase or with the context to understand what the code is doing, which is useful for maintanibility.
3. I think it's fine like this but maybe you could precise that this hack only applies to the few games that use 2-cell vertical scroll mode.
4. I would gladly accept this pull request in official repository so you could indeed submit a pull request for this in my repository instead (changes in my repository are usually synced into libretro repository after a while so it would benefit to both). In that case, it would be better to separate that pull request in two: one with SDL compilation fix for the libretro fork in libretro repository and the one with enhanced vscroll additions in my repository. If you want, you can add a copyright portion in vdp_render.c header to indicate that the enhanced vs mode rendering function is attributed to you as it's afaik an original creation.

[ekeeke]

If you don't mind, I have a few additional remarks in regard to portability, especially compatibility with some compilers that do not support C99 features (I think it is a requirement for libretro cores and ideally the core should remain ANSI C compatible for better portability):

• in vdp_render.c, you should correct (or just remove) the lines commented with //
• I see now you also modified the file shared.h to add an include to stdbool.h. I guess this fixes a compilation issue with SDL2 port ? In that case, this include should rather be moved to the file that needs it to compile as afaik, the core do not use bool types and neither libretro or sdl1 port need this (in my repository at least). More important, this file is only supported by compilers which have C99 support so it should not be included in core files to keep ANSI C compatibility. Ideally, this change should be put in a specific pull request that address compilation fixes (see comment above)

[ekeeke]

Looks fine to me but I remember @LibretroAdmin complaining that double-slash comments would not compile on some of the platforms they support in Retroarch so you might want to change those comments you added in render_bg_mode_5_vs_enhanced function to use /*... */ instead unless @LibretroAdmin is ok with this.

[LibretroAdmin]

Would indeed appreciate it if those comments were changed, yes.

[ekeeke]

I have no adverse comments following last commit and merged your pull request in my own repository, thanks for your work.

[ds22x]

Merged via ekeeke#453 and 9f407e8