Heretic: add true color support

[JNechaevsky]

Pretty much same to Doom, except much more strict with translucency effects - we must keep original look & feel as much as possible, so there is no additive blending.

I wasn't able to resolve two small remaining problems:

1. Chain shading. No idea what to do, simple replacement of byte to pixel_t doesn't work. Something must be wrong with either dest = declaring, or inside while loop.
2. Automap background drawing/parallaxing. Since maplump is a 320x158 raw screen, we can't use memcpy here. In theory, we can draw it same way as common 64x64 flat tiles, but how to achieve that dang parallax effect then - no idea.

Any help is highly appreciated! ❤️

[fabiangreffrath]

1. Chain shading. No idea what to do, simple replacement of byte to pixel_t doesn't work. Something must be wrong with either dest = declaring, or inside while loop.

I've just had a look it this so far and it is indeed trickier than it initially seemed. The code picks pixels that are already drawn to the framebuffer and feeds them back into the colormaps array to get an increasingly darker shaded pixel back. This won't work with RGB pixels, of course, so we'll have to e.g. introduce another function that takes a pixel and a "darkening value" and returns a new pixel value.

[mikeday0]

2. Automap background drawing/parallaxing. Since maplump is a 320x158 raw screen, we can't use memcpy here. In theory, we can draw it same way as common 64x64 flat tiles, but how to achieve that dang parallax effect then - no idea.

It has been some time since I've looked at the automap background parallaxing but I'm wondering if it could be refactored to use V_DrawBlock() instead.

Question: Does the end-of-game demonscroll sequence work okay in true color?

[JNechaevsky]

This won't work with RGB pixels, of course, so we'll have to e.g. introduce another function that takes a pixel and a "darkening value" and returns a new pixel value.

Hmph. Maybe we can apply something like I_VideoBuffer[y] = I_BlendDark(I_VideoBuffer[y], shade[y]) for darkening? Such approach will work, but in this case it's tricky, since we need to account both hires and widescreen variables, fill not whole screen but just two small areas, and two vectors (left-to-right and right-to-left). On top of that, such darkening might be inaccurate, comparing to vanilla.

