Skip to content
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

Generate skybrush material #1299

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

VReaperV
Copy link
Contributor

Generate a material for skybrushes and use it with the stencil buffer to prevent material system from rendering stuff over the skybox which would be normally culled by the BSP.

Fixes #1249.

@VReaperV VReaperV added T-Bug T-Improvement Improvement for an existing feature A-Renderer labels Sep 13, 2024
@slipher
Copy link
Member

slipher commented Sep 14, 2024

For me it fixes plat23, but introduces a regression on Watah, viewpos -8657 3096 179 177 5:

unvanquished_2024-09-14_063550_000

This is a special map because it has two skyboxes. For now the material system always shows the red one, but that happens to be the correct one at that spot. It shouldn't look like a box though.

@VReaperV
Copy link
Contributor Author

Hmm, so there are maps with more than 1 skybox. I think I know how to fix it in that case.

@VReaperV VReaperV force-pushed the generate-skybrush-material branch from 256c8b6 to 4fa3b84 Compare September 14, 2024 22:09
@VReaperV
Copy link
Contributor Author

For me it fixes plat23, but introduces a regression on Watah, viewpos -8657 3096 179 177 5:

unvanquished_2024-09-14_063550_000

This is a special map because it has two skyboxes. For now the material system always shows the red one, but that happens to be the correct one at that spot. It shouldn't look like a box though.

This is fixed now.

@cu-kai
Copy link
Contributor

cu-kai commented Sep 14, 2024

Hmm, so there are maps with more than 1 skybox. I think I know how to fix it in that case.

Another map with >1 skybox you may wish to test on is tremship.

@VReaperV
Copy link
Contributor Author

Another map with >1 skybox you may wish to test on is tremship.

Thanks. It's still a bit broken there, but an improvement over what we had.

Generate a material for skybrushes and use it with the stencil buffer to prevent material system from rendering stuff over the skybox which would be normally culled by the BSP.
@VReaperV VReaperV force-pushed the generate-skybrush-material branch from 4fa3b84 to bdbdd24 Compare September 15, 2024 11:23
@slipher
Copy link
Member

slipher commented Sep 15, 2024

Great, now both of the skyboxes can be drawn depending on where you are.

The spot I screenshotted above isn't 100% fixed FWIW. If you look through at an oblique angle, you can see some movers floating in space. But maybe that just needs to wait for other entities to be migrated to the material system.

@VReaperV
Copy link
Contributor Author

Yeah, I'm not sure yet what is causing that issue. On master though, you can see the whole room with those surfaces in that spot.

@VReaperV
Copy link
Contributor Author

I fixed the above issue, but this fix seems to degrade performance considerably.

@slipher
Copy link
Member

slipher commented Sep 16, 2024

Something wrong here:

	if ( environmentNoFogDraw && renderSkyBrushDepth ) {
		renderSkyBrushDepth = true;
	}

@VReaperV VReaperV force-pushed the generate-skybrush-material branch from 31d60b0 to aac0e5a Compare September 16, 2024 21:54
@VReaperV
Copy link
Contributor Author

Ah, right, should be set to false. Still doesn't fix the performance issue however.

@VReaperV
Copy link
Contributor Author

I have some ideas on how to fix this without decreasing performance, just need to correctly select which skybox to draw.

@VReaperV VReaperV marked this pull request as draft September 21, 2024 07:39
Copy link
Member

@slipher slipher left a comment

Choose a reason for hiding this comment

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

I just discovered some ancient pending review comments which were never sent out. Sending them now lol

@@ -124,7 +126,8 @@ struct Material {

bool operator==( const Material& other ) {
return program == other.program && stateBits == other.stateBits && vbo == other.vbo && ibo == other.ibo
&& cullType == other.cullType && usePolygonOffset == other.usePolygonOffset;
&& cullType == other.cullType && usePolygonOffset == other.usePolygonOffset
&& ( skyShader == nullptr || other.skyShader == nullptr || skyShader == other.skyShader );
Copy link
Member

Choose a reason for hiding this comment

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

Why can't it just be skyShader == other.skyShader?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC I did it because if one material has a skybox shader and the other doesn't, then they still can be considered the same.

@@ -5806,6 +5813,22 @@ static shader_t *FinishShader()
numStages = MAX_SHADER_STAGES;
GroupActiveStages();

if ( glConfig2.materialSystemAvailable && shader.isSky ) {
Copy link
Member

Choose a reason for hiding this comment

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

Needs a check that we don't overflow the stages array

glStencilFunc( GL_EQUAL, backEnd.viewParms.portalLevel, 0xff );
glStencilOp( GL_KEEP, GL_KEEP, GL_INCR );

glState.glStateBitsMask = 0;
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not needed, it should be 0 to start with

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was breaking otherwise with portals. Maybe not and I put there just in case because it was breaking elsewhere, but either way doesn't matter now because this way of rendering the skybox seems infeasible. Well, maybe I'll actually keep the stenciled skybrush part to reset depth, I'll need to look at it again later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Renderer T-Bug T-Improvement Improvement for an existing feature
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

Material system: can see through walls on plat23
3 participants