Simplify overbright implementation #1406

slipher · 2024-10-30T20:56:27Z

AIUI overbright simply means that the lightmaps and light grid should be scaled from 0 to 4, instead of 0 to 1, so we can represent a larger range of brightness. But image colors are only allowed to go from 0 to 1. This is a big problem... if we are using GL 1 (hence, perhaps, the mysterious "hardware implementations"). But we have shaders, so all we have to do is multiply by 4 whenever we read one of those special light textures. Problem solved?

I tried this approach and it looks the same to me, consistent with the complex cancellng system that we had before. Also fixes the Perseus glass and Procyon star chart issues addressed in #1404.

An explanation for the canceling approach is given in #1050:

We cannot overBright separate stage as stage are clamped within [0.0, 1.0] when blending multiple stages together, meaning the result of the multiplication of a separate light stage with a light factor will be clamped before blending it with the color map.

But this is not an accurate representation of how shaders work. Each stage acts independently and directly on the color and depth buffers. There is no scratch area where shader stages are blended before going into the color buffer.

I would expect this to ameliorate the color banding problems with low-precision color buffers since now we don't give up 2 bits of precision to make room for canceling.

I haven't tried vertex lighting yet. Not sure how it's supposed to work with overbright.

DolceTriade · 2024-10-30T21:01:40Z

Screenies?

VReaperV · 2024-10-30T21:07:05Z

I haven't tried vertex lighting yet. Not sure how it's supposed to work with overbright.

Vertex lighting seems to use inverse light factor the same way.

slipher · 2024-10-31T01:14:16Z

From a recent master, default settings:

With this PR:

illwieckz · 2024-10-31T03:03:44Z

I'm all in favor of a simpler implementation.

In the past I first tried what you did there, but I got bugs. Maybe they were mistakes I did?

My main concern was about surfaces where the lightmap is applied more than once (and then multiplied more than once), basically terrain blended surfaces. Fortunately with your branch those terrain blended surfaces look fine (thunder outdoor and station15 garden):

On the station15 garden, plant leaves are wrongly lit, maybe they are vertex lit.

So, here are the light sources that should be multiplied:

lightmap light
vertex light
grid light
~~white fullbright light WHEN the lightmode is not fullbright~~

illwieckz · 2024-10-31T03:06:26Z

white fullbright light WHEN the lightmode is not fullbright

Doing this properly is one of the fix from #1404 (the starchart issue) and it should be done in all implementations.

src/engine/renderer/glsl_source/computeLight_fp.glsl

illwieckz · 2024-10-31T03:14:21Z

On my end when I first tried your approach, I was having such places reproducing the same clamping as we get whih the legacy clamped overbright:

But as you see in this screenshot, your branch doesn't reproduce the clamping. That clamping in GLSL was also what prompted me to follow a more complex route to avoid that clamping.

I don't know why I got any clamping in the past even when doing overbright in GLSL that way. The GLSL code received heavy rewrites, maybe we fixed some bugs in the meantime?

slipher · 2024-10-31T03:19:52Z

So, here are the light sources that should be multiplied:

lightmap light

vertex light

grid light

white fullbright light WHEN the lightmode is not fullbright

Lightmap and grid light are what is implemented in the initial draft of this PR. I don't think the last case exists for the simplified approach; it sounds like a problem of canceling.

Vertex light is a tiny bit more complex than the other cases since everything has the vertex color attributes, not only things with precomputed lighting. Maybe I can check tess.bspSurface and scale u_ColorModulate by 4 if is true.

illwieckz · 2024-10-31T03:22:05Z

Here is an example of GLSL overbrigth clamping when I first applied the overbright on light per stage instead of screen:

I don't get why I got it and you're not getting it, but if we're not getting it with the per-light approach, that's great!

illwieckz · 2024-10-31T03:24:52Z

I don't think the last case exists for the simplified approach; it sounds like a problem of canceling.

Probably not, the fullbright light mode is to just apply a white lightmap, so it's just using the lightmap code. Maybe there is nothing to do.

Though, for vertex lighting, we must make sure the (multiplied) vertex light doesn't multiply with the (multiplied) white lightmap fallback, to not multiply twice.

illwieckz · 2024-10-31T03:51:55Z

I reproduce the clamped overbright with that GLSL implementation, now I remember when it's happening.

The clamped overbright happens with such GLSL implementation when the lightmap is blended as a separate stage (i.e. no collapsing is doable), when the engine processes a lightmap stage over a colormap stage (or the colormap stage over a lightmap stage), instead of a singe diffusemap stage:

This:

stage colormap
stage lightmap

Or this:

stage lightmap
stage colormap

Instead of:

stage diffusemap

Here is an example with a build of the pulse map attached (the one hosted on sweet folder has broken lightstyles so it doesn't reproduce the light clamping bug):

The master branch:

This branch:

File: map-pulse_0-20230207-114641-e9a0a5e.dpk.zip

viewpos: 6502 -3015 -1642 -92 12

I guess I don't reproduce the bug with plat23 anymore because I improved the collapse code and we now collapse more materials, getting them processed as a single diffusemap stage instead of separate colormap + lightmap stages.

illwieckz · 2024-10-31T04:03:44Z

To make it more obvious, the problem is where the red arrows are pointing at:

illwieckz · 2024-10-31T04:16:39Z

So I actually reproduce the clamping bug with a released Tremulous map that is the original map build by the original author (only repackaged, not rebuilt):

Map: https://users.unvanquished.net/~sweet/pkg/map-metro-b1-2_0%2B0.dpk

viewpos: 2123 -231 -166 -89 26

With master:

With this branch:

illwieckz · 2024-10-31T04:30:18Z

Interestingly, your branch fixes the (what is assumed to be a) floating light shader that should not be visible:

Map: https://users.unvanquished.net/~sweet/pkg/map-metro-b1-2_0%2B0.dpk

viewpos: 2267 -429 -223 -51 -19

branch master or illwieckz/obb:

branch slipher/overbright-simple:

The metro map is the only map known to reproduce this floating transparent surface bug, so it's low priority, but anything helping to pinpoint what is happening is welcome.

illwieckz · 2024-10-31T04:35:15Z

So basically, I'm all in favor of investing this more and if we can make it handle light styles properly, let's do it.

But for the 0.55.1 release we better merge my fixes, we'll have all the time to investigate a different implementation after that.

slipher · 2024-10-31T04:47:32Z

I looked at the Metro light style shaders. So apparently it builds a composite lightmap in the color buffer from 4 additive stages, then does a standard multiplicative blend with the diffusemap in the final stage.

I guess we could keep the cancellation approach for light styles only. There is already detection and special handling for light styles anyway.

illwieckz · 2024-10-31T04:49:36Z

I guess we could keep the cancellation approach for light styles only. There is already detection and special handling for light styles anyway.

If that's doable that would be awesome! I dislike the cancelling-everywhere approach a lot (it's very fragile because of the very large amounts of uses case…).

VReaperV · 2024-10-31T08:38:44Z

So, here are the light sources that should be multiplied:

lightmap light

vertex light

grid light

white fullbright light WHEN the lightmode is not fullbright

Lightmap and grid light are what is implemented in the initial draft of this PR. I don't think the last case exists for the simplified approach; it sounds like a problem of canceling.

Vertex light is a tiny bit more complex than the other cases since everything has the vertex color attributes, not only things with precomputed lighting. Maybe I can check tess.bspSurface and scale u_ColorModulate by 4 if is true.

Vertex lighting is multiplied with the diffuse colours, so multiplying it by 4 again would be wrong.

Although on master vertex lighting is also somewhat broken with overbright: e. g. on map railgun from Wolf:ET the icicles are almost completely black without overbright clamping, owing to the fact that they're entirely vertex-lit. Perhaps there needs to be a check for surfaces that use only vertex lighting, which would then be multiplied or whatever. That shouldn't be done in tess however, just multiply the vertex colours themselves instead.

illwieckz · 2024-10-31T08:41:43Z

Although on master vertex lighting is also somewhat broken with overbright: e. g. on map railgun from Wolf:ET the icicles are almost completely black without overbright clamping

Probably the same bug as the procyon hologram?

VReaperV · 2024-10-31T08:56:59Z

Although on master vertex lighting is also somewhat broken with overbright: e. g. on map railgun from Wolf:ET the icicles are almost completely black without overbright clamping

Probably the same bug as the procyon hologram?

I doubt it, the procyon one has a lightmap.

slipher · 2024-11-02T04:52:54Z

Besides canceling, another possible approach to light styles would be using multiple collapse lightmap stages with additive blending, and disable dynamic lights in all but one.

illwieckz · 2024-11-02T07:35:42Z

disable dynamic lights in all but one.

I guess it is already the case, at least with the tiled renderer: light styles light map are expected to be rendered with Renger_generic3D and not with Render_lightMapping as far as I know.

illwieckz · 2024-11-02T09:12:22Z

multiple collapse lightmap

Actually I don't see how that would works? to collapse lightmaps with what?

slipher · 2024-11-02T16:59:55Z

I mean the kind of stage that does diffusemap * lightmap. Like the ones notated { diffusemap textures/something }. Several stages like that with additive blending, but with different lightmaps each time, and dynamic lights disabled on all but one.

illwieckz · 2024-11-02T18:33:27Z

This may be theoretical, but we may not exclude that some non-lightstyle lightmaps rendered with the lightMapping stage get blended like the light style ones and reproduce the bugs that happens with light styles.

It's just that the collapsing has been well improved so standalone non-light-style lightmap stages are more rare nowadays, but it is not guaranteed we collapse all of them or that we even can collapse all of them.

illwieckz · 2024-11-02T18:37:40Z

@slipher do you have any understanding about how the light style lightmap blending is working and then how we may workaround the related bugs?

Actually we may workaround some wrongness by normalizing to 1 at the end of the GLSL shader, to avoid bad colors popping up there and there. But that will still clamp them (it would still be better than the legacy lightmap shift that is not actually clamping but turns 75%s of the data into garbage).

illwieckz · 2024-11-02T19:11:03Z

I added a commit to multiply the light factor instead of dividing the inverse light factor, that also cleaned-up code, so it helps to have a better look at things.

VReaperV · 2024-11-04T07:20:11Z

Interestingly, your branch fixes the (what is assumed to be a) floating light shader that should not be visible:

Map: https://users.unvanquished.net/~sweet/pkg/map-metro-b1-2_0%2B0.dpk

viewpos; 2267 -429 -223 -51 -19

branch master or illwieckz/obb:

branch slipher/overbright-simple:

The metro map is the only map known to reproduce this floating transparent surface bug, so it's low priority, but anything helping to pinpoint what is happening is welcome.

I'm guessing it's the weird square on the wall that is the issue, right?

illwieckz · 2024-11-08T22:47:24Z

Can you confirm if the issue is still there on your end?

I'll test this after sleeping. 🙂️

slipher · 2024-11-08T23:15:36Z

I'd like to avoid adding u_LightFactor to the generic shader at all. I think we can do it via u_Color instead. We can add a flag to make Tess_ComputeColor scale by the light factor. I'll try this later.

illwieckz · 2024-11-09T13:24:43Z

I still have one bug with the material system, off:

on:

You'll see the plants are too bright.

(Don't mind if the floor and the trees look a bit different than stock map, I'm also experimenting other things with the map, but those plants are not modified).

VReaperV · 2024-11-09T14:02:43Z

It's because the vertex colour attribute is disabled. I'll look at why that happens.

VReaperV · 2024-11-09T14:23:45Z

I still have one bug with the material system, off:

on:

You'll see the plants are too bright.

(Don't mind if the floor and the trees look a bit different than stock map, I'm also experimenting other things with the map, but those plants are not modified).

Fixed now.

VReaperV

LGTM

VReaperV · 2024-11-09T14:30:15Z

Since I only did a small change here, and the pool of people who can review this isn't much larger than this pr's contributors, I took the liberty of reviewing it, especially as I reviewed most of it earlier anyway.

illwieckz · 2024-11-09T16:14:25Z

I squashed the “fixup” commits.

This looks ready to me.

There are some improvements that may be doable:

