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

Fix oil spill rendering #240

Merged
merged 2 commits into from
Nov 17, 2022
Merged

Conversation

zear
Copy link
Collaborator

@zear zear commented Nov 11, 2022

Due to the wrong order of vertices in the model, oil spill object is subject to back face culling, preventing it from being rendered. Make the oil spill front faced w/ regards to the floor.

Note: Setting BR_MATF_TWO_SIDED for the material flags does not solve this problem - the oil spill is still invisible. Has the original order of vertices been taken from OG disassembly?

@madebr
Copy link
Collaborator

madebr commented Nov 11, 2022

I just checked and the ordering is 1:1 as used in CARM95.EXE.

I think the issue is our renderer does not respect the BR_MATF_TWO_SIDED flag.
Grep'ing for BR_MATF_TWO_SIDED gives no matches in neither src/BRSRC13 and src/harness.

@madebr
Copy link
Collaborator

madebr commented Nov 11, 2022

Btw, you're always welcome to join the Carmageddon Discord server to discuss various problems.

@madebr
Copy link
Collaborator

madebr commented Nov 12, 2022

I also just noticed the non-rendering of an oil spill, even though it caused the tyres to slip and an oil track to form.
I'm wondering what is the OG behavior.

@zear
Copy link
Collaborator Author

zear commented Nov 12, 2022

Updated the patch with gl renderer support of BR_MATF_TWO_SIDED.
This works by disabling culling for the time the two-sided model is being rendered, and then re-enabling it afterwards. I am not familiar with OpenGL API, so hopefully this is the right approach. Seems to work just fine with my renderer.

The oil spills now also render at the same time other back-faced geometry is properly culled (#187):
spill

Btw, you're always welcome to join the Carmageddon Discord server to discuss various problems.

Thanks, but I'm not using walled garden ecosystems. Unless you guys have an IRC channel :)

@@ -549,6 +550,10 @@ void GLRenderer_Model(br_actor* actor, br_model* model, br_matrix34 model_matrix
glUniform1i(uniforms_3d.light_value, -1);
}

twosided = actor->material && actor->material->flags & BR_MATF_TWO_SIDED;
if (twosided)
glDisable(GL_CULL_FACE);
Copy link
Owner

Choose a reason for hiding this comment

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

I would put this code in SetActiveMaterial.

And by framing it like this, we don’t have to have a separate piece of reset code.
if (double sided)
glDisable(culling)
else
glEnable(culling

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought about this initially, but the problem with doing so is that you can leave the culling disabled if the last model had BR_MATF_TWO_SIDED. So if anything else is rendered in the frame after the models, it might cause problems.

Copy link
Owner

@dethrace-labs dethrace-labs Nov 12, 2022

Choose a reason for hiding this comment

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

I think that’s ok - either the next thing to be rendered is:

  • another model with its own materials which will setup the rendering
    config with its own settings
  • or that model was the last one, and we move on
    to rendering the framebuffer. In this case it doesn’t care about
    culling, but generally should be setting up the rendering config regardless of
    what the current config is

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the patch to reflect the discussion.

@@ -41,6 +41,7 @@ void InitOilSpills() {
the_material->flags |= BR_MATF_LIGHT;
the_material->flags |= BR_MATF_PERSPECTIVE;
the_material->flags |= BR_MATF_SMOOTH;
the_material->flags |= BR_MATF_TWO_SIDED;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
the_material->flags |= BR_MATF_TWO_SIDED;
// TODO: added by dethrace, investigate why oil spills in OG do not need this flag set to render correctly
the_material->flags |= BR_MATF_TWO_SIDED;

Comment on lines 503 to 508
if (material->flags & BR_MATF_TWO_SIDED)
glDisable(GL_CULL_FACE);
else
glEnable(GL_CULL_FACE);

Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if (material->flags & BR_MATF_TWO_SIDED)
glDisable(GL_CULL_FACE);
else
glEnable(GL_CULL_FACE);
if (material->flags & BR_MATF_TWO_SIDED || material->flags & BR_MATF_ALWAYS_VISIBLE) {
glDisable(GL_CULL_FACE);
} else {
glEnable(GL_CULL_FACE);
}

Copy link
Owner

Choose a reason for hiding this comment

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

there is another flag BR_MATF_ALWAYS_VISIBLE that we should probably treat the same way

…SIBLE

Prevent the gl renderer from culling back-faced textures if the material
is two-sided or always visible.

Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
Due to the wrong order of vertices in the model, oil spill object is
subject to back face culling, preventing it from being rendered. Set
the oil spill material as two-sided, allowing it to be displayed.

Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
Copy link
Owner

@dethrace-labs dethrace-labs left a comment

Choose a reason for hiding this comment

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

LGTM!

@dethrace-labs dethrace-labs merged commit a150407 into dethrace-labs:main Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants