Lighting-related cleanup; make r_dynamicLight non-latch by slipher · Pull Request #1341 · DaemonEngine/Daemon

slipher · 2024-10-05T09:25:16Z

Remove unused and unneeded lighting code
Gate some tiled renderer-specific code behind the tiled cvar
Make r_dynamicLight non-latch, to quickly toggle dynamic lights

VReaperV

In commit "Remove redundant realtime light enabled tests" I assume that "If a type of realtime light is enabled, just don't add it to the scene." is a typo?

VReaperV · 2024-10-05T22:23:10Z

(moved from review since this would require targeting for-0.55) This:

Daemon/src/engine/renderer/tr_backend.cpp

Line 2696 in 14e7fb0

void RB_RunVisTests( )

apparently uses the same functionality and can be nuked as well later. It's only used for lightflares and seems to be pretty broken, so I assume it was just some proof of concept. Having to do extra IPC calls makes it even worse.

slipher · 2024-10-06T15:16:11Z

I'm not planning to nuke vis tests. What's being deleted here is

An extension cvar r_ext_occlusion_query which has no effect. Although we do use GL_ARB_occlusion query functionality, this usage is not actually gated by the cvar. I updated the commit message to clarify that it's the cvar that is unused, not the extension.
The vis bounds expansion code which was apparently needed for a different occlusion test code path that was removed in 2014. The vis tests that we still have are used for a similar purpose (seeing if a "light" would shine on certain areas), the vis bounds change code wouldn't do anything useful for it because light flares are just textures, not dynamic lights.

VReaperV · 2024-10-06T15:37:11Z

I see, thanks for the explanation.

visBounds is used only to choose zFar. A comment says that expanding the vis bounds was needed for occlusion queries (which were NUKED in 2014). I looked at the nuking commit ffd1714 and it seems that the occlusion code involved GL draw calls to "draw" the boundaries of the light and check whether anything would show up. So it's plausible that the expanded zFar would be needed for that stuff. But it shouldn't be needed now.

slipher · 2024-10-07T17:50:29Z

Regrettably, I have added more stuff to the PR:

NUKE static lights
Rename r_dynamicLight to r_drawDynamicLights and remove latch flag

VReaperV · 2024-10-07T20:24:02Z

src/engine/renderer/tr_light.cpp

@@ -1622,34 +1568,6 @@ void R_ComputeFinalAttenuation( shaderStage_t *pStage, trRefLight_t *light )
 =================
 R_CullLightTriangle


Leftover comment.

It wasn't me. The banner comment for R_CullLightWorldBounds already says "R_CullLightTriangle"

slipher · 2024-10-07T22:56:32Z

The default diff algorithm generates garbage for the static light nuking commit in tr_bsp.cpp. Reviewers may wish to view commits locally with a better algorithm, for example:

git fetch origin pull/1341/head
git -c diff.algorithm=patience show -9 FETCH_HEAD

A "static light" was a type of realtime light. It was instantiated by a BSP entity with classname "light", which must have the "noradiosity" keyword set in order to be rendered. Judging by https://users.unvanquished.net/~slipher/map-entity-directory.txt there are no Tremulous or Unvanquished maps using this. 90% of the code for this feature was optimizations specific to the deprecated forward renderer. (Optimizations in the sense that the code is intended to be faster than the dynamic light code path.) If we ever wanted to test real-time lights from map entities later, it could be done by just adding a dynamic light based on the BSP entity. The entity parser would be the only part that's needed. Closes DaemonEngine#1051.

We do use GL_ARB_occlusion_query functionality, but the cvar is not actually consulted before doing it.

If a dynamic light is disabled, just don't add it to the scene. No need to check again later if it is disabled

Giving the cvar for whether to add dynamic lights at runtime a different name from r_realtimeLighting's old name will help it to avoid getting mistakenly set by old autogen.cfg values. The name name follows the convention that r_drawXXX cvars that can be used to disable something that is usually rendered. (showXXX cvars on the other hand are cheat cvars for drawing a representation of something that is usually unseen.) Remove Cvar::Latch so it can be toggled instantly.

illwieckz · 2024-10-10T16:10:52Z

src/engine/renderer/tr_cmds.cpp

+	     r_realtimeLightingRenderer.Get() != Util::ordinal( realtimeLightingRenderer_t::TILED ) )
+	{
+		return;
+	}


Totally nitpicking but I prefer that syntax:

if ( !glConfig2.realtimeLighting ) { return; } if ( r_realtimeLightingRenderer.Get() != Util::ordinal( realtimeLightingRenderer_t::TILED ) ) { return; }

This is the pattern we usually implement (and I usually write new code or modify code this way), for example:

Daemon/src/engine/renderer/tr_scene.cpp

Lines 288 to 301 in 926d8f6

if ( !tr.registered )

{

return;

}

if ( r_numLights >= MAX_REF_LIGHTS )

{

return;

}

if ( l->radius <= 0 && !VectorLength( l->projTarget ) )

{

return;

}

I see it as one condition. "Is the tiled renderer active"

illwieckz

LGTM.

VReaperV reviewed Oct 5, 2024

View reviewed changes

slipher force-pushed the lightstuff branch 2 times, most recently from 0fee59e to 7d1d420 Compare October 6, 2024 15:11

slipher added 2 commits October 7, 2024 11:06

Delete 2 unused renderer functions

ba3d458

slipher force-pushed the lightstuff branch from 7d1d420 to 658cbfe Compare October 7, 2024 17:48

slipher mentioned this pull request Oct 7, 2024

ui, presets: adjust for new cvars and cvar types Unvanquished/Unvanquished#2894

Merged

VReaperV reviewed Oct 7, 2024

View reviewed changes

slipher added 7 commits October 8, 2024 13:28

Remove unused GL extension cvar (occlusion_query)

44effb2

We do use GL_ARB_occlusion_query functionality, but the cvar is not actually consulted before doing it.

Remove never-read field trRefLight_t::cull

8c72fc9

Run some tiled lighting code only in tiled mode

d23e076

Remove redundant dynamic light enabled tests

0139b9f

If a dynamic light is disabled, just don't add it to the scene. No need to check again later if it is disabled

Fix forward lighting binding null image

7ec38eb

slipher force-pushed the lightstuff branch from 658cbfe to 7ec38eb Compare October 8, 2024 18:28

slipher mentioned this pull request Oct 9, 2024

Runtime VBO layout: make half float usage optional #1344

Merged

illwieckz reviewed Oct 10, 2024

View reviewed changes

illwieckz approved these changes Oct 10, 2024

View reviewed changes

slipher merged commit afd87da into DaemonEngine:master Oct 10, 2024

slipher deleted the lightstuff branch October 10, 2024 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lighting-related cleanup; make r_dynamicLight non-latch#1341

Lighting-related cleanup; make r_dynamicLight non-latch#1341
slipher merged 9 commits intoDaemonEngine:masterfrom
slipher:lightstuff

slipher commented Oct 5, 2024

Uh oh!

VReaperV left a comment •

edited

Loading

Uh oh!

VReaperV commented Oct 5, 2024

Uh oh!

slipher commented Oct 6, 2024

Uh oh!

VReaperV commented Oct 6, 2024

Uh oh!

slipher commented Oct 7, 2024

Uh oh!

VReaperV Oct 7, 2024

Uh oh!

slipher Oct 7, 2024

Uh oh!

slipher commented Oct 7, 2024

Uh oh!

illwieckz Oct 10, 2024

Uh oh!

slipher Oct 10, 2024

Uh oh!

illwieckz left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -1622,34 +1568,6 @@ void R_ComputeFinalAttenuation( shaderStage_t pStage, trRefLight_t light )
		=================
		R_CullLightTriangle

	if ( !tr.registered )
	{
	return;
	}

	if ( r_numLights >= MAX_REF_LIGHTS )
	{
	return;
	}

	if ( l->radius <= 0 && !VectorLength( l->projTarget ) )
	{
	return;
	}

Conversation

slipher commented Oct 5, 2024

Uh oh!

VReaperV left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VReaperV commented Oct 5, 2024

Uh oh!

slipher commented Oct 6, 2024

Uh oh!

VReaperV commented Oct 6, 2024

Uh oh!

slipher commented Oct 7, 2024

Uh oh!

VReaperV Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

slipher Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

slipher commented Oct 7, 2024

Uh oh!

illwieckz Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

slipher Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

illwieckz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

VReaperV left a comment •

edited

Loading