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.

Sign up for GitHub

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

Jump to bottom

Add per-entity colored wireframes #3677

Closed
wants to merge 2 commits into from
Closed

Add per-entity colored wireframes #3677

wants to merge 2 commits into from

Conversation

UberLambda
Copy link

Objective

This branch adds per-entity-colored wireframes to WireframePlugin.
This feature is mostly useful for debugging; for instance, think of differently-colored selection wireframes in an editor.

image

Solution

  • The Wireframe component now has a color: Color field, which defaults to white.
    If present, this sets the color of the wirefame for that specific entity only.
  • For the "global" wireframe setting, the default color can now be set in the WireframeConfig.
    Wireframe colors have precedence over the WireframeConfig one.
  • The colors are stored in a DynamicUniformVec on GPU. They are extracted from Wireframe components (and the WireframeConfig) by a system in the RenderApp.
  • Finally, the wireframe sample has been modified to document these new capabilities.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 14, 2022
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Jan 14, 2022
alice-i-cecile
alice-i-cecile requested changes Jan 15, 2022
View reviewed changes
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.

Just a couple of nits. The examples are good, the code is clear and the functionality is lovely. Thanks!

crates/bevy_pbr/src/wireframe.rs Outdated Show resolved Hide resolved
alice-i-cecile
alice-i-cecile approved these changes Jan 15, 2022
View reviewed changes
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.

Looks great. We can quibble over the pub-ness, but I'm otherwise very happy with this PR.

@IceSentry
Copy link
Contributor

@UberLambda Could you rebase this PR, I was about to make a PR with exactly this feature. The code looks good to me, but there are conflicts.

@UberLambda UberLambda force-pushed the colored_wireframe branch 2 times, most recently from de1f156 to e096e8e Compare May 2, 2022 11:26
@UberLambda
Copy link
Author

UberLambda commented May 2, 2022 •
edited
Loading

@IceSentry Rebased, it seems to work + it passes tests (locally) 👀

alice-i-cecile
alice-i-cecile requested changes May 2, 2022
View reviewed changes
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.

Left some feedback about doc comments that I'd like to see cleaned up before we merge :)

@alice-i-cecile
Copy link
Member

I like the new Option<Color> change a lot. Nice work!

IceSentry
IceSentry reviewed May 2, 2022
View reviewed changes
examples/3d/wireframe.rs Outdated Show resolved Hide resolved
@IceSentry
Copy link
Contributor

About the overloaded use of global. I somewhat agree with that, but debug_wireframes doesn't feel like a better alternative. Wireframes are already a debugging tool and this name doesn't really imply that it's going to show wireframes on everything.

Maybe something like all_meshes or enable_all could be clearer about what it does.

@UberLambda
Copy link
Author

About the overloaded use of global. I somewhat agree with that, but debug_wireframes doesn't feel like a better alternative. Wireframes are already a debugging tool and this name doesn't really imply that it's going to show wireframes on everything.

Maybe something like all_meshes or enable_all could be clearer about what it does.

Maybe on_all_meshes?

@IceSentry
Copy link
Contributor

on_all_meshes is fine too

@alice-i-cecile 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 May 2, 2022
superdump
superdump requested changes May 5, 2022
View reviewed changes
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.

Resolve the conflicts and switch to ExtractComponentPlugin for the Wireframe component extraction and we can get this merged.

crates/bevy_pbr/src/wireframe.rs Outdated Show resolved Hide resolved
@UberLambda
Copy link
Author

Done, I think? (I did not have much time for this in the past week)

@inodentry
Copy link
Contributor

I suggest making wireframe color a separate component from the toggle to enable/disable it. This would allow entities to specify their preferred wireframe color in general, even when wireframe is off (useful for when controlling the wireframe globally via the resource).

@alice-i-cecile 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 May 16, 2022
@alice-i-cecile
Copy link
Member

I'd like to block on @inodentry's comment above. @UberLambda can you make that change when you have time? I did a full review pass on stream today and was very keen on this functionality and the quality of the work.

@UberLambda
Add colored wireframes
4443213 
Separate Wireframe from WireframeColor
@UberLambda
Copy link
Author

Done! (I like @inodentry's solution more than having an Optional in there, it was a bit awkward)

@IceSentry
Copy link
Contributor

This is much better now, but next time, please make a separate commit. Rebasing is completely fine, but I would have liked seeing the diff between the previous version. Other than that, good job 👍

@UberLambda
Copy link
Author

This is much better now, but next time, please make a separate commit. Rebasing is completely fine, but I would have liked seeing the diff between the previous version. Other than that, good job +1

Yeah, sorry - I had to get accustomed to the squash/rebase/squash dance lately 😅

@jack1232 jack1232 mentioned this pull request May 21, 2022
Open
@IceSentry
Copy link
Contributor

IceSentry commented Jul 10, 2022 •
edited
Loading

@UberLambda could you rebase and fix the conflicts? Hopefully we can get this merged before 0.8

@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Jul 10, 2022
@alice-i-cecile
Copy link
Member

I've spoken with the author; this needs to be adopted.

@IceSentry IceSentry mentioned this pull request Jul 13, 2022
Merged
@coreh
Copy link
Contributor

coreh commented Nov 17, 2022 •
edited
Loading

I can adopt this, if @UberLambda is still without bandwidth/availability to get this to the latest main.

How is this normally done? Via a new PR (since we can't push to their repo?)

EDIT: Duh, there's already another PR #5303. I can review it

@alice-i-cecile
Copy link
Member

Closing in favor of #5303.

github-merge-queue bot pushed a commit that referenced this pull request Oct 13, 2023
@IceSentry
Configurable colors for wireframe (#5303)
068e42a 
# 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.~~
regnarock pushed a commit to regnarock/bevy that referenced this pull request Oct 13, 2023
@IceSentry
Configurable colors for wireframe (bevyengine#5303)
6973886 
# 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.~~
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
@IceSentry @ameknite
Configurable colors for wireframe (bevyengine#5303)
fe39bbe 
# 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.~~
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
@IceSentry
Configurable colors for wireframe (bevyengine#5303)
793dd4d 
# 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.~~
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IceSentry IceSentry IceSentry approved these changes

@MinerSebas MinerSebas MinerSebas left review comments

@superdump superdump superdump requested changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@UberLambda @IceSentry @alice-i-cecile @inodentry @coreh @superdump @MinerSebas