use `Material` for wireframes #5314

IceSentry · 2022-07-14T02:18:50Z

Objective

Use the Material abstraction for the Wireframes
- Right now this doesn't have many benefits other than simplifying some of the rendering code
- We can reuse the default vertex shader and avoid rendering inconsistencies
- The goal is to have a material with a color on each mesh so this approach will make it easier to implement
Originally done in Configurable colors for wireframe #5303 but I decided to split the Material part to it's own PR and then adding per-entity colors and globally configurable colors will be a much simpler diff.

Solution

Use the new Material abstraction for the Wireframes

Notes

It's possible this isn't ideal since this adds a Handle<WireframeMaterial> to all the meshes compared to the original approach that didn't need anything. I didn't notice any performance impact on my machine.

This might be a surprising usage of Material at first, because intuitively you only have one material per mesh, but the way it's implemented you can have as many different types of materials as you want on a mesh.

Migration Guide

WireframePipeline was removed. If you were using it directly, please create an issue explaining your use case.

alice-i-cecile · 2022-07-14T02:23:12Z

I'm on board with this direction; unifying this is nice.

IceSentry · 2022-07-14T02:23:15Z

@alice-i-cecile from a user perspective there is no breaking change right now

IceSentry · 2022-07-14T02:24:24Z

It's the same WireframePlugin and Wireframe Component that was used before, it's just the internals that changed, so I don't think it counts as breaking.

crates/bevy_pbr/src/wireframe.rs

github-actions · 2023-08-14T20:44:31Z

Example alien_cake_addict failed to run, please try running it locally and check the result.

DGriffin91 · 2023-08-14T20:58:11Z

It may be worth turning the original version without the material into an example since it was a good reference for rendering something simple without using the material system.

IceSentry · 2023-08-14T21:13:02Z

I think the mesh2d_manual example serves that purpose pretty well. Maybe we should have a 3d version

crates/bevy_pbr/src/wireframe.rs

alice-i-cecile · 2023-09-29T15:25:27Z

How does this relate to #5303?

IceSentry · 2023-09-29T18:28:51Z

How does this relate to #5303?

I think it was just me trying to aggressively reduce the size of the PR and have the customizable color in it's own PR

crates/bevy_pbr/src/wireframe.rs

IceSentry · 2023-10-04T17:36:45Z

I realized we don't need the vertex shader anymore since we can use the vertex shader from the Material api. This also fixed a bug where wireframes are broken in DX12 because of some vertex shader issues.

IceSentry · 2023-10-04T17:38:21Z

There's now a few conflicts because of the wireframe override PR. I personally think the api of that PR is flawed so I I'll submit a PR with an API that isn't a breaking change and should create less conflict with this PR.

I think we should decided if we want to merge this PR first since it will make it easier for me to resolve conflicts #10023

IceSentry · 2023-10-05T19:05:26Z

I fixed the conflicts with the NoWireframe feature. I believe this is ready for a final review.

IceSentry · 2023-10-05T20:44:41Z

I don't know what's up with the CI failure. It works fine locally and it's complaining about completely unrelated parts of the codebase.

crates/bevy_pbr/src/wireframe.rs

Co-authored-by: Alice Cecile <[email protected]>

# Objective - Make the wireframe colors configurable at the global level and the single mesh level - Based on #5314 This video shows what happens when playing with various settings from the example https://github.com/bevyengine/bevy/assets/8348954/1ee9aee0-fab7-4da8-bc5d-8d0562bb34e6 ## Solution - Add a `color` field to the `WireframeMaterial` - Use a `WireframeColor` component to configure the color per entity - Add a `default_color` field to `WireframeConfig` for global wireframes or wireframes with no specified color. ## Notes - Most of the docs and the general idea for `WireframeColor` came from [UberLambda](https://github.com/UberLambda) in #3677 but the code ended up completely different so I created a separate branch. ~~I'm not sure how to correctly credit them on this PR.~~ (I re-created the commit but I added them as co-author in the commit message) ~~Closes #3677 ~~Closes #5301 ~~#5314 should be merged before this PR.~~

# Objective - Use the `Material` abstraction for the Wireframes - Right now this doesn't have many benefits other than simplifying some of the rendering code - We can reuse the default vertex shader and avoid rendering inconsistencies - The goal is to have a material with a color on each mesh so this approach will make it easier to implement - Originally done in bevyengine#5303 but I decided to split the Material part to it's own PR and then adding per-entity colors and globally configurable colors will be a much simpler diff. ## Solution - Use the new `Material` abstraction for the Wireframes ## Notes It's possible this isn't ideal since this adds a `Handle<WireframeMaterial>` to all the meshes compared to the original approach that didn't need anything. I didn't notice any performance impact on my machine. This might be a surprising usage of `Material` at first, because intuitively you only have one material per mesh, but the way it's implemented you can have as many different types of materials as you want on a mesh. ## Migration Guide `WireframePipeline` was removed. If you were using it directly, please create an issue explaining your use case. --------- Co-authored-by: Alice Cecile <[email protected]>

