TrueColor: smoother diminished lighting #1220
Conversation
src/doom/m_crispy.c
Outdated
| #ifdef CRISPY_TRUECOLOR | ||
| // [crispy] zero out colormaps[] array so it can be | ||
| // reallocated and recalculated in R_InitColormaps() | ||
| if (colormaps != NULL) |
There was a problem hiding this comment.
Could you move this part straight into R_InitColormaps(), please?
src/doom/p_user.c
Outdated
| #define INVERSECOLORMAP 32 | ||
| #else | ||
| // [crispy] parameterized for smooth diminishing lighting | ||
| int INVERSECOLORMAP; |
There was a problem hiding this comment.
Let's just turn it into an int everywhere.
There was a problem hiding this comment.
Nope, wait. Let's turn NUMCOLORMAPS into an int and have #define INVERSECOLORMAP NUMCOLORMAPS everywhere.
src/doom/r_main.c
Outdated
| LIGHTSCALESHIFT = 12; | ||
| MAXLIGHTZ = 1024; | ||
| LIGHTZSHIFT = 17; | ||
| #ifndef CRISPY_TRUECOLOR |
There was a problem hiding this comment.
This one is tricky. 😁 How abot this logic?
// [crispy] smooth diminishing lighting
if (crispy->smoothlight)
{
#ifdef CRISPY_TRUECOLOR
if (crispy->truecolor)
{
// [crispy] if in TrueColor mode, use smoothest diminished lighting
LIGHTLEVELS = 256;
LIGHTSEGSHIFT = 0;
LIGHTBRIGHT = 15;
MAXLIGHTSCALE = 376;
LIGHTSCALESHIFT = 9;
MAXLIGHTZ = 1024;
LIGHTZSHIFT = 17;
}
else
#endif
{
// [crispy] else, use paletted paletted approach
LIGHTLEVELS = 32;
LIGHTSEGSHIFT = 3;
LIGHTBRIGHT = 2;
MAXLIGHTSCALE = 48;
LIGHTSCALESHIFT = 12;
MAXLIGHTZ = 1024;
LIGHTZSHIFT = 17;
}
}
else
{
LIGHTLEVELS = 16;
LIGHTSEGSHIFT = 4;
LIGHTBRIGHT = 1;
MAXLIGHTSCALE = 48;
LIGHTSCALESHIFT = 12;
MAXLIGHTZ = 128;
LIGHTZSHIFT = 20;
}There was a problem hiding this comment.
Ah, good old dance with mixed macros and conditions. Thank you, I wouldn't have been able to handle it!
src/doom/r_main.c
Outdated
| } | ||
| else | ||
| { | ||
| // [crispy] else, use paletted paletted approach |
There was a problem hiding this comment.
paletted paletted
I apologize, was literally working on a last breath, yesterday's marathon with light values exhausted me completely. But promised word should be kept. Will do all requested corrections today.
There was a problem hiding this comment.
No need to apologize! I am more than thankful that you guys keep on working on the ports and dig up new interesting projects each time. I don't even start writing any line of code when I'm tired, which is... about always nowadays. 😪
There was a problem hiding this comment.
Everything done as requested. It gets much more cleaner and simpler, thank you for suggestions!
Honestly, I was never willing to ask what the The Idea of implementing TrueColor support and not willing to ask now. I even didn't realized all it's potential and implementation's clarity until start to work with it by myself, but since then... By breaking limitation of 256 colors, it provides great power for graphical abilities, but these abilities have to used wisely and carefully. At least in terms of source port, that's aimed for compatibility, not for reutilization for own needs.
Small self-nitpick for graphical-only parts:
- Fake contrast is simply destroying diminishing logics in this case. It's a must-have de-facto for paletted render, especially for 1x resultion, but here... Just have a look at difference with and with out it.
- Visplanes lighting (a.k.a.
zlight) is not as smooth as walls lighting (a.k.a.scalelight), screenshot. Not sure what to do, visplanes already have widest amount of possible lightsMAXLIGHTZ = 1024. ChangingLIGHTZSHIFTjust leads to inaccuracies, but changing visplanes lighting formula is a no-go. Another thing is, despite of we are "3D" space, walls and floors are absolutely different entities.
There was a problem hiding this comment.
- Visplanes lighting (a.k.a.
zlight) is not as smooth as walls lighting (a.k.a.scalelight), screenshot. Not sure what to do, visplanes already have widest amount of possible lightsMAXLIGHTZ = 1024. ChangingLIGHTZSHIFTjust leads to inaccuracies, but changing visplanes lighting formula is a no-go. Another thing is, despite of we are "3D" space, walls and floors are absolutely different entities.
Yes, that's an issue, though a minor one. I guess one has to compare how both arrays are calculated. For zlight[][] it's level = startmap - (FixedDiv((ORIGWIDTH/2*FRACUNIT), (j+1)<<LIGHTZSHIFT)>>LIGHTSCALESHIFT)/DISTMAP; whereas for scalelight[][] it's level = startmap - j*NONWIDEWIDTH/(viewwidth_nonwide<<detailshift)/DISTMAP;.
There was a problem hiding this comment.
Now that we have 15 levels of LIGHTBRIGHT, you could aadapt fake contrast much finer against the wall's actual angle. But that wouldn't help with the case in your screenshots, though. I guess we need to keep it as it is now.
There was a problem hiding this comment.
Hmm.. Changing
1to0in bit-shifting seems to work, but probably it could be much better. Need to think, if needed at all, better just follow your recommendation. For comparison: before and after.
Well, no. Changing bit-shifting to 0 in the formula in p_setup.c simply reverts back to normal fake-contrast. That is, it only applies to strictly vertical and horizontal lines. The smooth color grading comes from the increasing distance to the player.
What this code does is to apply half of fake contrast to lines that are aligned in a certain angle interval around 45°, i.e. half-way between strictly vertical and horizontal. What we could do, though, is to spread the available 15 levels of LIGHTBRIGHT across more intervals between 0° and 90°, but that'd involve some more math. 😉
There was a problem hiding this comment.
but that'd involve some more math
You know me, I'm certainly bad with maths and formulas, so barely will be able to write something useful. 😟
Ideally, if it will be possible to correct the formula without too much changes, conditions and macroses.
Won't budge that easy. But if not changing formula, here's a small improvement:
MAXLIGHTZ = 1024*8;
LIGHTZSHIFT = 17-2;
Result is barely noticeable, but have a close look at the floor: before and after. Looking down reveals difference far better: before and after. I have a bad feeling that it is maximum that can be done without changing original formula, but at least it's something.
There was a problem hiding this comment.
Was walking home after job, and screenshots with lowered camera makes me think. I'll gladly explain, but it can't be explained just in a few words, so sorry for a long read.
Both of screenshots were made with disabled fake contrast, I need a cleaner view of how light levels are aligned, while fake contrast can be considered as a post-processing effect.
To be more precise, I was thinking about this misalignment. In ideal or simply real world, such misalignment shouldn't happen, light levels must be perfectly aligned and placed in harmony. But this is not our case, because the Mission is to make lighting smooth, not change it's model. You are probably remember Doom 95 and iconic view of Entryway with high resolution? It look different there, obliviously because of lighting code wasn't properly updated for higher resolutions. It was totally okay for that time, and even Doom Legacy have same issue, though I don't know, was it intentional or by design, but it's still there nowadays.
But we are seeking for accuracy. And most likely, if lighting will be shifted for perfect alignment, whole game world's lighting model will look... Different. So, I'm suggesting to consider vanilla lighting formulas as a sacred codes. Necessary changes were made to support higher resolutions, it is a must have, but even that changes keeping original look&feel.
So, to summarize, what should I do next:
- Fine-tune
MAXLIGHTZandLIGHTZSHIFTfor more smoothing. Multiplier8will probably consume even more memory, and probably it doesn't needed to be that big. - Fine-tune walls smoothing a bit, I'm still unhappy with that "1% inaccuracy". It should be a tiny correction in
MAXLIGHTSCALE, just need to find a right value.
There was a problem hiding this comment.
Nearly perfect smoothing, but it cost about 12 megabytes of memory (20 in total for smoother implementation). 😢 It feels like it is showing more proper colors in extreme distance, the tunnel I'm looking at is about 4300 map units long.
MAXLIGHTZ = 1024*10;
LIGHTZSHIFT = 17-3;
Less smooth and less voracious costs 4,7 megabytes and still:
MAXLIGHTZ = 1024*4;
LIGHTZSHIFT = 17-2;
For comparison: current approach, less smooth, more smooth. -timedemo results are same for all three approaches..
Which one you would like to have?
There was a problem hiding this comment.
Something is still odd. No matter which color you take in RGB color space, it will never take more than 256 steps to fade it to black, because then all the color channels will be zero. So, it makes no sense you'll have to allocate 10 MB of color tables to achieve a fade to black for a palette of RGB colors. I'll have a look at it later...
|
One more change, could you please enable |
|
Sure. Should it be in |
--- 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, |
src/doom/r_main.c
Outdated
| if (crispy->truecolor) | ||
| { | ||
| // [crispy] if in TrueColor mode, use smoothest diminished lighting | ||
| NUMCOLORMAPS = 256; |
There was a problem hiding this comment.
Could you formulate the values as multiples of the Vanilla values, e.g. LIGHTLEVELS = 16 * 16, and the shifts as e.g. LIGHTSEGSHIFT = 4 - 4?
There was a problem hiding this comment.
Sure, here it is:
// [crispy] if in TrueColor mode, use smoothest diminished lighting
NUMCOLORMAPS = 32 * 8; // 256
LIGHTLEVELS = 16 * 16; // 256
LIGHTSEGSHIFT = 4 - 4; // 0
LIGHTBRIGHT = 1 + 14; // 15
MAXLIGHTSCALE = 48 * 8; // 384, but initial is 376 / 48 = 7,833333333333333
LIGHTSCALESHIFT = 12 - 3; // 9
MAXLIGHTZ = 128 * 8; // 1024
LIGHTZSHIFT = 20 - 3; // 17
There was a problem hiding this comment.
Pretty sure LIGHTBRIGHT = 1 * 16.
There was a problem hiding this comment.
But if it's not a secret, what these multipliers and substractors can give? They are mostly different from each other.
There was a problem hiding this comment.
Take for example MAXLIGHTZ. You increase its value by factor 8, so this means we have 8 times as many light levels. To map distance to light levels, the distance value is right-shifted by the value of LIGHTZSHIFT. You'll have to decrease this value by 3, because 8 = 2^3 = 1 << 3, to map the distance value into a light level range that is 8 times as large. All values are connected with each other in one way or another. 😉
There was a problem hiding this comment.
So, I think the MAXLIGHTZ array is already large enough. It's the formula to fill it with actual light values that is too coarse. Maybe we'll have to make the value of DISTMAP parametric, but if this doesn't work out, and we don't want to manipulate the Vanilla formula, then I'm perfectly fine with flats rendering as it is now.
There was a problem hiding this comment.
Here I can only rely on your diction. My idea of keeping formulas is not because of "read-only", but only because of it's keeping vanilla lighting model. 12 megabytes is maybe okay, I'm just trying to don't turn program into a Google Chrome where simple blank page (about:blank, w/o anything!) consumes about 35 megabytes God knows for what kind of purposes.
I have experimented a bit with MAXLIGHTSCALE yesterday, but nope, it doesn't help. However, I decided to stop pixel surgery and just play casually through Doom 1, I have not just muscule but also a photographical memory for it, and played it a lot with TrueColor. 🙂 What can I say: that 1% inaccuracy is simply impossible to see while normal playing and not comparing screenshots. The game still looks as it should, though soft diminishing is more noticeable in darker areas. I think it's all absolutely fine, at least in visual aspect.
P.S. Speaking of 12 megabytes... Please do not ask how much will cost perfectly smooth diminishing of 28 custom + few extra colormaps. It's about 650 megabytes of memory.
P.P.S. Have to correct myself: by saying "what the The Idea of implementing TrueColor" I wasn't meaning to say "why it was done", no, of course not. It was meant to be "was it an technical interest, or a self-challenge, or some other motivation". But please do not answer.
There was a problem hiding this comment.
that 1% inaccuracy is simply impossible to see while normal playing and not comparing screenshots. The game still looks as it should, though soft diminishing is more noticeable in darker areas. I think it's all absolutely fine, at least in visual aspect.
That's what I say! 😉
|
Two remarks:
Ready to merge for me, or are there still any doubts left @JNechaevsky ? |
I agree. Technically, there might be some calculating speed-up, as looking into generated values by At the moment, we have achieved nearly ideal smoothing without changing lighting model itself, and this is most important.
Just try this in // [crispy] smoother fake contrast
- lightnum += curline->fakecontrast;
+ // lightnum += curline->fakecontrast;But please don't merge yet, we need some extra conclusions!
|
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). |
The beauty of the formula! Most importantly, MAXLIGHTZ now have 8192 values, not 10240 as was planned initially, yet still producing nearly perfect smoothing.
|
Let me please reply to this after the weekend. |
|
Of course! Meanwhile, I'll prepare everything for Heretic and Hexen. |
src/doom/r_data.c
Outdated
|
|
||
| // [crispy] zero out colormaps[] array so it can be | ||
| // reallocated and recalculated with various amount of colormaps | ||
| if (colormaps != NULL) |
There was a problem hiding this comment.
TODO, CRITICAL: this doesn't seems to work correctly by preventing gamma-correction to work while in smoothest lighting. Possible solution:
- Use
malloc/freeto freecolormaps[]array immediately. Doesn't seems to be safe by itself, as toggling smooth lighting on/off and changing gamma-correction may eventually lead to program crash, unless always allocate256 + 1instead ofNUMCOLORMAPS + 1inZ_Mallocbellow.
Patch, friendly with memory consumption with smooth lighting on / off:
// [crispy] zero out colormaps[] array so it can be
// reallocated and recalculated with various amount of colormaps
- if (colormaps != NULL)
+ if (colormaps)
{
free(colormaps);
- colormaps = NULL;
}
- if (!colormaps)
- {
- colormaps = (lighttable_t*) malloc((NUMCOLORMAPS + 1) * 256 * sizeof(lighttable_t));
- }
+ colormaps = (lighttable_t*) malloc((256 + 1) * 256 * sizeof(lighttable_t));There was a problem hiding this comment.
I don't quite get that code change. Why should we always allocate memory for 256 colormaps even if we explicitly have them set to 32? And let's please just use I_Realloc() here instead of that free()/malloc() dance. Thank you!
There was a problem hiding this comment.
I apologize. I have overheated a bit while code grinding, of course it should be just:
colormaps = I_Realloc (colormaps, (NUMCOLORMAPS + 1) * 256 * sizeof(lighttable_t));
There was a problem hiding this comment.
Nice to know in this context:
In the GNU C Library, if the new size is the same as the old, realloc and reallocarray are guaranteed to change nothing and return the same address that you gave. However, POSIX and ISO C allow the functions to relocate the object or fail in this situation.
https://www.gnu.org/software/libc/manual/html_node/Changing-Block-Size.html
So, at least one implementation does the right thing by default. 😁
There was a problem hiding this comment.
I was trying to call R_InitLightTables() and R_ExecuteSetViewSize() for arrays recalculation while gamma changing, and it seems to fixing issue. But this was obliviously silly, turning simple gamma code into something too overcomplicated. 😶 Oh well.
Have a bad feeling about Hexen, it seems to have nearly same issue I have bumped while initial TrueColor support - brightest colormaps gets broken, but I'll investigate, no need to panic yet.
There was a problem hiding this comment.
Have a bad feeling about Hexen, it seems to have nearly same issue I have bumped while initial TrueColor support - brightest colormaps gets broken, but I'll investigate, no need to panic yet.
Solved by using I_Realloc, so it was a right thing to do!
|
Have compiled the latest "smoother_light" Crispy branch today and have to say it looks excellent in the both games it works now. 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. 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. Awesome work! |
Hm, I wouldn't mind if we got rid of the intermediate
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.
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.
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.
We could |
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. 😕
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 ... and ...
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. |
|
Not that easy. 😉 This will simply reset fixed colormap until next tic, i.e. pause or menu have to be disabled. 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 |
|
You could keep track of a |
|
Hm. Can't imagine it yet, but probably, |
|
I can't find good use of 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;
+ }
+ }
|
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 |
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 Or I am getting something wrong again? |
|
Alright, I see. How many different values can That would mean back to |
|
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 Also, getting back to macrocizing invul colormap will require switching back to generating BW effect from |
|
I'm not talking about macroizing invul colormap. I just want to keep the original meaning of the if (player->fixedcolormap) // either 0 or 1 or 32
{
fixedcolormap = colormaps + (player->fixedcolormap * NUMCOLORMAPS / 32) * 256;
...
}
else
{
fixedcolormap = 0;
} |
|
Everything else can stay as is. |
|
Ah, got it now, thank you! Then we'll just have to clarify that |
|
The code snipped above can be found in |
I.e. handle all possible player-fixedcolormap changes via rendering only. Co-Authored-By: Fabian Greffrath <[email protected]>
|
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. |
|
Yep. right, especially because dividing by 32 is a simple bit-shift. |
src/heretic/r_data.c
Outdated
| r = gamma2table[crispy->gamma][playpal[3 * colormap[c * 256 + i] + 0]] & ~3; | ||
| g = gamma2table[crispy->gamma][playpal[3 * colormap[c * 256 + i] + 1]] & ~3; | ||
| b = gamma2table[crispy->gamma][playpal[3 * colormap[c * 256 + i] + 2]] & ~3; | ||
| r = gamma2table[crispy->gamma][playpal[3 * colormap[invul_index * 256 + i] + 0]] & ~3; |
There was a problem hiding this comment.
You could use INVERSECOLORMAP again here.
Co-Authored-By: Fabian Greffrath <[email protected]>
src/heretic/r_main.c
Outdated
| int LIGHTSCALESHIFT; | ||
| int MAXLIGHTZ; | ||
| int LIGHTZSHIFT; | ||
| int TORCHGLOWSHIFT; |
There was a problem hiding this comment.
Is TORCHGLOWSHIFT still needed?
src/doom/r_main.c
Outdated
| if (crispy->truecolor) | ||
| { | ||
| // [crispy] if in TrueColor mode, use smoothest diminished lighting | ||
| NUMCOLORMAPS = 32 << 3; |
There was a problem hiding this comment.
Do we even have to set NUMCOLORMAPS again in R_InitLightTables()? It is already set in R_InitColormaps().
There was a problem hiding this comment.
I apologize again. It's a fourth day of having a 🤧, and third time of having a 🤧 in two weeks. Was able to quickly recover first two times, but third one finally got me. The best thing I can do is to follow orders and instructions, rather than thinking clearly by myself.
Will commit requested changes shortly.
There was a problem hiding this comment.
Do we even have to set NUMCOLORMAPS again in R_InitLightTables()? It is already set in R_InitColormaps()
This is a bit tricky, as we handing NUMCOLORMAPS exclusively for TrueColor in R_InitColormaps(), while in R_InitLightTables() we handling all possible cases. Having NUMCOLORMAPS = 32 << 3; is redundant there, but it gives some "sync" with other conditions below. What is your recommendation then? Just delete it?
There was a problem hiding this comment.
but it gives some "sync" with other conditions below.
I'd rather see it as a potential source for bugs. 😉
Let's explicitly set NUMCOLORMAPS also in the #ifndef CRISPY_TRUECOLOR branch of R_InitColormaps() and not touch it again in R_InitLightTables().
|
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 🤧. |
Welcome to the world without color limitations! See: fabiangreffrath/crispy-doom#1220 Co-Authored-By: Fabian Greffrath <[email protected]>
|
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. Since it is the only one occurance of using #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)
#endifHexen and Strife seems to be fine. |
|
Yes, just go ahead and fix it, please. |
Just as promised. Having a
16 777 216colors at hand allows to have nearly perfectly smoothed diminished lighting, nearly close to hardware renderers. Few remarks and notes, as always:-timedemoruns.Testing map: epiclight.zip
Screenshots:
No smoothing:
Smoothing:
@fabiangreffrath, few explanations:
#ifndef CRISPY_TRUECOLORto 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.