TrueColor: smoother diminished lighting

[JNechaevsky]

Just as promised. Having a 16 777 216 colors at hand allows to have nearly perfectly smoothed diminished lighting, nearly close to hardware renderers. Few remarks and notes, as always:

• Doesn't impact performance, at least on average results of -timedemo runs.
• Costs about ~4 megabytes of memory.
• Values has been empirically verified to represent original Doom lighting model.
• Inspired by this line of code from Jaguar Doom.
• Fake contrast is killing all impressions...

Testing map: epiclight.zip

Screenshots:

No smoothing:  Smoothing: 

@fabiangreffrath, few explanations:

• Ideally to avoid using extra macroses #ifndef CRISPY_TRUECOLOR to have one logic for everything, but in this case it may lead to some confusion while comparing to Chocolate Doom code base. It's like a "smaller diff vs. cleaner code" game. Not sure what will be better.
• When TrueColor compiled in, but disabled via config-file (it is possible to make it hot-swappable, but needs thinking how to fit it in Crispness menu), it is impossible to use more than original 32 colormaps. This is not limitation of implementation, this is how colormaps are working for emulated paletted render. So, this feature require TrueColor-in + TrueColor-on to be set.
• Any thoughts, recommendations and suggestions are highly appreciated!

[fabiangreffrath]

One more change, could you please enable crispy->smoothlight by default in the CRISPY_TRUCOLOR build?

[JNechaevsky]

Sure. Should it be

#ifndef CRISPY_TRUECOLOR
    int smoothlight;
#else
    // [crispy] enable smoother diminishing lighting by default in TrueColor
    int smoothlight = 1;
#endif

in crispy.h, or is there a possibly more elegant solution?

[fabiangreffrath]

--- a/src/crispy.c
+++ b/src/crispy.c
@@ -29,6 +29,7 @@ static crispy_t crispy_s = {
        .smoothscaling = 1,
        .soundfix = 1,
 #ifdef CRISPY_TRUECOLOR
+       .smoothlight = 1,
        .truecolor = 1,
 #endif
        .vsync = 1,

[fabiangreffrath]

Two remarks:

1) I think it's okay that flat rendering is a bit coarse. Look at how much more coarse it is than wall rendering on your screenshot showing the original rendering. I guess this comes from the original formula and we decided not to change it if we don't understand it fully.

2) Depth rendering is a bit brighter overall. Again, I think this is fine and merely a result of finer color grading, i.e. we don't and up in a darker slot so quickly and render an intermediate color instead. However, I think the math is right, the formulas look correct and haven't been modified here.

Ready to merge for me, or are there still any doubts left @JNechaevsky ?

[JNechaevsky]

1. ... I guess this comes from the original formula and we decided not to change it if we don't understand it fully.

I agree. Technically, there might be some calculating speed-up, as looking into generated values by printf(), which are later clamped to if < 0, then = 0 and if > 255, then = 255. There might be some kind of Ideal Formula to avoid using extra values and get exactly needed ones, but even if such formula will be written through a lot of headaches, there is still no guarantee that value precisions will be precise enough. In theory, we can tablify zlight values, but this won't work with scalelight as it's taking into account viewwidth_nonwide and detailshift.

At the moment, we have achieved nearly ideal smoothing without changing lighting model itself, and this is most important.

2 Depth rendering is a bit brighter overall.

Just try this in R_StoreWallRange() and you'll see who is causing such extra brightness. 😉

        // [crispy] smoother fake contrast
-       lightnum += curline->fakecontrast;
+       // lightnum += curline->fakecontrast;

But please don't merge yet, we need some extra conclusions!

1. ~ZLight smoothing, suggested in this post. Personally, I would like to recommend "more smooth" 12 MB approach, as it giving softest result. Maybe 12 MB is not that much, since TrueColor itself is all about uint32_t, not uint8_t's which already taking extra memory for nearly everything render-related.~ Ahem. 🥰

       MAXLIGHTZ =       128 << 6; // 8192, not initial 10240 and still perfectly smooth!
       LIGHTZSHIFT =      20 -  6;

2. Definitely worth to take care about other three games. Strife have "Smooth lighting" option (thanks @ceski-1!), Heretic and Hexen have not. The only problem is - there is no room in first Crispness page for both H&H, but probably moving "Audible" to second page will help. Ideally, if we could have a "TrueColor: ON/OFF" there as well for TrueColor build (yes, it's easily hot-swappable!), but that's a story for another PR.
3. ~I've made some benchmarks, but all of them were in stock Doom 2 demos, where scenes showing literally 2.5 visplanes and 3.14 segments. I need to benchmark extreme scenes, Sunder MAP15 can again be handy, even with simple -timedemo by starring into complex scene for a couple of second.~ Done. No performance impact even on extreme scenes!
4. For now, colormaps[] array is forcefully re-allocated (NULLified and allocated again, to be more correct) even when it's not really needed. Having NULLification in M_CrispyToggleSmoothLightingHook() wasn't nice, but at re-allocate only when it's really needed leads to having a boolean like R_InitColormaps (boolean recalc_numlights), which is not good too as it will lead to higher diff in the code and it's absolutely not needed in paletted render. Having a global boolean, guarded by #ifdef CRISPY_TRUECOLOR is even uglier. Maybe it's not a problem after all, since R_InitColormaps is not a every-tic/frame procedure, called not that often and properly used Z_Free/ZMalloc costs nothing.

[JNechaevsky]

3. I've made some benchmarks, but all of them were in stock Doom 2 demos

And done. Absolutely no difference on average -timedemo fps while starring 5 seconds to this crazy scene (screenshot) with new smoothest lighting (plus smoothest zlight) and original one. Planes value must be even higher on Crispy, I just want to show render counters. I can't believe it...

In cause you want to checkout zlight not with just pure white color, then I can only gladly invite you back to MAP18, this room (screenshot).

[fabiangreffrath]

Let me please reply to this after the weekend.

[JNechaevsky]

Of course! Meanwhile, I'll prepare everything for Heretic and Hexen.

[Meerschweinmann]

Have compiled the latest "smoother_light" Crispy branch today and have to say it looks excellent in the both games it works now. This is the next step and the cherry on top that software rendered truecolor rendering needs.

And performance is great too.

Even i would have expected it differently, the truecolor version with the new smooth diminished lightning has 5-10 frames MORE then the paletted 7.0 release version. Truecolor had never less frames then paletted renderer. Measured more then one time to ensure everything is correct.

Both measured in the a bit over 500fps range with boomer.wad -timedemo demo1 and the widest aspect ratio Crispy can do on a Ryzen 8700G. Match screen brings me a bit wider then 21:9 mode, but can't do 32:9.

Awesome work!

[fabiangreffrath]

Just try this in R_StoreWallRange() and you'll see who is causing such extra brightness. 😉

Hm, I wouldn't mind if we got rid of the intermediate fakeconstrast levels if they stand in the way. What do you think?

      MAXLIGHTZ =       128 << 6; // 8192, not initial 10240 and still perfectly smooth!
      LIGHTZSHIFT =      20 -  6;

Alright, although this is a bit less than proposed before, this is still an absurd amount of memory. Now we allocate 256 arrays of 8192 pixel values each to map distance to one of 256 colors. That's a whopping 8 MB just for flat lighting, the whole game once used to fit into this. 😉

I am aware of the fact that this won't change unless we review the way that distance maps to colors in the game in general, but I don't have any incentive to do so. And I also know that nowadays even 8 GB of RAM are considered low-end, but it's still something to keep in mind.

The only problem is - there is no room in first Crispness page for both H&H, but probably moving "Audible" to second page will help.

Sure, the menu layout isn't set in stone, for none of the games. If such a nice new feature requires some space in the menu, it needs to get rearranged a bit.

Done. No performance impact even on extreme scenes!

I don't find that too surprising. In the end, the processor has to look up a value in an array and it shouldn't make a difference how big that array actually is.

4. For now, colormaps[] array is forcefully re-allocated (NULLified and allocated again, to be more correct) even when it's not really needed.

We could realloc() the array, which takes care that it isn't resized if not necessary. On the other hand it brings some overhead, because it makes sure to preserve the array content, which we override in the very next step anyway.

[JNechaevsky]

Hm, I wouldn't mind if we got rid of the intermediate fakeconstrast levels if they stand in the way. What do you think?

Will do as you say. But probably worth to leave them, they are doing it's job, though smoothing itself could be better. How exactly to improve it? No idea. 😕

That's a whopping 8 MB just for flat lighting, the whole game once used to fit into this. 😉

But we already have this for a good cause. I agree here too, and hate when something require too much "just for nothing" or "just because". Modern web browsers and Windows Metro apps are notable examples. But in our case, when smoothed diminished lighting is disabled, it does not consumes extra memory, just have a look at task manager (or top on Linux?) after toggling on/off this feature.

... and ...

I don't find that too surprising. In the end, the processor has to look up a value in an array and it shouldn't make a difference how big that array actually is.

I believe, absolutely most important thing in this case is performance. Imagine, it won't require that 8 MB, but performs such smoothing dynamically every frame? Neither CPU not end user will be happy about such extra calculations, which may eventually lead to framerare dropoffs.

[JNechaevsky]

Now final stretch for most useful artifact in Raven games. I have a suspicion that using LIGHTBRIGHT here is inaccurate. It's working, but glowing amplitude becomes more intensive, comparing to vanilla approach.

[JNechaevsky]

Alright, most useful artifact seems to be fine, but this is not over yet. The problem is - fixedcolormap is also a part of player_t, so it have to be updated as well while toggling smooth lighting in TrueColor. Case to reproduce:

• Enable smooth lighting.
• Type IDBEHOLDV to get invulnerability.
• Press Pause.
• Toggle smooth lighting in Crispness menu. One of this may happen:
  • Worst case: program crash.
  • Non-worst: black screen until next game tic.

[fabiangreffrath]

You may want to iterate through players[i] and set the fixedcolormap element to 0.

[JNechaevsky]

Not that easy. 😉 This will simply reset fixed colormap until next tic, i.e. pause or menu have to be disabled. But look likes yeah, we have to update fixedcolormap for all players, not just displayplayer or consoleplayer.

Maybe something like this?

static void CrispySmoothLightingHook (void)
{
    crispy->smoothlight = !crispy->smoothlight;
#ifdef CRISPY_TRUECOLOR
    // [crispy] re-calculate amount of colormaps and light tables
    R_InitColormaps();

+   for (int i = 0; i < MAXPLAYERS; i++)
+   {
+       if (playeringame[i])
+       {
+           if (crispy->smoothlight)
+           players[i].fixedcolormap <<= 3;
+           else
+           players[i].fixedcolormap >>= 3;
+       }
+   }
#endif

[fabiangreffrath]

You could keep track of a static int prev_colormaps variable and only act when a change is detected.

[JNechaevsky]

Hm. Can't imagine it yet, but probably, prev_colormaps must be an array type, as we dealing potentially multiple players? As an alternative, I can suggest expanding condition to if (playeringame[i] && players[i].fixedcolormap) which means "perform shifting only if .fixedcolormap is not zero".

[JNechaevsky]

I can't find good use of static int prev_colormaps[], unfortunately. This seems to require two for passing of MAXPLAYERS, i.e. and first pass we have to track current .fixedcolormap for playeringame[] to write down current values, then toggle crispy->smoothlight, then make second for to compare .fixedcolormap, again for "playeringame" only, and make changes if necessary.

Extra brackets will be needed, otherwise we'll get "ISO C90 forbids mixed declarations and code" warning.

So at the moment, I'm pretty much stucked at this suggested approach, updated to "don't shift zeroes":

+   for (int i = 0; i < MAXPLAYERS; i++)
+   {
+       if (playeringame[i] && players[i].fixedcolormap)
+       {
+           if (crispy->smoothlight)
+           players[i].fixedcolormap <<= 3;
+           else
+           players[i].fixedcolormap >>= 3;
+       }
+   }

.fixedcolormap correction have to be done for all four games, since we have Torch artifact in Heretic and Hexen (glowing lighting effect) and Sigil weapon firing BW colormap.

[fabiangreffrath]

I can't find good use of static int prev_colormaps[], unfortunately.

I never meant this as an array. Just a static variable to keep track of the number of colormaps that was valid when the function was called last time, so we can check if we need to change the fixedcolormap properties of the players[i] or not.

[JNechaevsky]

I never meant this as an array.

But it have to be array, we have to deal with possibly multiple players. Non-array type can't keep more than one variable, right?

Imagine the case: paused demo playback of four players. Two of them have invulnerability, two have not. While "paused" state, we are toggling smooth lighting and pressing F12 to toggle views between players. Renderer already switched number of colormaps included fixed, but rendered frame will not be changed until next game tic, i.e. only after unpausing. This will lead to "worst/non worst" case of possibly crashing or showing wrong colormap.

Or I am getting something wrong again?

[fabiangreffrath]

Alright, I see. How many different values can player->fixedcolormap take? I think that's 0 for "none", 1 for the light power-up and 32 for the invulnerability power-up. Let's just keep these three possible values and multiply them with LIGHTBRIGHT when applied to the renderer.

That would mean back to #define INVERSECOLORMAP 32. Just an idea...

[JNechaevsky]

Will took some extra time, but the idea seems to be interesting. Just a quick thought: unlikely it will work "as is" for Torch effect, as it's player->fixedcolormap is not static and using glowing effect.

Also, getting back to macrocizing invul colormap will require switching back to generating BW effect from COLORMAP lump, like it's done in Heretic. Maybe it's not that bad, by losing very few possible colors, we can provide compatibility for custom invul colormap.

[fabiangreffrath]

I'm not talking about macroizing invul colormap. I just want to keep the original meaning of the player->fixedcolormap element. Something like this:

if (player->fixedcolormap) // either 0 or 1 or 32
{
    fixedcolormap = colormaps + (player->fixedcolormap * NUMCOLORMAPS / 32) * 256;
    ...
}
else
{
    fixedcolormap = 0;
}

[fabiangreffrath]

Everything else can stay as is.

[JNechaevsky]

Ah, got it now, thank you! Then we'll just have to clarify that player is a player_t structure. Should be doable without passing player parameter to M_CrispyToggleSmoothLightingHook. players[i].fixedcolormap is still easier and possible, though player->fixedcolormap looks neater.

[fabiangreffrath]

The code snipped above can be found in r_main.c:R_SetupFrame() and other functions. We don't need to iterate through the players[i] in any hook function. The players[i].fixedcolormaps values will make sense in any case, no matter if smoothlight or truecolor or none or both.

[JNechaevsky]

Done as requested. I think it's a great, safest correction, although now we have to perform one extra simplest math every frame. But comparing to dozens or even hundreds of other maths, this is literally costs nothing.

[fabiangreffrath]

Yep. right, especially because dividing by 32 is a simple bit-shift.

[JNechaevsky]

Thank you for your patience! I think we did something special by starting low with playing around with values and ending high with all the polish. My only remaining request for now is to postpone the celebration until recover from this darn 🤧.

[JNechaevsky]

Just spotted small unpleasant surprise from Heretic's status bar. When TrueColor's smooth lighting is on, gem/chain shading no longer works as intended (top: no smooth, bottom: smooth):

It's because of this line: https://github.com/fabiangreffrath/crispy-doom/blob/master/src/heretic/sb_bar.c#L453. Should NUMCOLORMAPS be replaced with 32, problem will be solved.

Since it is the only one occurance of using NUMCOLORMAPS outside from render functions, we probably don't need to have extra int variable and can go with simple 32?

#ifndef CRISPY_TRUECOLOR
    shades = colormaps + 9 * 256 + shade * 2 * 256;
#else
-    shade = 0xFF - (((9 + shade * 2) << 8) / NUMCOLORMAPS);
+    shade = 0xFF - (((9 + shade * 2) << 8) / 32); // [crispy] shade to darkest COLORMAP row (32)
#endif

Hexen and Strife seems to be fine.

[fabiangreffrath]

Yes, just go ahead and fix it, please.

[JNechaevsky]

Totally forgot about one interesting thing. Have a look at this and this comments, and this line in our implementation. I'm absolutely sure that original comment was meaning "...to have fewer calculations" but what a nice coincidence. 🙂