# Objective - Make the wireframe colors configurable at the global level and the single mesh level - Based on bevyengine#5314 This video shows what happens when playing with various settings from the example https://github.com/bevyengine/bevy/assets/8348954/1ee9aee0-fab7-4da8-bc5d-8d0562bb34e6 ## Solution - Add a `color` field to the `WireframeMaterial` - Use a `WireframeColor` component to configure the color per entity - Add a `default_color` field to `WireframeConfig` for global wireframes or wireframes with no specified color. ## Notes - Most of the docs and the general idea for `WireframeColor` came from [UberLambda](https://github.com/UberLambda) in bevyengine#3677 but the code ended up completely different so I created a separate branch. ~~I'm not sure how to correctly credit them on this PR.~~ (I re-created the commit but I added them as co-author in the commit message) ~~Closes bevyengine#3677 ~~Closes bevyengine#5301 ~~bevyengine#5314 should be merged before this PR.~~

# Objective - Use the `Material` abstraction for the Wireframes - Right now this doesn't have many benefits other than simplifying some of the rendering code - We can reuse the default vertex shader and avoid rendering inconsistencies - The goal is to have a material with a color on each mesh so this approach will make it easier to implement - Originally done in bevyengine#5303 but I decided to split the Material part to it's own PR and then adding per-entity colors and globally configurable colors will be a much simpler diff. ## Solution - Use the new `Material` abstraction for the Wireframes ## Notes It's possible this isn't ideal since this adds a `Handle<WireframeMaterial>` to all the meshes compared to the original approach that didn't need anything. I didn't notice any performance impact on my machine. This might be a surprising usage of `Material` at first, because intuitively you only have one material per mesh, but the way it's implemented you can have as many different types of materials as you want on a mesh. ## Migration Guide `WireframePipeline` was removed. If you were using it directly, please create an issue explaining your use case. --------- Co-authored-by: Alice Cecile <[email protected]>

# Objective - Make the wireframe colors configurable at the global level and the single mesh level - Based on bevyengine#5314 This video shows what happens when playing with various settings from the example https://github.com/bevyengine/bevy/assets/8348954/1ee9aee0-fab7-4da8-bc5d-8d0562bb34e6 ## Solution - Add a `color` field to the `WireframeMaterial` - Use a `WireframeColor` component to configure the color per entity - Add a `default_color` field to `WireframeConfig` for global wireframes or wireframes with no specified color. ## Notes - Most of the docs and the general idea for `WireframeColor` came from [UberLambda](https://github.com/UberLambda) in bevyengine#3677 but the code ended up completely different so I created a separate branch. ~~I'm not sure how to correctly credit them on this PR.~~ (I re-created the commit but I added them as co-author in the commit message) ~~Closes bevyengine#3677 ~~Closes bevyengine#5301 ~~bevyengine#5314 should be merged before this PR.~~

# Objective - Use the `Material` abstraction for the Wireframes - Right now this doesn't have many benefits other than simplifying some of the rendering code - We can reuse the default vertex shader and avoid rendering inconsistencies - The goal is to have a material with a color on each mesh so this approach will make it easier to implement - Originally done in bevyengine#5303 but I decided to split the Material part to it's own PR and then adding per-entity colors and globally configurable colors will be a much simpler diff. ## Solution - Use the new `Material` abstraction for the Wireframes ## Notes It's possible this isn't ideal since this adds a `Handle<WireframeMaterial>` to all the meshes compared to the original approach that didn't need anything. I didn't notice any performance impact on my machine. This might be a surprising usage of `Material` at first, because intuitively you only have one material per mesh, but the way it's implemented you can have as many different types of materials as you want on a mesh. ## Migration Guide `WireframePipeline` was removed. If you were using it directly, please create an issue explaining your use case. --------- Co-authored-by: Alice Cecile <[email protected]>

# Objective - Make the wireframe colors configurable at the global level and the single mesh level - Based on bevyengine#5314 This video shows what happens when playing with various settings from the example https://github.com/bevyengine/bevy/assets/8348954/1ee9aee0-fab7-4da8-bc5d-8d0562bb34e6 ## Solution - Add a `color` field to the `WireframeMaterial` - Use a `WireframeColor` component to configure the color per entity - Add a `default_color` field to `WireframeConfig` for global wireframes or wireframes with no specified color. ## Notes - Most of the docs and the general idea for `WireframeColor` came from [UberLambda](https://github.com/UberLambda) in bevyengine#3677 but the code ended up completely different so I created a separate branch. ~~I'm not sure how to correctly credit them on this PR.~~ (I re-created the commit but I added them as co-author in the commit message) ~~Closes bevyengine#3677 ~~Closes bevyengine#5301 ~~bevyengine#5314 should be merged before this PR.~~

IceSentry added the A-Rendering Drawing game state to the screen label Jul 14, 2022

IceSentry mentioned this pull request Jul 14, 2022

Configurable colors for wireframe #5303

Merged

alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jul 14, 2022

IceSentry removed the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 14, 2022

alice-i-cecile reviewed Jul 14, 2022

View reviewed changes

crates/bevy_pbr/src/wireframe.rs Show resolved Hide resolved

IceSentry added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 14, 2022

nicopap mentioned this pull request Nov 14, 2022

Immediate Mode Line/Gizmo Drawing #6529

Merged

3 tasks

IceSentry force-pushed the wireframe-material-no-color branch 2 times, most recently from f3ef677 to 6b12dc9 Compare January 15, 2023 23:26

alice-i-cecile removed the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jan 15, 2023

alice-i-cecile approved these changes Jan 15, 2023

View reviewed changes

alice-i-cecile requested a review from superdump January 15, 2023 23:58

JMS55 added this to the 0.11 milestone Feb 17, 2023

mockersf mentioned this pull request Apr 25, 2023

Colored wireframe #8480

Closed

JMS55 modified the milestones: 0.11, 0.12 Jun 11, 2023

robtfm self-requested a review August 14, 2023 20:05

IceSentry force-pushed the wireframe-material-no-color branch from 6b12dc9 to bb3a879 Compare August 14, 2023 20:33

robtfm reviewed Aug 16, 2023

View reviewed changes

crates/bevy_pbr/src/wireframe.rs Outdated Show resolved Hide resolved

superdump reviewed Oct 1, 2023

View reviewed changes

crates/bevy_pbr/src/wireframe.rs Outdated Show resolved Hide resolved

docs and PR fixes

de097f4

alice-i-cecile requested review from superdump, robtfm and JMS55 October 2, 2023 01:27

yeet vertex shader

4983a41

IceSentry mentioned this pull request Oct 4, 2023

wireframes: workaround for DX12 #10022

Merged

IceSentry added 2 commits October 5, 2023 14:57

Merge branch 'main' into wireframe-material-no-color

ebbfdca

support NoWireframe

ceb163d

use color constants where possible

bd19b54

Merge branch 'main' into wireframe-material-no-color

17464a7

mockersf reviewed Oct 8, 2023

View reviewed changes

crates/bevy_pbr/src/wireframe.rs Outdated Show resolved Hide resolved

Update crates/bevy_pbr/src/wireframe.rs

4bfd58d

Co-authored-by: Alice Cecile <[email protected]>

JMS55 approved these changes Oct 10, 2023

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 10, 2023

alice-i-cecile added this pull request to the merge queue Oct 10, 2023

Merged via the queue into bevyengine:main with commit e05a9f9 Oct 10, 2023
22 checks passed

cart mentioned this pull request Oct 13, 2023

News: Release 0.12 bevyengine/bevy-website#754

Merged

43 tasks

IceSentry deleted the wireframe-material-no-color branch October 18, 2023 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use `Material` for wireframes #5314

use `Material` for wireframes #5314

IceSentry commented Jul 14, 2022 •

edited

Loading

alice-i-cecile commented Jul 14, 2022

IceSentry commented Jul 14, 2022

IceSentry commented Jul 14, 2022

github-actions bot commented Aug 14, 2023

DGriffin91 commented Aug 14, 2023

IceSentry commented Aug 14, 2023 •

edited

Loading

alice-i-cecile commented Sep 29, 2023

IceSentry commented Sep 29, 2023

IceSentry commented Oct 4, 2023

IceSentry commented Oct 4, 2023 •

edited

Loading

IceSentry commented Oct 5, 2023

IceSentry commented Oct 5, 2023

use Material for wireframes #5314

use Material for wireframes #5314

Conversation

IceSentry commented Jul 14, 2022 • edited Loading

Objective

Solution

Notes

Migration Guide

alice-i-cecile commented Jul 14, 2022

IceSentry commented Jul 14, 2022

IceSentry commented Jul 14, 2022

github-actions bot commented Aug 14, 2023

DGriffin91 commented Aug 14, 2023

IceSentry commented Aug 14, 2023 • edited Loading

alice-i-cecile commented Sep 29, 2023

IceSentry commented Sep 29, 2023

IceSentry commented Oct 4, 2023

IceSentry commented Oct 4, 2023 • edited Loading

IceSentry commented Oct 5, 2023

IceSentry commented Oct 5, 2023

use `Material` for wireframes #5314

use `Material` for wireframes #5314

IceSentry commented Jul 14, 2022 •

edited

Loading

IceSentry commented Aug 14, 2023 •

edited

Loading

IceSentry commented Oct 4, 2023 •

edited

Loading