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

bevy_pbr2: added vertex color support #3187

Closed
wants to merge 4 commits into from
Closed

Conversation

folke
Copy link
Contributor

@folke folke commented Nov 25, 2021

Objective

There's already an attribute (Mesh::ATTRIBUTE_COLOR) to set custom vertex colors, but they are not used in the pbr2 pipeline.

Solution

This PR adds support for vertex colors (Mesh::ATTRIBUTE_COLOR) in the pbr2 pipeline by initially setting the output_color to the custom vertex color, instead of the base color of the material.

I've also added an example based on the custom_attributre example, that renders a cube with custom vertex colors using the pbr2 pipeline.

image

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Nov 25, 2021
@folke
Copy link
Contributor Author

folke commented Nov 25, 2021

With this PR, colors could also be enabled in the gltf2 loader

// if let Some(vertex_attribute) = reader
// .read_colors(0)
// .map(|v| VertexAttributeValues::Float32x4(v.into_rgba_f32().collect()))
// {
// mesh.set_attribute(Mesh::ATTRIBUTE_COLOR, vertex_attribute);
// }

@folke folke changed the title added Vertex_Color support to bevy_pbr2 bevy_pbr2: added vertex color support Nov 25, 2021
@parasyte
Copy link
Contributor

See also #3120

@folke
Copy link
Contributor Author

folke commented Nov 26, 2021

See also #3120

I'm very new to Bevy and hadn't seen your PR yet. Would make it definitely cleaner to add in the color support!

Once that PR is merged, I'll update the code here.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Enhancement A new feature S-Needs-Review and removed S-Needs-Triage This issue needs to be labelled labels Nov 26, 2021
};

[[stage(fragment)]]
fn fragment(in: FragmentInput) -> [[location(0)]] vec4<f32> {
var output_color: vec4<f32> = material.base_color;

#ifdef VERTEX_COLORS
output_color = in.world_color;
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't it make more sense for the vertex color to be multiplied with the base color from the material, rather than overriding it?

It seems a lot more useful to me (though I admit I don't know what's the typical/standard behavior for this case / what other game engines do).

That way, the shader combines the color from the texture (if any), the color from the material (if any), and the color from the vertex (if any). This gives the most flexibility, as the user could use all of these variables to create cool graphics effects. It seems strictly more useful than what you are doing right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep totally makes sense. I've actually moved on to another method and personally no longer use the color vertices. I set the material base color to white and then create a custom texture with the colors I need. Instead of using vertex colors I then simply use correct uv coords for the texture. Once that other pr is merged, I'll update this PR and change it to multiply instead. Great suggestion

Copy link
Contributor

Choose a reason for hiding this comment

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

Which other PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think he’s referring to #3120

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought so but didn't see any direct reference. :) I agree it makes sense to get the flexible vertex attributes merged first and then this. I have added both of these to the rendering project board so they get prioritised as these are common problems that people hit.

@superdump
Copy link
Contributor

Now that #3959 is merged, I think this can be reworked on top of that. It should hopefully be simpler to do too! :)

@superdump
Copy link
Contributor

Closing as vertex colours are now supported via a different PR.

@superdump superdump closed this May 7, 2022
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-Enhancement A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants