Get rid of a bunch of duplicate code in map loading and srf*_t by VReaperV · Pull Request #1635 · DaemonEngine/Daemon

VReaperV · 2025-04-01T13:05:48Z

I've run into a bunch of duplicate code while making some improvements to map processing, any improvements to which required to add more such duplication, while the code underneath is the exact same.

This pr NUKES srfTriangles_t and srfSurface_t, and moves all the common information between them, srfGridMesh_t, and srfVBOMesh_t , into srfGeneric_t.

The common parts of ParseFace() and ParseTriSurf() are moved into a new ParseTriangleSurface(), which is all of it except the plane calculation. The comment about .lwo models in ParseTriSurf() is irrelevant, because we don't support those.

ParseTriangleSurface() and ParseMesh() now use SphereFromBounds() instead of FinishGenericSurface(), because the only difference between the 2 was just duplicated code.

VReaperV · 2025-04-01T13:26:24Z

CI fail is unrelated, and is due to Ubuntu 20.04 being phased out on azure pipelines.

slipher

There are some tiny model lighting changes from Move some common stuff from srf* to srfGeneric_t and Move duplicate code from ParseFace() and ParseTriSurf() into ParseTriangleSurface(). I will try to check later if those diffs are anything.

src/engine/renderer/tr_local.h

VReaperV · 2025-04-03T11:52:31Z

There are some tiny model lighting changes from Move some common stuff from srf* to srfGeneric_t and Move duplicate code from ParseFace() and ParseTriSurf() into ParseTriangleSurface(). I will try to check later if those diffs are anything.

For BSP models? I don't think other models can even be affected by this.

slipher · 2025-04-04T09:02:50Z

No, it was a buildable model.

VReaperV · 2025-04-04T09:18:44Z

That's weird... Buildings should only ever use srfVBOMD5Mesh_t or srfVBOMDVMesh_t.

slipher · 2025-04-04T21:37:32Z

I can reproduce a similar small diff by adding unused fields to srfGeneric_t, e.g.

diff --git a/src/engine/renderer/tr_local.h b/src/engine/renderer/tr_local.h
index fe658c0b2..c3c4a9b3d 100644
--- a/src/engine/renderer/tr_local.h
+++ b/src/engine/renderer/tr_local.h
@@ -1738,9 +1738,15 @@ enum class ssaoMode {
        {
                surfaceType_t surfaceType;
 
+               int argle;
+
                // culling information
                vec3_t   bounds[ 2 ];
+
+               int bargle;
+
                vec3_t   origin;
+
                float    radius;
        };

Maybe the layout is assumed somewhere else.

slipher · 2025-04-04T21:52:06Z

Without/with the above patch, the lower part of the Overmind looks a bit more brightly lit on the before side.

slipher · 2025-04-05T02:49:57Z

The lighting diffs reproduce with different OS, compiler, and GPU.

I tried getting rid of srfGeneric_t and commenting out or rewriting all places that use it. After that I still get the same diff by adding extra fields in srfVBOMesh_t only.

The diffs go away if I disable r_deluxeMapping.

slipher · 2025-04-05T04:11:34Z

I found the cause of that stuff in #1639.

VReaperV · 2025-04-05T07:49:02Z

Thanks, that is very helpful!

`srfTriangles_t` doesn't have any difference with `srfGeneric_t` now.

slipher

I think the last commit, NUKE srfSurfaceFace_t, should be dropped. It will just cause confusion for one of the surface types to not have its own struct. But a couple changes from that commit of changing some casts to srfGeneric_t * should be kept (these fix comments I made on earlier commits complaining of casting to an inappropriate type).

slipher · 2025-04-08T06:48:56Z

src/engine/renderer/tr_bsp.cpp

+			|| *surface->data == surfaceType_t::SF_TRIANGLES )
 		{
-			srfSurfaceFace_t *face = ( srfSurfaceFace_t * ) surface->data;
+			srfSurfaceFace_t* srf = ( srfSurfaceFace_t * ) surface->data;


Cast to wrong type

src/engine/renderer/tr_bsp.cpp

src/engine/renderer/tr_local.h

src/engine/renderer/tr_bsp.cpp

src/engine/renderer/tr_main.cpp

VReaperV · 2025-04-08T09:10:31Z

I think the last commit, NUKE srfSurfaceFace_t, should be dropped. It will just cause confusion for one of the surface types to not have its own struct.

But this is also the case with srfTriangles_t.

slipher · 2025-04-08T09:12:53Z

I think the last commit, NUKE srfSurfaceFace_t, should be dropped. It will just cause confusion for one of the surface types to not have its own struct.

But this is also the case with srfTriangles_t.

OK then I complain about that one too 😛

VReaperV · 2025-04-08T09:18:08Z

I guess I can revert those, though I don't see much point in keeping around structs that have nothing in them other than stuff they inherit.

slipher · 2025-04-08T09:21:22Z

I guess it's fine as long as there are comments on the SF_FACE etc. enum.

VReaperV · 2025-04-08T10:24:54Z

I've added the comments into srfGeneric_t.

…angleSurface() Also moves `plane` from `srfSurfaceFace_t` to `srfSurfaceGeneric_t`, since it's the only difference between them, and plane computations were already being done on `srfSurfaceGeneric_t` anyway, so they can be moved there too.

The only other code in `FinishGenericSurface()` was duplicate from `ParseFace()`. Also moved both `ShaderForShaderNum()` and `SphereFromBounds()` closer to where they're actually used.

It is now the same as `srfGeneric_t`, so just use that instead.

slipher · 2025-04-09T13:15:51Z

src/engine/renderer/tr_local.h

-		// triangle definitions
-		int           numTriangles;
-		srfTriangle_t *triangles;
+		cplane_t plane; // Valid in: SF_FACE, SF_TRIANGLES


Does SF_TRIANGLES really have a plane? It doesn't make much sense and the old srfTriangles_t doesn't have it.

Also I don't think it's necessary to have comments on ones that are always valid; you can remove those if you want.

I thought that too, but R_PlaneForSurface() was already using the first triangle's plane as surface plane for SF_TRIANGLES:

Daemon/src/engine/renderer/tr_main.cpp

Lines 1226 to 1234 in bc09529

case surfaceType_t::SF_TRIANGLES:

tri = ( srfTriangles_t * ) surfType;

v1 = tri->verts + tri->triangles[ 0 ].indexes[ 0 ];

v2 = tri->verts + tri->triangles[ 0 ].indexes[ 1 ];

v3 = tri->verts + tri->triangles[ 0 ].indexes[ 2 ];

PlaneFromPoints( plane4, v1->xyz, v2->xyz, v3->xyz );

VectorCopy( plane4.normal, plane->normal );

plane->dist = plane4.dist;

return;

For code comments, I'll adjust them in a later pr, since I found some more things that can be cleaned up there, but don't want to overload this pr.

I see. It seems R_PlaneForSurface is only used for portals.

slipher · 2025-04-11T11:43:46Z

LGTM

VReaperV added A-Renderer T-Cleanup labels Apr 1, 2025

VReaperV force-pushed the generic-srf branch from f9f73f4 to b271cd5 Compare April 1, 2025 13:23

slipher reviewed Apr 2, 2025

View reviewed changes

src/engine/renderer/tr_local.h Outdated Show resolved Hide resolved

VReaperV force-pushed the generic-srf branch 2 times, most recently from df6eecc to 04e9f54 Compare April 4, 2025 21:01

VReaperV added 4 commits April 6, 2025 13:13

NUKE unused MAX_FACE_POINTS

f08438e

Move some common stuff from srf* to srfGeneric_t

148bf37

Change srfTriangles_t to srfGeneric_t

37f0741

`srfTriangles_t` doesn't have any difference with `srfGeneric_t` now.

NUKE some duplicated code

439ded0

VReaperV force-pushed the generic-srf branch from 04e9f54 to 7225e7d Compare April 6, 2025 10:14

VReaperV mentioned this pull request Apr 6, 2025

Add GeometryOptimiser, move some stuff there from tr_bsp #1640

Merged

slipher reviewed Apr 8, 2025

View reviewed changes

VReaperV added 3 commits April 8, 2025 13:41

Move SF_TRIANGLES plane calculation to map loading

6026757

NUKE duplicate code from FinishGenericSurface()

a1c061e

VReaperV added 5 commits April 8, 2025 13:41

Use SphereFromBounds() instead of FinishGenericSurface()

c5c12f2

The only other code in `FinishGenericSurface()` was duplicate from `ParseFace()`. Also moved both `ShaderForShaderNum()` and `SphereFromBounds()` closer to where they're actually used.

NUKE srfSurfaceFace_t

e796514

It is now the same as `srfGeneric_t`, so just use that instead.

Don't override SF_SKIP surfaces

ed43689

Make surface parse error messages more meaningful

a0355e6

Add comments to tell which fields in srfGeneric_t are valid

76e74e6

VReaperV force-pushed the generic-srf branch from ba40318 to 76e74e6 Compare April 8, 2025 10:42

srfGeneric_t face->srf

169984e

slipher reviewed Apr 9, 2025

View reviewed changes

VReaperV merged commit e14bb7a into DaemonEngine:master Apr 11, 2025
9 checks passed

VReaperV deleted the generic-srf branch April 11, 2025 12:21

	case surfaceType_t::SF_TRIANGLES:
	tri = ( srfTriangles_t * ) surfType;
	v1 = tri->verts + tri->triangles[ 0 ].indexes[ 0 ];
	v2 = tri->verts + tri->triangles[ 0 ].indexes[ 1 ];
	v3 = tri->verts + tri->triangles[ 0 ].indexes[ 2 ];
	PlaneFromPoints( plane4, v1->xyz, v2->xyz, v3->xyz );
	VectorCopy( plane4.normal, plane->normal );
	plane->dist = plane4.dist;
	return;

Conversation

VReaperV commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VReaperV commented Apr 1, 2025

Uh oh!

slipher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

VReaperV commented Apr 3, 2025

Uh oh!

slipher commented Apr 4, 2025

Uh oh!

VReaperV commented Apr 4, 2025

Uh oh!

slipher commented Apr 4, 2025

Uh oh!

slipher commented Apr 4, 2025

Uh oh!

slipher commented Apr 5, 2025

Uh oh!

slipher commented Apr 5, 2025

Uh oh!

VReaperV commented Apr 5, 2025

Uh oh!

slipher left a comment

Choose a reason for hiding this comment

Uh oh!

slipher Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

VReaperV commented Apr 8, 2025

Uh oh!

slipher commented Apr 8, 2025

Uh oh!

VReaperV commented Apr 8, 2025

Uh oh!

slipher commented Apr 8, 2025

Uh oh!

VReaperV commented Apr 8, 2025

Uh oh!

slipher Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

VReaperV Apr 9, 2025 • 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 Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

slipher Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

slipher commented Apr 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

VReaperV commented Apr 1, 2025 •

edited

Loading

VReaperV Apr 9, 2025 •

edited

Loading