Non fair but extremely simple approach is to draw gradients by hand, hard-code them into executable (such gradients aren't supposed to be replaced by mods any way) and draw them as translucent patches. But this is artistical approach, sadly.

Like this:

Question: Does the end-of-game demonscroll sequence work okay in true color?

Absolutely fine, as well as all other RAW screens. Episode 2 underwater ending with custom palette working fine, too.

[JNechaevsky]

But even if w/o parallax effect, how to properly draw tiled 320x158 raw lump? Should be something like:

    for (int y = 0; y < SCREENHEIGHT - SBARHEIGHT; y++)
    {
        for (int x = 0; x < SCREENWIDTH; x++)
        {
            *dest++ = colormaps[src[((y & 157) * 320) + (x & 319)]];
        }
}

But it's not, it appears as broken. 🙁

[mikeday0]

Needs polish, but something like this should do the trick:

Diff

diff --git a/src/heretic/am_map.c b/src/heretic/am_map.c
index 32a3f7b5..53e6b0b0 100644
--- a/src/heretic/am_map.c
+++ b/src/heretic/am_map.c
@@ -1044,10 +1044,8 @@ void AM_Ticker(void)
 
 void AM_clearFB(int color)
 {
-    int i, j;
     int dmapx;
     int dmapy;
-    int x1, x2, x3;
 
     if (followplayer && !paused)
     {
@@ -1106,31 +1104,10 @@ void AM_clearFB(int color)
     // [crispy] To support widescreen, increase the number of possible
     // background tiles from 2 to 3. To support rendering at 2x resolution,
     // treat original 320 x 158 tile image as 640 x 79.
-    j = mapystart * (MAPBGROUNDWIDTH << crispy->hires);
 
-    x1 = mapxstart;
-    x2 = finit_width - x1;
-
-    if (x2 > MAPBGROUNDWIDTH << crispy->hires)
-        x2 = MAPBGROUNDWIDTH << crispy->hires;
-
-    x3 = finit_width - x2 - x1;
-
-    for (i = 0; i < finit_height; i++)
-    {
-        memcpy(I_VideoBuffer + i * finit_width,
-               maplump + j + (MAPBGROUNDWIDTH << crispy->hires) - x3, x3);
-
-        memcpy(I_VideoBuffer + i * finit_width + x3,
-               maplump + j + (MAPBGROUNDWIDTH << crispy->hires) - x2, x2);
-
-        memcpy(I_VideoBuffer + i * finit_width + x2 + x3,
-               maplump + j, x1);
-
-        j += MAPBGROUNDWIDTH << crispy->hires;
-        if (j >= MAPBGROUNDHEIGHT * MAPBGROUNDWIDTH)
-            j = 0;
-    }
+    V_DrawRawTiled(mapxstart, mapystart, MAPBGROUNDWIDTH << crispy->hires,
+                   MAPBGROUNDHEIGHT >> crispy->hires,
+                   SCREENHEIGHT - SBARHEIGHT, maplump);
 
 //       memcpy(I_VideoBuffer, maplump, finit_width*finit_height);
 //  memset(fb, color, f_w*f_h);
diff --git a/src/v_video.c b/src/v_video.c
index f602feb0..c7bfd65c 100644
--- a/src/v_video.c
+++ b/src/v_video.c
@@ -732,6 +732,27 @@ void V_LoadXlaTable(void)
     xlatab = W_CacheLumpName("XLATAB", PU_STATIC);
 }
 
+void V_DrawRawTiled(int x_off, int y_off, int width, int height, int v_max,
+                    byte *src)
+{
+    pixel_t *dest = dest_screen;
+    int x, y;
+
+    for (int i = 0; i < v_max; i++)
+    {
+        for (int j = 0; j < SCREENWIDTH; j++)
+        {
+            x = (j + x_off) % width;
+            y = (i + y_off) % height;
+#ifndef CRISPY_TRUECOLOR
+            *dest++ = src[width * y + x];
+#else
+            *dest++ = colormaps[src[width * y  + x]];
+#endif
+        }
+    }
+}
+
 //
 // V_DrawBlock
 // Draw a linear block of pixels into the view buffer.
diff --git a/src/v_video.h b/src/v_video.h
index ee13a51b..437673fa 100644
--- a/src/v_video.h
+++ b/src/v_video.h
@@ -67,6 +67,9 @@ void V_DrawXlaPatch(int x, int y, patch_t * patch);     // villsa [STRIFE]
 void V_DrawPatchDirect(int x, int y, patch_t *patch);
 void V_DrawPatchFullScreen(patch_t *patch, boolean flipped);
 
+void V_DrawRawTiled(int x_off, int y_off, int width, int height, int v_max,
+                    byte *src);
+
 // Draw a linear block of pixels into the view buffer.
 
 void V_DrawBlock(int x, int y, int width, int height, pixel_t *src);

I always hated that memcpy() in the original code.

[JNechaevsky]

These two commits are pure Black Magic, I even willing to ask about them! 🧙

Looks likes everything is done and working fine, and can be marked as review-ready for now. But before, couple of suggested self-nitpicks:

• Making extern declarations in the middle of the functions is not nice, even if it's between curly brackets {...}. Both R_SetUnderwaterPalette and R_InitColormaps should be declared as extern void in r_local.h. From another hand, R_SetUnderwaterPalette can be placed to f_finale.c w/o any externals, but it's a render-related thing, better keep it together with colormaps re-initialization, it seems to be the right place for such things.
• Probably same for gamma2table, worth to externalize to i_video.h. But it's global problem, not just for Heretic's true color and might be slightly out of scope of current PR.
• For 64x64 flat tiling, we can have a sonsolidated function like this one (inspired by PrBoom), so there will be no need to have couple of #ifndef CRISPY_TRUECOLOR macroses. Again, out of scope of current PR, but idea itself may be handly for future. Most important, such flats should be placed in PU_CACHE for sure.
• I have left antialias_* arrays untouched despite of only the first color is needed for REINDEX(№), i.e. all non-first (non-zero, actually) numbers are just a dead code now. Worth small discussion what to do with them, since I want to make everything carefull, without unnecessary changes and keep diff as small as possible.

[fabiangreffrath]

This function would be more universally usable (for truecolor Hexen, maybe) if you passed the palette lump name as an argument.

[JNechaevsky]

Fixed in 00c44aa. It's still local as discussed, but passing parametrized lump name was a very good idea:

• This makes code much neater and w/o extra macroses.
• E2PAL seems to be HeHacked`able name, so we have to care about it's possible name replaced, even though such replacement probably will never happen.

[fabiangreffrath]

Just get away with it, not worth a comment.

[JNechaevsky]

Fixed in 1f81552. But should I remove /* * sizeof(lighttable_t)*/ as well? A bit scarry for possible diff in future, like in Chocolate Doom sizeof exist, but here it doesn't. The reason might be not obivious for someone who haven't suffered this issue.

[fabiangreffrath]

These two commits are pure Black Magic, I even willing to ask about them! 🧙

Thank you! 😁

* Making `extern` declarations in the middle of the functions is not nice, even if it's between curly brackets `{...}`. Both `R_SetUnderwaterPalette` and `R_InitColormaps` should be declared as `extern void` in r_local.h. From another hand, `R_SetUnderwaterPalette` can be placed to f_finale.c w/o any externals, but it's a render-related thing, better keep it together with colormaps re-initialization, it seems to be the right place for such things.

Yes, let's please not add even more extern declarations scattered around the code. The palette switching function should probably even get moved to i_video.c.

* Probably same for `gamma2table`, worth to externalize to i_video.h. But it's global problem, not just for Heretic's true color and might be slightly out of scope of current PR.

Let's fix it while we are at it.

* For 64x64 flat tiling, we can have a sonsolidated function like [this one](https://github.com/JNechaevsky/international-doom/blob/main/src/v_video.c#L512) (inspired by PrBoom), so there will be no need to have couple of `#ifndef CRISPY_TRUECOLOR` macroses. Again, out of scope of current PR, but idea itself may be handly for future. Most important, such flats should be placed in `PU_CACHE` for sure.

Yes, please. These are really annoying code duplications.

* I have left `antialias_*` arrays untouched despite of only the first color is needed for `REINDEX(№)`, i.e. all non-first (non-zero, actually) numbers are just a dead code now. Worth small discussion what to do with them, since I want to make everything carefull, without unnecessary changes and keep diff as small as possible.

I think you even obsoleted these arrays entirely by using the color_shades[] approach. Maybe worth a comment.

[JNechaevsky]

I even willing to ask about them!

Oops, "not willing to ask" was meant! 😱

The palette switching function should probably even get moved to i_video.c.

I have double-checked, Heretic is the only game which is using such palette swapping, Hexen and Strife are operating with PLAYPAL only (and have own TC non-friendly nightmares, but for God's sake not here and not now). But if you still want it to be moved to i_video.c, sure, will do. Probably I_SwapPalette name fits well?

Yes, please. These are really annoying code duplications. (about 64x64)

Should I replace true color only code, or paletted code as well? And, in Heretic only, or in all four games?

The rest of comments are clear, thanks for making a fresh look!

[fabiangreffrath]

Oops, "not willing to ask" was meant! 😱

I guessed that. 😉

I have double-checked, Heretic is the only game which is using such palette swapping, Hexen and Strife are operating with PLAYPAL only (and have own TC non-friendly nightmares, but for God's sake not here and not now). But if you still want it to be moved to i_video.c, sure, will do. Probably I_SwapPalette name fits well?

Alright to leave it in place then. The function name is fine.

Should I replace true color only code, or paletted code as well? And, in Heretic only, or in all four games?

All games, all modes, please.

[fabiangreffrath]

All games, all modes, please.

Well, but that's really stuff for a separate PR.

[JNechaevsky]

Alright to leave it in place then. The function name is fine.

Thinking more, you're right, R_SetUnderwaterPalette should accept any given name since originally, E2PAL is called via DEH_String, which means that in theory this name can be HeHackEd'ized. As about R_SetUnderwaterPalette name itself, yeah, it is worth to leave "as is", since this palette swap effect is only doing it's short lived thing in F_DrawUnderwater.

Renaming E2PAL is a crazy case, and most likely will never happen, though.

[JNechaevsky]

I think you even obsoleted these arrays entirely by using the color_shades[] approach. Maybe worth a comment.

Just checked, fortunately not, still both arrays are used, except only first their numbers are needed. We still must have two arrays for normal and overlay modes, but now they can be as simple as:

// [crispy] gradient table for map normal mode
static pixel_t antialias_normal[NUMALIAS] = {
    96, 110, 75
};

// [crispy] gradient table for map overlay mode
static pixel_t antialias_overlay[NUMALIAS] = {
    100, 110, 75
};

Fun thing is - the only different color is the first one now (96 vs 100), it's common walls. Easiest way here is to use single array with middle color between 96 and 100 (it's 98), it seems to fit perfectly for both normal and overlay modes. This could be simplified a lot, but needs some extra thinking and good monitor, which is only at home, not at day job. 😒

[JNechaevsky]

I think it's all done and ready for full review. My last idea is reducing amount of incoming parateters for DrawWuLine, since NumLevels and IntensityBits are always contant and can be replaced with macroses on same manner as it is done in Doom. But I'm still a bit worried about possible redundant diff.

[fabiangreffrath]

Awesome job, thank you very much!

[JNechaevsky]

It was a pleasure! And it's started from pure interest, thank you for such gift as true color render!

On next step I'll try to simplify flat filling, I think function should accept different heights for filling both full screens (height is always y<SCREENHEIGHT) and screen borders (height is y<SCREENHEIGHT-SBARHEIGHT, depending on game). Side borders of status bar is slightly different, but should be also doable, need to think.

Meanwhile, which game should be adopted for true color next? We have Hexen and Strife left, both are evil, here's why:

Hexen:

• Uses not one, but two colormaps, second one is for foggy maps and composed pretty difficult (no diminished lighting, just rushy fading to gray?):
  
• We will need couple of extra panes, as game uses 28 palette indexes:
  • 14-21 poison effect
  • 22 freeze effect (player is turned into icicle)
  • 23-25 Cleric's BFG firings effect
  • 26-28 Mage's BFG firing effect
  • ... and of course, we need coloring formulas. I have small though about it, will explain privately - we'll probably can avoid too much headache.
• Fading in/out effects on end game screens. Not a big problem, we could use blending tricks probably.

Strife:

• While the rest of code is mostly Doom'y and we already have a template for writing XLATAB blending function in true color (should be same to Raven's TINTTAB), it's wiping, i.e. crossfading effect is heavily related to XLATAB which we can not use in true color. Possible solution is to create a copy of SDL texture and blend it with different opacity while effect is in progress, but such code should be in upper level, not just in f_wipe.c. A bit too complicated even while thinking about it.

[fabiangreffrath]

Needs polish, but something like this should do the trick:

[...]
I always hated that memcpy() in the original code.

@mikeday0 Oops, sorry! I just realized that I outright ignored your code suggestion.

[mikeday0]

Needs polish, but something like this should do the trick:
[...]
I always hated that memcpy() in the original code.

@mikeday0 Oops, sorry! I just realized that I outright ignored your code suggestion.

No need to apologize! Just happy to see this truecolor effort come to fruition.