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

WIP cull mode specialization #3734

Closed
wants to merge 7 commits into from

Conversation

rparrett
Copy link
Contributor

@rparrett rparrett commented Jan 21, 2022

Objective

Fixes #3729

Solution

Add cull mode specialization

Caveats

I never got around to tidying this up, but I'm pushing it here for the sake of potential collaboration.

  • It seems like the front / back face end up being lit slightly differently. Is the "normal flip" right?
  • Does it make sense to use 2 bits in the MeshPipelineKey for every possible cull mode? Or is on/off enough?
  • If we're already using 2 bits, should I just use two separate fields for ON/OFF and FRONT/BACK?
  • I ended up sprinkling a bunch of fn double_sided on various material... things. My "generic rust" skills aren't fantastic. Are all these even necessary?

Marking as a draft PR for now.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 21, 2022
@rparrett
Copy link
Contributor Author

Fixed a stray unrelated modification

@james7132 james7132 added A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior and removed S-Needs-Triage This issue needs to be labelled labels Jan 21, 2022
Copy link
Member

@aevyrie aevyrie left a comment

Choose a reason for hiding this comment

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

Looks good, only uncertain about the bit mask for the specialization flag.

crates/bevy_pbr/src/render/mesh.rs Outdated Show resolved Hide resolved
@rparrett
Copy link
Contributor Author

Okay, I've at least documented all the known concerns in the PR body.

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

As mentioned, it's basically good. Just cleaning up a few rough edges. :)

crates/bevy_pbr/src/material.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/material.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/render/mesh.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/render/mesh.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/render/mesh.rs Outdated Show resolved Hide resolved
rparrett and others added 6 commits January 21, 2022 06:31
Co-authored-by: Robert Swain <robert.swain@gmail.com>
Co-authored-by: Robert Swain <robert.swain@gmail.com>
Co-authored-by: Robert Swain <robert.swain@gmail.com>
Co-authored-by: Robert Swain <robert.swain@gmail.com>
@rparrett
Copy link
Contributor Author

Thanks for the feedback, explanation, and janitorial work.

Here's the potential issue I was seeing with lighting:

Top is a quad with a base_color and double_sided: true, bottom is the same quad with flipped: true.
Screen Shot 2022-01-21 at 7 31 05 AM
Screen Shot 2022-01-21 at 7 31 24 AM

@superdump
Copy link
Contributor

superdump commented Jan 21, 2022

Hmm. It looks like setting Quad flip: true is supposed to only change the UVs according to the documentation:

Flips the texture coords of the resulting vertices.

Flips how? Horizontally? Vertically? I don't know what is intended so that probably needs to be improved. :)

But anyway, what the code seems to do is swap the vertex positions horizontally, so the bottom-left and bottom-right get swapped, and the top-left and top-right get swapped. However, the indices remain the same. This results in the winding order being reversed. So flipping is not even just making the texture coordinates flip, it is practically rotating the quad 180 degrees about its y-axis.

Did you mean the bottom quad is with the same base colour, double-sided set to true, and flip set to true on the quad?

As for what the pbr.wgsl shader is doing for double-sided:

        if ((material.flags & STANDARD_MATERIAL_FLAGS_DOUBLE_SIDED_BIT) != 0u) {
            if (!in.is_front) {
                N = -N;
#ifdef VERTEX_TANGENTS
#ifdef STANDARDMATERIAL_NORMAL_MAP
                T = -T;
                B = -B;
#endif
#endif
            }
        }

So if the material indicates that the mesh is double-sided, with your PR back-face culling is disabled (and front-face culling for that matter) and the winding is used to identify in the fragment shader whether the fragment is for the front or back face of the triangle. Our winding convention is counter-clockwise, due to using a right-handed projection I believe, though I may be wrong. So when you set flip: true on the quad, and it practically reverses the winding of the triangle, it results in the fragments looking at the back face of the triangle. That the means is_front is false, and the normal which points out of the front face is inverted by multiplying by -1 to instead point out of the back face. I think that all makes sense.

All of that said, I see that the shading is different. The question is why. :)

@superdump
Copy link
Contributor

Ohhh, but even if the winding is changed, the normal isn't changed. So the normal points in the wrong direction. I think it looks like the intention is that flip: true should only flip the UVs horizontally, not also flip the vertex positions horizontally. I tried to look back through the git history and this flip property goes all the way back to June 2020 in a commit by @cart :

commit 75429f46398cee2ba624980e15e1efb5a049dfe0
Author: Carter Anderson <mcanders1@gmail.com>
Date:   Wed Jun 24 15:29:10 2020 -0700

    render: use left-handed coordinate system and y-up

Different times. I think the code should not be this:

        let north_west = vec2(-extent_x, extent_y);
        let north_east = vec2(extent_x, extent_y);
        let south_west = vec2(-extent_x, -extent_y);
        let south_east = vec2(extent_x, -extent_y);
        let vertices = if quad.flip {
            [
                (
                    [south_east.x, south_east.y, 0.0],
                    [0.0, 0.0, 1.0],
                    [1.0, 1.0],
                ),
                (
                    [north_east.x, north_east.y, 0.0],
                    [0.0, 0.0, 1.0],
                    [1.0, 0.0],
                ),
                (
                    [north_west.x, north_west.y, 0.0],
                    [0.0, 0.0, 1.0],
                    [0.0, 0.0],
                ),
                (
                    [south_west.x, south_west.y, 0.0],
                    [0.0, 0.0, 1.0],
                    [0.0, 1.0],
                ),
            ]
        } else {
            [
                (
                    [south_west.x, south_west.y, 0.0],
                    [0.0, 0.0, 1.0],
                    [0.0, 1.0],
                ),
                (
                    [north_west.x, north_west.y, 0.0],
                    [0.0, 0.0, 1.0],
                    [0.0, 0.0],
                ),
                (
                    [north_east.x, north_east.y, 0.0],
                    [0.0, 0.0, 1.0],
                    [1.0, 0.0],
                ),
                (
                    [south_east.x, south_east.y, 0.0],
                    [0.0, 0.0, 1.0],
                    [1.0, 1.0],
                ),
            ]
        };

but should instead be this:

        let north_west = vec2(-extent_x, extent_y);
        let north_east = vec2(extent_x, extent_y);
        let south_west = vec2(-extent_x, -extent_y);
        let south_east = vec2(extent_x, -extent_y);
        let vertices = if quad.flip {
            [
                (
                    [south_west.x, south_west.y, 0.0],
                    [0.0, 0.0, 1.0],
                    [1.0, 1.0],
                ),
                (
                    [north_west.x, north_west.y, 0.0],
                    [0.0, 0.0, 1.0],
                    [1.0, 0.0],
                ),
                (
                    [north_east.x, north_east.y, 0.0],
                    [0.0, 0.0, 1.0],
                    [0.0, 0.0],
                ),
                (
                    [south_east.x, south_east.y, 0.0],
                    [0.0, 0.0, 1.0],
                    [0.0, 1.0],
                ),
             ]
        } else {
            [
                (
                    [south_west.x, south_west.y, 0.0],
                    [0.0, 0.0, 1.0],
                    [0.0, 1.0],
                ),
                (
                    [north_west.x, north_west.y, 0.0],
                    [0.0, 0.0, 1.0],
                    [0.0, 0.0],
                ),
                (
                    [north_east.x, north_east.y, 0.0],
                    [0.0, 0.0, 1.0],
                    [1.0, 0.0],
                ),
                (
                    [south_east.x, south_east.y, 0.0],
                    [0.0, 0.0, 1.0],
                    [1.0, 1.0],
                ),
            ]
        };

That is, quad.flip only flips the UV coordinates in x. As the vertex positions and normals are identical, it doesn't affect the lighting at all.

@rparrett
Copy link
Contributor Author

rparrett commented Jan 21, 2022

To sidestep the quad flip issue that I don't fully understand, here's a quad rotated 180 degrees on the y (or x) axis.

image

edit: but a quad with [0.0, 0.0, -1.0] normals appears like the "grey quad" screenshot above?

@rparrett
Copy link
Contributor Author

Closing in favor of #3982

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StandardMaterial's double_sided field does not prevent backface culling
4 participants