Configurable colors for wireframe by IceSentry · Pull Request #5303 · bevyengine/bevy

IceSentry · 2022-07-13T04:49:45Z

Objective

Make the wireframe colors configurable at the global level and the single mesh level
Based on use Material for wireframes #5314

This video shows what happens when playing with various settings from the example

wireframe_5FP7z78Faz.mp4

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 in Add per-entity colored wireframes #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.~~

superdump · 2022-07-13T04:59:55Z

Could you restructure the code to follow the order of the original so that the diff is cleaner? It’s a bit all over the place removing and inserting lines that are from practically the same code in different places in the file which makes it difficult to see what actually changed and so, difficult to review.

IceSentry · 2022-07-13T05:05:04Z

Done. It's still a bit noisy because the approach is so different, but it's much better now and that also made me realize I removed the Reflect stuff by mistake.

IceSentry · 2022-07-13T14:27:29Z

@superdump should I make a PR with only the update to the material system and make another PR that adds per-entity colors after that? It would probably make it easier to review, but it felt like the PR was small enough to review all in one go. It would help make the diffs less noisy though.

crates/bevy_pbr/src/wireframe.rs

alice-i-cecile

Minor feedback, mostly on docs, but looking good!

examples/3d/wireframe.rs

JMS55

I'm a bit confused on the different components. Can you explain the structure of how things work? Why is WireframeColor a separate component, instead of an optional field of Wireframe?

crates/bevy_pbr/src/wireframe.rs

IceSentry · 2023-01-15T21:51:55Z

I somewhat forgot about this PR, I'll try to update it.

For the WireframeColor component, I don't remember the detaisl, but I believe this was discussed in this PR #3677

Also, I originally wanted us to merge, #5314 first and then add color support just to make the PRs easier to review and merge. Looking back on the changes now, I think they are simple enough to just be done in one PR.

IceSentry · 2023-01-15T22:05:33Z

Right, so the WireframeColor component makes it way easier to use change detection to detect when/if the color changed

mogambro

I don't know about the finished conversation https://github.com/bevyengine/bevy/pull/5303/files?diff=split&w=0#r1025198770

but apart from that, I reviewed it and I approve the changes.

I am not sure about potential performance pitfalls or such, but the code changes look clean and understandable.

IceSentry · 2023-10-10T15:38:19Z

@nerdachse the main blocker is waiting on #5314 the issue is that most rendering devs are currently working on other bigger reviews but once those are done both wireframe PRs should be merged fairly quickly.

# 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 #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 <alice.i.cecile@gmail.com>

alice-i-cecile · 2023-10-10T19:27:25Z

@IceSentry the prerequisite PR has been merged now; can you clean up merge conflicts when you get a chance?

alice-i-cecile · 2023-10-10T23:03:26Z

Thanks :) FYI, I'm going to wait to merge this until #9258 is through: Rob's asked me to hold off on merging rendering PRs until that's done to avoid generating complex conflicts for the author.

IceSentry · 2023-10-11T05:18:39Z

I don't think there would be any conflict with this PR, but I'm in no hurry to merge this PR either.

mogambro · 2023-10-11T05:35:07Z

If deferred rendering for some reason doesn't make it into 0.12 it would still be awesome to have this.

I am highly anticipating this feature and would hate to wait another 3 months.

Also, while I understand that there are PRs that warrant holding off other work, I argue that they should be sparse and not hold too much other valuable work "hostage" so to speak.

Just my 0.05 of course. I am very grateful for both, deferred rendering and this, actually most that's happening in bevy! So please don't take this the wrong way!

alice-i-cecile · 2023-10-11T10:53:06Z

Yep, this will be in 0.12 one way or another :) Definitely agree that blocking work to get specific PRs through should be very rare: I think the only other time we've done that this cycle is for Assets v2.

crates/bevy_pbr/src/wireframe.rs

# 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 <alice.i.cecile@gmail.com>

# 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 <alice.i.cecile@gmail.com>

# 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 <alice.i.cecile@gmail.com>

# 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 A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible labels Jul 13, 2022

IceSentry force-pushed the wireframe-material branch from aa8b88c to f93e37e Compare July 13, 2022 04:55

IceSentry mentioned this pull request Jul 14, 2022

use Material for wireframes #5314

Merged

nicopap mentioned this pull request Nov 14, 2022

Immediate Mode Line/Gizmo Drawing #6529

Merged

3 tasks

coreh mentioned this pull request Nov 17, 2022

Add per-entity colored wireframes #3677

Closed

alice-i-cecile reviewed Nov 17, 2022

View reviewed changes

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

alice-i-cecile reviewed Nov 17, 2022

View reviewed changes

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

alice-i-cecile reviewed Nov 17, 2022

View reviewed changes

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

alice-i-cecile requested changes Nov 17, 2022

View reviewed changes

alice-i-cecile added this to the 0.10 milestone Nov 17, 2022

JMS55 reviewed Jan 14, 2023

View reviewed changes

examples/3d/wireframe.rs Outdated Show resolved Hide resolved

JMS55 reviewed Jan 14, 2023

View reviewed changes

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

IceSentry force-pushed the wireframe-material branch from 323b3b3 to 84c5d44 Compare January 15, 2023 23:14

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

alice-i-cecile removed this from the 0.10 milestone Feb 13, 2023

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

alice-i-cecile approved these changes Sep 29, 2023

View reviewed changes

alice-i-cecile removed this from the 0.12 milestone Sep 29, 2023

IceSentry force-pushed the wireframe-material branch 2 times, most recently from c407f7b to df3d127 Compare October 5, 2023 19:58

IceSentry changed the title ~~use Material for wireframes and configurable colors~~ Configurable colors for wireframe Oct 5, 2023

mogambro 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 to the 0.12 milestone Oct 10, 2023

alice-i-cecile removed 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 requested a review from JMS55 October 10, 2023 18:53

colored wireframe

4e076ac

IceSentry force-pushed the wireframe-material branch from 18e4e3d to 4e076ac Compare October 10, 2023 21:36

pub material

2ce0daa

IceSentry 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

JMS55 suggested changes Oct 12, 2023

View reviewed changes

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

use run condition

f50beaa

JMS55 approved these changes Oct 12, 2023

View reviewed changes

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

Merged via the queue into bevyengine:main with commit 068e42a Oct 13, 2023

cart mentioned this pull request Oct 13, 2023

News: Release 0.12 bevyengine/bevy-website#754

Merged

43 tasks

IceSentry deleted the wireframe-material branch October 18, 2023 19:38

Uh oh!

Conversation

IceSentry commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Notes

Uh oh!

superdump commented Jul 13, 2022

Uh oh!

IceSentry commented Jul 13, 2022

Uh oh!

IceSentry commented Jul 13, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JMS55 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IceSentry commented Jan 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IceSentry commented Jan 15, 2023

Uh oh!

mogambro left a comment

Choose a reason for hiding this comment

Uh oh!

IceSentry commented Oct 10, 2023

Uh oh!

alice-i-cecile commented Oct 10, 2023

Uh oh!

alice-i-cecile commented Oct 10, 2023

Uh oh!

IceSentry commented Oct 11, 2023

Uh oh!

mogambro commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Oct 11, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

IceSentry commented Jul 13, 2022 •

edited

Loading

IceSentry commented Jan 15, 2023 •

edited

Loading

mogambro commented Oct 11, 2023 •

edited

Loading