setting per-stage lightFactor at shader load time (this requires to rewrite some BSP loading code),
pre-multiplying lightFactor with u_Color to not have to pass it to GLSL (this may benefit from the former),
turn colormap stage with vertex color blending to a diffusemap one when the shader has no “nolightmap” keyword (so it can also receive dynamic light).

But that can be done in the future in subsequent PRs.

illwieckz · 2024-11-09T18:17:42Z

I moved the fix for the metro lanterns here:

tr_shader: skip standalone lightmaps, they are assumed to be buggy #1426

illwieckz · 2024-11-09T18:18:13Z

I also renamed the PR to fit what it does, as it is now in good shape for merging and we will going to do things that way.

slipher · 2024-11-09T22:48:01Z

Please remember that according to our PR etiquette, no one should merge another person's PR :)

slipher · 2024-11-12T07:50:27Z

I've now reimplemented the three special cases treated by illwieckz in way that avoids adding u_LightFactor to the generic shader. Those fixes are separated into a temporary fixup commit for convenient reading.

Let us call BASE the source tree obtained from merging #1426, #1431 and #1434 into master. Let us call SIMPLE this PR's tree, the slipher/overbright/simple branch (which incorporates the three aforementioned PRs). To verify the changes I have, using the scenes listed here, done a 6-way comparison of BASE with all default settings, BASE with material system, BASE without 16-bit framebuffers, SIMPLE with all default settings, SIMPLE with material system, and SIMPLE without 16-bit framebuffers. (Please suggest more scenes!)

Some bugs fixed by these overbright changes

Non-lightmapped vertex-lit surface not overbright scaled (station15 `462 -1428 -114 178 -8`)

BASE

SIMPLE

Improperly scaled 'glow map' (zittrig-arena `1201 -301 -338 87 0`)

BASE ( + material system)

SIMPLE ( + material system)

Procyon star chart darkened with material system

BASE + material system

SIMPLE + material system

Consequences for framebuffers with only 1 byte per color

In some cases things are worse because of clamping in the color buffer that wasn't there before. In some cases things are a bit better because of the effective increased precision. It may be preferable to enable overbright clamping on low-end systems that don't have 16-bit float framebuffers.

Color clamping regression - Pulse `4822 -1312 -1314 98 13`

BASE + r_highPrecisionRendering 0

SIMPLE + r_highPrecisionRendering 0

Decreased noise in dark area - Procyon `-955 3443 276 94 7`

BASE + r_highPrecisionRendering 0

SIMPLE + r_highPrecisionRendering 0

illwieckz · 2024-11-13T16:32:08Z

src/engine/renderer/Material.cpp

 	// u_ColorModulate
 	colorGen_t rgbGen = SetRgbGen( pStage );
 	alphaGen_t alphaGen = SetAlphaGen( pStage );

-	gl_genericShaderMaterial->SetUniform_ColorModulate( rgbGen, alphaGen );
+	bool mayUseVertexOverbright = pStage->type == stageType_t::ST_COLORMAP && drawSurf->bspSurface;
+	gl_genericShaderMaterial->SetUniform_ColorModulate( rgbGen, alphaGen, mayUseVertexOverbright );


What is preventing to multiply tess.svars.color in Tess_ComputeColor?

Why is the material variant calling Tess_ComputeColor but not the non-material one? Is that a dormant bug we missed?

Non-material code does Tess_ComputeColor() in Tess_StageIteratorColor(). Material system does not have that.

Ah so in non-material code, Tess_ComputeColor() is done before calling Render_generic3D(). Is it too early to multiply the light factor? Because both tess and pStage are known as well.

Well, we can merge this as it is. Possible optimizations can be done later.

Ah so in non-material code, Tess_ComputeColor() is done before calling Render_generic3D(). Is it too early to multiply the light factor? Because both tess and pStage are known as well.

I suppose that was just to avoid having to add it to all relevant stage renderers.

What is preventing to multiply tess.svars.color in Tess_ComputeColor?

u_Color is not the same thing as u_ColorModulate. The formula for the "lighting" color (which will be multiplied by the diffuse) is u_ColorModulate * (vertex color) + u_Color. For vertex lighting, u_ColorModulate is 1, or 4 with overbright without lightmap, and u_Color is 0. Otherwise, u_ColorModulate is 0, so it's u_Color that needs to be scaled for light styles.

illwieckz

LGTM.

This allows us to use values greater than 1 in the color buffer and as the color output of a fragment shader, so we don't have to worry so much about values getting clamped in multi-stage shaders. Co-authored-by: Thomas Debesse <dev@illwieckz.net>

Multiply lightmaps and light grid by the light factor right when they are used, instead of the elaborate canceling system. Vertex light multiplication in lightmapped stages is handled by the scaling of the white image lightmap (treated as 4.0). Various bugs will be fixed, since the canceling approach relied on detecting an enormous amount of special cases and not all of them were handled. In a few places there can be issues with intermediate color buffer values exceeding 1.0, but this is worked around (on reasonably good hardware) by the previous commit setting up an RGBA16F framebuffer. When limited to RGBA8 framebuffers, as can be tried with r_highPrecisionRendering 0, this rewrite is a mixed bag. In some places it leads to clamping in the color buffer (example: Pulse viewpos 4822 -1312 -1314 98 13), while in others it improves results due to the effective 2 extra bits of precision in the framebuffer (example: Procyon viewpos -955 3443 276 94 7). There are a few cases where lightmap/lightgrid scaling doesn't apply that required special handling: - For intermediate stages of light styles, the already-existing detection of STYLELIGHTMAP and STYLECOLORMAP is used. For these stage types u_Color is scaled by the light factor. Example: metro-b1-2 viewpos -1172 627 107 94 -21. - For non-lightmapped vertex-lit surfaces, u_ColorModulate is scaled by the light factor. Example: station15 small plants. - For fullbright lightmapped stages, the light factor is set to 1. Example: Procyon star chart. Some bugs (in non-clamped overbright mode) that are fixed by this commit: - Non-lightmapped vertex-lit surfaces (like the station15 plants) are now scaled by the light factor as they should be. - Procyon star chart is no longer too dark with material system. - Glowmap-like stage implemented with q3 syntax in a control panel shader are no longer wrongly overbright-scaled, causing visual noise. zittrig-arena viewpos 1201 -301 -338 87 0 Co-authored-by: Thomas Debesse <dev@illwieckz.net>

slipher mentioned this pull request Oct 31, 2024

Fix overbright bugs #1404

Merged

illwieckz reviewed Oct 31, 2024

View reviewed changes

src/engine/renderer/glsl_source/computeLight_fp.glsl Outdated Show resolved Hide resolved

slipher mentioned this pull request Oct 31, 2024

Broken lantern/candle in metro map, light emitting shader is displayed #322

Closed

illwieckz force-pushed the slipher/overbright-simple branch from 596c674 to 25a5873 Compare November 9, 2024 10:04

illwieckz force-pushed the slipher/overbright-simple branch from 77a478d to b593d73 Compare November 9, 2024 13:40

VReaperV approved these changes Nov 9, 2024

View reviewed changes

VReaperV marked this pull request as ready for review November 9, 2024 14:36

illwieckz force-pushed the slipher/overbright-simple branch from 1a4f01b to 990a80e Compare November 9, 2024 16:09

illwieckz mentioned this pull request Nov 9, 2024

tr_shader: skip standalone lightmaps, they are assumed to be buggy #1426

Merged

illwieckz force-pushed the slipher/overbright-simple branch from 990a80e to 4d63f4d Compare November 9, 2024 18:15

illwieckz changed the title ~~Why don't we do overbright like this?~~ Simplify overbright implementation Nov 9, 2024

slipher mentioned this pull request Nov 10, 2024

Set vertex colour attr correctly with material system #1431

Merged

slipher force-pushed the slipher/overbright-simple branch from 4d63f4d to eba7c4b Compare November 12, 2024 07:17

illwieckz mentioned this pull request Nov 12, 2024

renderer: split generic and generic3D #1436

Open

slipher force-pushed the slipher/overbright-simple branch from eba7c4b to 94ce0a5 Compare November 13, 2024 10:04

illwieckz reviewed Nov 13, 2024

View reviewed changes

illwieckz approved these changes Nov 13, 2024

View reviewed changes

slipher and others added 2 commits November 13, 2024 16:26

slipher force-pushed the slipher/overbright-simple branch from 94ce0a5 to 344932f Compare November 13, 2024 22:26

slipher merged commit c750866 into master Nov 14, 2024
9 checks passed

slipher deleted the slipher/overbright-simple branch November 14, 2024 01:34

Simplify overbright implementation #1406

Simplify overbright implementation #1406

Conversation

slipher commented Oct 30, 2024

DolceTriade commented Oct 30, 2024

VReaperV commented Oct 30, 2024

slipher commented Oct 31, 2024

illwieckz commented Oct 31, 2024 • edited Loading

illwieckz commented Oct 31, 2024 • edited Loading

illwieckz commented Oct 31, 2024

slipher commented Oct 31, 2024 • edited Loading

illwieckz commented Oct 31, 2024

illwieckz commented Oct 31, 2024 • edited Loading

illwieckz commented Oct 31, 2024 • edited Loading

illwieckz commented Oct 31, 2024

illwieckz commented Oct 31, 2024 • edited Loading

illwieckz commented Oct 31, 2024 • edited Loading

illwieckz commented Oct 31, 2024 • edited Loading

slipher commented Oct 31, 2024

illwieckz commented Oct 31, 2024

VReaperV commented Oct 31, 2024

illwieckz commented Oct 31, 2024

VReaperV commented Oct 31, 2024

slipher commented Nov 2, 2024

illwieckz commented Nov 2, 2024

illwieckz commented Nov 2, 2024

slipher commented Nov 2, 2024

illwieckz commented Nov 2, 2024

illwieckz commented Nov 2, 2024

illwieckz commented Nov 2, 2024

VReaperV commented Nov 4, 2024

illwieckz commented Nov 8, 2024

slipher commented Nov 8, 2024

illwieckz commented Nov 9, 2024

VReaperV commented Nov 9, 2024

VReaperV commented Nov 9, 2024

VReaperV left a comment

Choose a reason for hiding this comment

VReaperV commented Nov 9, 2024 • edited Loading

illwieckz commented Nov 9, 2024

illwieckz commented Nov 9, 2024

illwieckz commented Nov 9, 2024

slipher commented Nov 9, 2024

slipher commented Nov 12, 2024 • edited Loading

Some bugs fixed by these overbright changes

Non-lightmapped vertex-lit surface not overbright scaled (station15 462 -1428 -114 178 -8)

Improperly scaled 'glow map' (zittrig-arena 1201 -301 -338 87 0)

Procyon star chart darkened with material system

Consequences for framebuffers with only 1 byte per color

Color clamping regression - Pulse 4822 -1312 -1314 98 13

Decreased noise in dark area - Procyon -955 3443 276 94 7

illwieckz Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

VReaperV Nov 13, 2024

Choose a reason for hiding this comment

illwieckz Nov 13, 2024

Choose a reason for hiding this comment

illwieckz Nov 13, 2024

Choose a reason for hiding this comment

VReaperV Nov 13, 2024

Choose a reason for hiding this comment

slipher Nov 13, 2024

Choose a reason for hiding this comment

illwieckz left a comment

Choose a reason for hiding this comment

illwieckz commented Oct 31, 2024 •

edited

Loading

illwieckz commented Oct 31, 2024 •

edited

Loading

slipher commented Oct 31, 2024 •

edited

Loading

illwieckz commented Oct 31, 2024 •

edited

Loading

illwieckz commented Oct 31, 2024 •

edited

Loading

illwieckz commented Oct 31, 2024 •

edited

Loading

illwieckz commented Oct 31, 2024 •

edited

Loading

illwieckz commented Oct 31, 2024 •

edited

Loading

VReaperV commented Nov 9, 2024 •

edited

Loading

slipher commented Nov 12, 2024 •

edited

Loading

Non-lightmapped vertex-lit surface not overbright scaled (station15 `462 -1428 -114 178 -8`)

Improperly scaled 'glow map' (zittrig-arena `1201 -301 -338 87 0`)

Color clamping regression - Pulse `4822 -1312 -1314 98 13`

Decreased noise in dark area - Procyon `-955 3443 276 94 7`

illwieckz Nov 13, 2024 •

edited

Loading