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

[Merged by Bors] - Fix mesh2d_manual example #4037

Closed
wants to merge 5 commits into from

Conversation

rparrett
Copy link
Contributor

@rparrett rparrett commented Feb 25, 2022

Objective

Fixes #4036

Solution

  • Use VertexBufferLayout::from_vertex_formats
  • Actually put a u32 into ATTRIBUTE_COLOR and convert it in the shader

I'm not 100% sure about the color stuff. It seems like ATTRIBUTE_COLOR has been Uint32 this whole time, but this example previously worked with [f32; 4] somehow, perhaps because the vertex layout was manually specified.

Let me know if that can be improved, or feel free to close for an alternative fix.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 25, 2022
@alice-i-cecile alice-i-cecile 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 Feb 25, 2022
// This is the sum of the size of position and color attributes (12 + 16 = 28)
let vertex_array_stride = 28;

let vertex_layout =
Copy link
Member

Choose a reason for hiding this comment

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

Much more robust!

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Much more robust and clear than before, and confirmed to fix the issue. I'm unsure about the color question, but otherwise this LGTM.

@rparrett
Copy link
Contributor Author

I updated the description with slightly more info about the color situation.

@alice-i-cecile alice-i-cecile added the P-Regression Functionality that used to work but no longer does. Add a test for this! label Feb 26, 2022
@alice-i-cecile alice-i-cecile added this to the Bevy 0.7 milestone Feb 26, 2022
@alice-i-cecile
Copy link
Member

@bevyengine/rendering-team, can I get a review from one of y'all? Then I'll be able to merge this in to fix the broken example :)

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.

It feels clunky that ATTRIBUTE_COLOR is a u32 but that's not your fault.

That said, it feels like it would be nicer to have Color::as_rgba_u32(self: Color) -> u32 and Color::as_linear_rgba_u32(self: Color) -> u32 to support use of ATTRIBUTE_COLOR.

@superdump
Copy link
Contributor

bors bot pushed a commit that referenced this pull request Mar 8, 2022
# Objective

- `Mesh::ATTRIBUTE_COLOR` expects colors as `u32`s but there is no function for easy conversion.
- See #4037 (review)

## Solution

- Added `Color::as_rgba_u32` and `Color::as_linear_rgba_u32`
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

We definitely need shader utilities for handling conversion back to vec4, but that's for another time. This works well as an example.

@@ -243,7 +230,7 @@ fn vertex(vertex: Vertex) -> VertexOutput {
var out: VertexOutput;
// Project the world position of the mesh into screen position
out.clip_position = view.view_proj * mesh.model * vec4<f32>(vertex.position, 1.0);
out.color = vertex.color;
out.color = vec4<f32>((vec4<u32>(vertex.color) >> vec4<u32>(0u, 8u, 16u, 24u)) & vec4<u32>(255u)) / 255.0;
Copy link
Member

Choose a reason for hiding this comment

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

a comment explaining this line could be nice

Copy link
Contributor Author

@rparrett rparrett Mar 8, 2022

Choose a reason for hiding this comment

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

I'm not sure that I can adequately explain the process, but I added a comment that hopefully communicates the intent of the code.

Copy link
Member

Choose a reason for hiding this comment

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

good enough for me, thanks!

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Mar 8, 2022
# Objective

Fixes #4036 

## Solution

- Use `VertexBufferLayout::from_vertex_formats`
- Actually put a u32 into `ATTRIBUTE_COLOR` and convert it in the shader

I'm not 100% sure about the color stuff. It seems like `ATTRIBUTE_COLOR` has been `Uint32` this whole time, but this example previously worked with `[f32; 4]` somehow, perhaps because the vertex layout was manually specified.

Let me know if that can be improved, or feel free to close for an alternative fix.
@bors bors bot changed the title Fix mesh2d_manual example [Merged by Bors] - Fix mesh2d_manual example Mar 8, 2022
@bors bors bot closed this Mar 8, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
# Objective

- `Mesh::ATTRIBUTE_COLOR` expects colors as `u32`s but there is no function for easy conversion.
- See bevyengine#4037 (review)

## Solution

- Added `Color::as_rgba_u32` and `Color::as_linear_rgba_u32`
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
# Objective

Fixes bevyengine#4036 

## Solution

- Use `VertexBufferLayout::from_vertex_formats`
- Actually put a u32 into `ATTRIBUTE_COLOR` and convert it in the shader

I'm not 100% sure about the color stuff. It seems like `ATTRIBUTE_COLOR` has been `Uint32` this whole time, but this example previously worked with `[f32; 4]` somehow, perhaps because the vertex layout was manually specified.

Let me know if that can be improved, or feel free to close for an alternative fix.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- `Mesh::ATTRIBUTE_COLOR` expects colors as `u32`s but there is no function for easy conversion.
- See bevyengine#4037 (review)

## Solution

- Added `Color::as_rgba_u32` and `Color::as_linear_rgba_u32`
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Fixes bevyengine#4036 

## Solution

- Use `VertexBufferLayout::from_vertex_formats`
- Actually put a u32 into `ATTRIBUTE_COLOR` and convert it in the shader

I'm not 100% sure about the color stuff. It seems like `ATTRIBUTE_COLOR` has been `Uint32` this whole time, but this example previously worked with `[f32; 4]` somehow, perhaps because the vertex layout was manually specified.

Let me know if that can be improved, or feel free to close for an alternative fix.
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 P-Regression Functionality that used to work but no longer does. Add a test for this!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

example mesh2d_manual crash
5 participants