Skip to content

screenSpace: make it work when GL_EXT_gpu_shader4 is missing #1739

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 15, 2025

Conversation

illwieckz
Copy link
Member

WIP: screenSpace: make it work when GL_EXT_gpu_shader4 is missing

It doesn't work yet, I'm not sure to understand how that works.

The purpose is to slightly modify #1593:

Commit 5666be1 from #1593 brings many improvements, but also makes use of gl_VertexID, which is introduced by EXT_gpu_shader4 if I'm right, making it mandatory.

So I tried to add an alternate code path based on specific parts I read from 5666be1 that replaced the old code (among many other things), but it's not working yet.

It's now probably very close to have it working so if you have any idea, please share. ☺️

Sadly I do not have the artistic talent to render a new coordinate
system for the ASCII art square so I had to delete it :-(  --slipher

Co-Authored-By: slipher <[email protected]>
@illwieckz
Copy link
Member Author

This is how it currently looks:

unvanquished_2025-08-08_233356_000

@slipher
Copy link
Member

slipher commented Aug 9, 2025

I tried it and it rendered correctly only the upper right quarter of the screen. So I tried this and it worked:

--- a/src/engine/renderer/tr_vbo.cpp
+++ b/src/engine/renderer/tr_vbo.cpp
@@ -555,7 +555,7 @@ void R_BindNullIBO()

 static void R_InitGenericVBOs() {
        // Min and max coordinates of the quad
-       static const vec3_t min = { 0.0f, 0.0f, 0.0f };
+       static const vec3_t min = { -1.0f, -1.f, -1.0f };
        static const vec3_t max = { 1.0f, 1.0f, 0.0f };
        {
                /*

I don't know why it would be necessary to use -1 to 1 now if 0 to 1 worked before. Note that this might break other, non-screenspace uses of genericQuad.

Your screenshot seems to have additional bugs so I guess there must be more incompatible things if you have the real old hardware.

@illwieckz
Copy link
Member Author

We switched something from a quad to a triangle, this explains why:

And to have a triangle covering all the screen we need the triangle being larger than the screen.

So maybe that's related. We may have modified some generic defaults for the triangle code, and when running in no-gpu_shader4 code path, if we want to keep most of the triangle code unmodified, maybe using -1 as quad min makes sure the quad fits the whole screen in that triangle? @VReaperV may know better about it.

I'll try the -1 trick soon. 🙂

That Intel computer is used in some family vacation home and is used by teens to play games in the holidays, I would like to keep the game working on it. ☺️

@illwieckz
Copy link
Member Author

illwieckz commented Aug 9, 2025

Your screenshot seems to have additional bugs so I guess there must be more incompatible things if you have the real old hardware.

Yes those were driver bugs, after an update they disappeared.

So, when I use both:

I get this (plat23):

unvanquished_2025-08-09_172927_000

The forcefields are a bit dim because that's an sRGB-compiled map running in the linear pipeline, so it's not because of an engine bug or a driver bug on this specific hardware. This produces what we currently expect.

@illwieckz
Copy link
Member Author

illwieckz commented Aug 9, 2025

And when also using:

I get this (atcshd):

unvanquished_2025-08-09_173755_000

This is using the lowest graphics preset and then the vertex lighting, so that ugliness is expected, I see no bug there.

@illwieckz illwieckz changed the title WIP: screenSpace: make it work when GL_EXT_gpu_shader4 is missing screenSpace: make it work when GL_EXT_gpu_shader4 is missing Aug 9, 2025
@illwieckz illwieckz marked this pull request as ready for review August 9, 2025 16:45
@illwieckz illwieckz force-pushed the illwieckz/screenspace-nogpushader4 branch from 7f5ed27 to 579d0cd Compare August 9, 2025 16:45
@slipher
Copy link
Member

slipher commented Aug 10, 2025

PTAL at the branchslipher/1739-fixup. There I have changed the projection matrix of Tess_InstantQuad so it can produce the correct results while having replaced the [0, 1] square with [-1, 1].

@illwieckz
Copy link
Member Author

I don't mind the implementation, if you prefer yours please force-push this PR.

I've tested your implementation and it works as well.

Maybe @VReaperV has an opinion on that too.

@illwieckz
Copy link
Member Author

Screenshots with your branch (and all the other patches listed above), but this time with the low preset:

unvanquished_2025-08-11_011242_000

unvanquished_2025-08-11_011756_000

@illwieckz
Copy link
Member Author

illwieckz commented Aug 10, 2025

I forgot to mention but outside of actually making the engine works on hardware without GL_EXT_gpu_shader4, this would also make it consistent because we actually disable GL_EXT_gpu_shader4 on GLSL 1.20 because of having a different syntax on some stuff like unsigned int and uint, as discussed here:

So, without this branch, when this extension is available, on some hardware we disable some GL_EXT_gpu_shader4 code while still using the extension, which is a bit confusing and inconsistent.

@slipher slipher force-pushed the illwieckz/screenspace-nogpushader4 branch from 579d0cd to a8d6b36 Compare August 11, 2025 15:36
@slipher
Copy link
Member

slipher commented Aug 11, 2025

I don't mind the implementation, if you prefer yours please force-push this PR.

Done. The point of my edit is to avoid breaking the Tess_InstantQuad semantics. Breaking that might not have any consequences outside of r_showBspNodes for now, but it would be a stumbling block for the future.

@illwieckz illwieckz force-pushed the illwieckz/screenspace-nogpushader4 branch from a8d6b36 to f7b3f60 Compare August 12, 2025 01:28
@slipher
Copy link
Member

slipher commented Aug 12, 2025

You have pushed the branch again and it no longer contains my changes from slipher/1739-fixup.

@illwieckz illwieckz force-pushed the illwieckz/screenspace-nogpushader4 branch 2 times, most recently from a4ac5cb to 3ef8cd9 Compare August 13, 2025 00:55
@illwieckz
Copy link
Member Author

You have pushed the branch again and it no longer contains my changes from slipher/1739-fixup.

Oupsie, that should now be fixed.

I also added comments linking to this PR so future selves may know why we picked those numbers.

@@ -550,12 +558,15 @@ void Tess_InstantQuad( u_ModelViewProjectionMatrix &shader, const float x, const

Tess_Begin( Tess_StageIteratorDummy, nullptr, true, -1, 0 );

/* These values make possible to use either the legacy quad rendering
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of this function is to render any screen-space axis-aligned rectangle specified by x, y, width, height so the comment seems out of place.

@@ -554,24 +554,19 @@ void R_BindNullIBO()
}

static void R_InitGenericVBOs() {
/* These values make possible to use either the legacy quad rendering
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To explain the values chosen here, you could say that they are chosen to specify a full-screen rectangle for screen-space shaders without a projection matrix. The values here don't influence the triangle but it's good to mention that this is a fallback when the gl_VertexID-based triangle can't be used.

@illwieckz illwieckz force-pushed the illwieckz/screenspace-nogpushader4 branch from 3ef8cd9 to 28eabd0 Compare August 14, 2025 17:25
@illwieckz
Copy link
Member Author

I updated the comments.

@slipher
Copy link
Member

slipher commented Aug 14, 2025

LGTM

@illwieckz illwieckz merged commit 1bb5e9e into master Aug 15, 2025
9 checks passed
@illwieckz illwieckz deleted the illwieckz/screenspace-nogpushader4 branch August 15, 2025 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants