-
Notifications
You must be signed in to change notification settings - Fork 60
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
base: master
Are you sure you want to change the base?
Conversation
For me it fixes plat23, but introduces a regression on Watah, viewpos 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. |
Hmm, so there are maps with more than 1 skybox. I think I know how to fix it in that case. |
256c8b6
to
4fa3b84
Compare
This is fixed now. |
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.
4fa3b84
to
bdbdd24
Compare
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. |
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. |
I fixed the above issue, but this fix seems to degrade performance considerably. |
Something wrong here:
|
31d60b0
to
aac0e5a
Compare
Ah, right, should be set to false. Still doesn't fix the performance issue however. |
I have some ideas on how to fix this without decreasing performance, just need to correctly select which skybox to draw. |
There was a problem hiding this 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 ); |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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 ) { |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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.