Fix r_forceAmbient comparison #1510

VReaperV · 2025-01-21T11:37:17Z

No description provided.

DolceTriade · 2025-01-21T18:42:27Z

for the peanut gallery, why was the before comparison wrong?

VReaperV · 2025-01-21T18:44:20Z

r_forceAmbient ranges in [0.0; 0.3], while tmpAmbient is in range of [0; 255].

DolceTriade · 2025-01-21T19:28:11Z

I meant in the commit message or PR description.

VReaperV · 2025-01-21T23:58:32Z

Oh, sure, I added it to the commit description.

slipher · 2025-01-22T11:00:54Z

Setting it to 0 ought to mean no modification to the light grid. But if I set r_forceAmbient to 0 it makes stuff way brighter anyway.

Before r_forceAmbient changes (with any cvar value as it makes no difference):

With this PR and r_forceAmbient 0:

Also I think the light factor handling should be different. IMO it should target the final light value, not the pre-lightfactor-scaling value. With the max value of .3 something even seems to overflow somehow and it is less bright than with 0.

VReaperV · 2025-01-22T22:10:45Z

Setting it to 0 ought to mean no modification to the light grid. But if I set r_forceAmbient to 0 it makes stuff way brighter anyway.

Before r_forceAmbient changes (with any cvar value as it makes no difference):

The original code was supposed to set it to the value provided by the map in this case, it was just broken.

On hangar28 it works as you'd expect when the map doesn't set any value there:
0:

0.3:

Perhaps it should be changed such that this behaviour is used if the cvar is set to -1.0 instead?

Also I think the light factor handling should be different. IMO it should target the final light value, not the pre-lightfactor-scaling value. With the max value of .3 something even seems to overflow somehow and it is less bright than with 0.

That I agree with.

VReaperV · 2025-01-22T22:20:05Z

I've added both of these changes here.

slipher · 2025-01-23T06:56:55Z

IIUC the worldspawn _color or ambientColor keyword is not supposed to do anything unless the map light grid is broken or missing. Is that correct?

VReaperV · 2025-01-23T07:28:32Z

Not entirely.

This seems to be an XreaL feature, and from what I can tell:
If there's no lightGrid, either the worldspawn or r_forceAmbient value is used:
https://github.com/QuakeEngines/ET-XreaL/blob/7e5bcc9082d85ecf9cc3b29ccbddba21e80d7c06/src/engine/rendererGL3/tr_light.c#L375-L386
If the interpolated grid light average of the three colour channels is < r_forceAmbient (for ambient light), then r_forceAmbient value is used instead:
https://github.com/QuakeEngines/ET-XreaL/blob/7e5bcc9082d85ecf9cc3b29ccbddba21e80d7c06/src/engine/rendererGL3/tr_light.c#L266-L271

There might be a slight difference in doing this check on lightGrid points rather than the interpolated value, but I don't think it would make much of a difference.

slipher · 2025-01-23T08:44:56Z

If the interpolated grid light average of the three colour channels is < r_forceAmbient (for ambient light), then r_forceAmbient value is used instead:
https://github.com/QuakeEngines/ET-XreaL/blob/7e5bcc9082d85ecf9cc3b29ccbddba21e80d7c06/src/engine/rendererGL3/tr_light.c#L266-L271

Oh right, R_LightForPoint is also affected (not that this makes any sense, if you're not going to modify the light grid samples anywhere else). But for the vast majority of maps, which have a valid light grid and no realLight particles, nothing should be affected.

VReaperV · 2025-01-23T09:25:11Z

Oh right, R_LightForPoint is also affected (not that this makes any sense, if you're not going to modify the light grid samples anywhere else).

I think that's because it was originally used for entities as well, but the r_forceAmbient use was lost with the GLSL transition.

VReaperV · 2025-01-26T02:19:57Z

Anyway, if there are no further concerns with these changes, I'll merge them a bit later.

slipher

On some maps, with all default settings, this somehow makes the light grid darker rather than brighter. Before/after on nano

slipher · 2025-01-26T08:11:10Z

src/engine/renderer/tr_bsp.cpp

 		if ( tr.legacyOverBrightClamping )
 		{
 			R_ColorShiftLightingBytes( tmpAmbient );
 			R_ColorShiftLightingBytes( tmpDirected );
 		}

+		const byte forceAmbientNormalised = floatToUnorm8( r_forceAmbient.Get() );


The overbright is still not taken into account in non-clamped mode.

Isn't that what the multiplication by lightFactor does in the shader?

If we request minimum light of 0.4 then that should be applied as x = max(0.1, x) in the light grid since it will be multiplied later.

0.1 being 0.4 / tr.mapLightFactor? Or where does it come from?

Yes that's right

Well, that gives wrong results.
This pr without the scaling:

Dividing by tr.mapLightFactor:

Both are with default overbright values.

If the scaling is done after comparison, it's also wrong:

But beside that, I don't really see where the lightGrid values would be multiplied by anything extra in case of r_forceAmbient being < the value in the lightGrid. It's just substituting the values after they're normalised to [0; 1] range.

slipher · 2025-01-26T08:16:54Z

src/engine/renderer/tr_init.cpp

-	Cvar::Range<Cvar::Cvar<float>> r_forceAmbient( "r_forceAmbient", "Minimal light amount in lightGrid", Cvar::NONE,
-		0.125f, 0.0f, 0.3f );
+	Cvar::Range<Cvar::Cvar<float>> r_forceAmbient( "r_forceAmbient", "Minimal light amount in lightGrid; -1 to use map value", Cvar::NONE,
+		0.125f, -1.0f, 0.3f );


The _color keyword is used in a lot of extant maps, sometimes with pretty large values. I imagine it must have some meaning for q3map2 which is distinct from this XReal renderer's meaning. Maybe we should remove the _color worldspawn key (leaving just ambientColor) so that it doesn't mess up those maps.

Should probably be cheat, that's a big cheat to make models always displayed brightly. Maybe a bigger cheat even than r_gamma since you could make only the targets be bright, rather than everything

There's this in q3map:
https://github.com/id-Software/Quake-III-Arena/blob/dbe4ddb10315479fc00086f08e25d968b4b43c49/q3map/lightv.c#L4941
But I can't find any notion of that being used in the game's code.

Yeah that's what I'm saying. _color is intended for the map compiler and interpreting it like that in the renderer seems bad and wrong.

Removed the check for _color key.

What do you think about the cheat comment?

Good question... Maybe it should just be restricted to a lower range? The problem is that there are no real gameplay means of lighting up an area (firebomb with dynamic lights doesn't really count).

Interesting idea, maybe this could be an alternative to r_gamma that degrades the map's aesthetics less.

Yeah, I was thinking that too.

VReaperV · 2025-01-26T23:04:00Z

On some maps, with all default settings, this somehow makes the light grid darker rather than brighter. Before/after on nano

Fixed.

slipher

Still getting altered grid lighting for all the maps I test (nano, plat23 and sokolov). Sokolov has the biggest difference (much brighter). I'm still confused why this is happening... reading the code it looks like there should be no difference as long as ambientColor and r_forceAmbient are not set.

src/engine/renderer/tr_bsp.cpp

slipher · 2025-01-29T22:26:56Z

src/engine/renderer/tr_init.cpp

-	Cvar::Range<Cvar::Cvar<float>> r_forceAmbient( "r_forceAmbient", "Minimal light amount in lightGrid", Cvar::NONE,
-		0.125f, 0.0f, 0.3f );
+	Cvar::Range<Cvar::Cvar<float>> r_forceAmbient( "r_forceAmbient", "Minimal light amount in lightGrid; -1 to use map value", Cvar::NONE,
+		0.125f, -1.0f, 0.3f );


What do you think about the cheat comment?

VReaperV · 2025-01-29T23:09:06Z

Still getting altered grid lighting for all the maps I test (nano, plat23 and sokolov). Sokolov has the biggest difference (much brighter). I'm still confused why this is happening... reading the code it looks like there should be no difference as long as ambientColor and r_forceAmbient are not set.

Any specific viewpos? I checked on Sokolov at spectator spawn after rebasing and fixing && to ||, looked the same to me compared to 0.55.2.

slipher · 2025-01-29T23:14:36Z

Any specific viewpos? I checked on Sokolov at spectator spawn after rebasing and fixing && to ||, looked the same to me compared to 0.55.2.

How are you going to check at the spectator spawn? There need to be players or buildables to see the light grid changes. I'm seeing the differences on buildables as in the screenshots posted upthread.

VReaperV · 2025-01-29T23:22:41Z

Well, I just built the same buildings in that are on this branch and on 0.55.2.

VReaperV · 2025-01-29T23:25:35Z

(it also affects the first-person weapon)

slipher · 2025-01-29T23:43:33Z

If I check out this PR and go to the alien base on Sokolov, it is still a lot brighter. Closer to the second screenshot than the first in #1510 (comment)

VReaperV · 2025-01-29T23:48:50Z

I get this in alien base on Sokolov:

Do note though that r_forceAmbient should be set to -1 to disable it.

slipher · 2025-01-29T23:52:25Z

I see. I assumed -1 is supposed to be the default value. So is that an intentional change, to light everything up so it is easier to see for gameplay purposes or something?

VReaperV · 2025-01-29T23:53:09Z

Not really, I just didn't touch the default value.

slipher · 2025-01-29T23:55:24Z

If that is not intentional then let's set it to -1

VReaperV · 2025-01-29T23:57:35Z

Yeah, that makes sense, changed now.

r_forceAmbient ranges in [0.0; 0.3], while tmpAmbient is in range of [0; 255].

Allow using 0 as an actual value.

Avoid overflow and incorrect results due to the overbright shift. Also fix `r_forceAmbient` affecting lightGrid points inside of walls.

`_color` is used by q3map2 for some internal purposes, so don't use it here.

This cvar used to have no effect after lightGrid code was moved to GLSL, so disable it by default to keep the lighting the same.

VReaperV added T-Bug A-Renderer labels Jan 21, 2025

VReaperV mentioned this pull request Jan 21, 2025

NUKE leftover CPU lightGrid code, minor fixes and cleanup #1496

Merged

DolceTriade approved these changes Jan 21, 2025

View reviewed changes

VReaperV force-pushed the fix-force-ambient branch from 1eb37cd to 32c0a87 Compare January 21, 2025 23:58

VReaperV force-pushed the fix-force-ambient branch from 61cc149 to 1d3667a Compare January 26, 2025 01:14

slipher reviewed Jan 26, 2025

View reviewed changes

VReaperV force-pushed the fix-force-ambient branch 2 times, most recently from 420a9a8 to b8b0936 Compare January 26, 2025 23:01

VReaperV force-pushed the fix-force-ambient branch 2 times, most recently from fb9d9ca to 05e4367 Compare January 28, 2025 20:30

slipher reviewed Jan 29, 2025

View reviewed changes

VReaperV force-pushed the fix-force-ambient branch 2 times, most recently from 779b491 to 3fd76e5 Compare January 29, 2025 23:05

VReaperV force-pushed the fix-force-ambient branch from 3fd76e5 to 647e7fb Compare January 29, 2025 23:13

VReaperV added 5 commits January 30, 2025 06:06

Fix r_forceAmbient comparison

f25c65d

r_forceAmbient ranges in [0.0; 0.3], while tmpAmbient is in range of [0; 255].

Use -1 for map-value in r_forceAmbient

5523535

Allow using 0 as an actual value.

Set lightGrid point colour to r_forceAmbient after overbright shift

04300cd

Avoid overflow and incorrect results due to the overbright shift. Also fix `r_forceAmbient` affecting lightGrid points inside of walls.

Only check for ambient color with ambientColor key

a9212b3

`_color` is used by q3map2 for some internal purposes, so don't use it here.

Set r_forceAmbient to -1 by default

b12f641

This cvar used to have no effect after lightGrid code was moved to GLSL, so disable it by default to keep the lighting the same.

VReaperV force-pushed the fix-force-ambient branch from 1c774bb to b12f641 Compare January 30, 2025 03:07

Fix r_forceAmbient comparison #1510

Are you sure you want to change the base?

Fix r_forceAmbient comparison #1510

Conversation

VReaperV commented Jan 21, 2025

DolceTriade commented Jan 21, 2025

VReaperV commented Jan 21, 2025

DolceTriade commented Jan 21, 2025

VReaperV commented Jan 21, 2025

slipher commented Jan 22, 2025

VReaperV commented Jan 22, 2025

VReaperV commented Jan 22, 2025

slipher commented Jan 23, 2025 • edited Loading

VReaperV commented Jan 23, 2025

slipher commented Jan 23, 2025

VReaperV commented Jan 23, 2025

VReaperV commented Jan 26, 2025 • edited Loading

slipher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VReaperV Jan 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VReaperV commented Jan 26, 2025

slipher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VReaperV commented Jan 29, 2025

slipher commented Jan 29, 2025

VReaperV commented Jan 29, 2025

VReaperV commented Jan 29, 2025

slipher commented Jan 29, 2025

VReaperV commented Jan 29, 2025

slipher commented Jan 29, 2025

VReaperV commented Jan 29, 2025

slipher commented Jan 29, 2025

VReaperV commented Jan 29, 2025 • edited Loading

slipher commented Jan 23, 2025 •

edited

Loading

VReaperV commented Jan 26, 2025 •

edited

Loading

VReaperV Jan 26, 2025 •

edited

Loading

VReaperV commented Jan 29, 2025 •

edited

Loading