Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Heretic: add true color support #1111

Merged
merged 25 commits into from
Nov 29, 2023
Merged

Heretic: add true color support #1111

merged 25 commits into from
Nov 29, 2023

Conversation

JNechaevsky
Copy link
Collaborator

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! ❤️

Despite of here in V_ functions TINTTAB is used for shadows drawing, it's initial purpose is slightly different - it is a blending table, which is *also* used for shadows.

"drawtinttab" must be more clear in this context, and in case one day TC implementation will be done for Strife, XLA's function should be named "drawxlatab".
Remove duplication of invul. colormap generation.
As not used in true color mode at all.
Fairly simple. We need only proper 256 colors for drawing E2END itself, but we also need to do a proper reset while switching back to normal palette, otherwise we'll get some wrong colors.

Suggestions for future:
- externalize gamma2table to i_video.h
- externalize R_InitColormaps and R_SetUnderwaterPalette to r_local.h
Also, "viewactive" is not needed. Despite of existing ovet the code, it's not doing anything. Using "setsizeneeded" wasn't optimal, it too expensive. All we need is:
- BorderNeedRefresh - updates screen border and bezel, including message area.
- SB_state - do a full redraw of whole status bar.
This is, hopefully, as small diff as possible, which could be useful for custom COLORMAPs (but are there any existing?). Note that unlike Doom, REINDEX is using lesser color range to be as close to original smoothing as possible.

At this point, only background is not working on automap.
@fabiangreffrath
Copy link
Owner

  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
Copy link
Collaborator

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
Copy link
Collaborator Author

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: image

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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.

// [crispy] Changes palette to E2PAL. Used exclusively in true color rendering
// for proper drawing of E2END pic in F_DrawUnderwater. Changing palette back to
// original PLAYPAL for restoring proper colors will be done in R_InitColormaps.
void R_SetUnderwaterPalette(void)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -879,7 +879,9 @@ void R_SetupFrame(player_t * player)
if (player->fixedcolormap)
{
fixedcolormap = colormaps + player->fixedcolormap
* 256 * sizeof(lighttable_t);
// [crispy] sizeof(lighttable_t) not needed in paletted render
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just get away with it, not worth a comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Owner

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
Copy link
Collaborator Author

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
Copy link
Owner

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
Copy link
Owner

All games, all modes, please.

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

Not sure if R_SetUnderwaterPalette needed to be guarded by a macro in header file, but probably this will make sence for better understanding about the nature of this function, i.e. it's "for true color only".
No changes for Hexen and Strife needed, as these externals were made for true color render only.
@JNechaevsky
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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. 😒

* Revert back pixel_t to byte for arrays of line colors. My typo made back in the time I was just started working on automap.

* Still use two arrays for normal and overlay mode. Idea of having "middle" color for walls drawing and array does not seems to be good enough, as we loose original color for both modes.

In theory, both arrays can be consolidated into one, but this is a bit tricky because we have to define "which color should be used for common walls drawing" w/o hitting if/else condition.
@JNechaevsky
Copy link
Collaborator Author

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.

@JNechaevsky JNechaevsky marked this pull request as ready for review November 28, 2023 17:09
Copy link
Owner

@fabiangreffrath fabiangreffrath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome job, thank you very much!

@JNechaevsky
Copy link
Collaborator Author

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?):
    image
  • 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.

@JNechaevsky JNechaevsky merged commit f9ddf41 into master Nov 29, 2023
6 checks passed
@JNechaevsky JNechaevsky deleted the truecolor_heretic branch November 29, 2023 08:48
@fabiangreffrath
Copy link
Owner

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants