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

Alternate wireframe override api #10023

Merged

Conversation

IceSentry
Copy link
Contributor

@IceSentry IceSentry commented Oct 4, 2023

Objective

#7328 introduced an API to override the global wireframe config. I believe it is flawed for a few reasons.

This PR uses a non-breaking API. Instead of making the Wireframe an enum I introduced the NeverRenderWireframe component. Here's the reason why I think this is better:

  • Easier to migrate since it doesn't change the old behaviour. Essentially nothing to migrate. Right now this PR is a breaking change but I don't think it has to be.
  • It's similar to other "per mesh" rendering features like NotShadowCaster/NotShadowReceiver
  • It doesn't force new users to also think about global vs not global if all they want is to render a wireframe
  • This would also let you filter at the query definition level instead of filtering when running the query

Solution

  • Introduce a NeverRenderWireframe component that ignores the global config

Changelog

  • Added a NeverRenderWireframe component that ignores the global WireframeConfig

@IceSentry IceSentry added the A-Rendering Drawing game state to the screen label Oct 4, 2023
@alice-i-cecile
Copy link
Member

@Wcubed, opinions here?

@alice-i-cecile alice-i-cecile added C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Oct 4, 2023
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.

This is clean, and should be meaningfully faster. This is a better way to tackle this I think.

@alice-i-cecile alice-i-cecile added this to the 0.12 milestone Oct 4, 2023
Copy link
Contributor

@Shatur Shatur left a comment

Choose a reason for hiding this comment

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

Like the consistency with other engine API.

@rparrett
Copy link
Contributor

rparrett commented Oct 4, 2023

It's similar to other "per mesh" rendering features like NotShadowCaster/NotShadowReceiver

Also NoFrustumCulling and now NoAutomaticBatching.

Bikeshedding: How do people feel about NoWireframe?

@IceSentry
Copy link
Contributor Author

I originally started with NoWireframe but switched while making the PR. I don't even remember why 😅 because I prefer NoWireframe

@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 Oct 5, 2023
@alice-i-cecile
Copy link
Member

@IceSentry swap to NoWireframe and I'll merge this for you :)

@Wcubed
Copy link
Contributor

Wcubed commented Oct 5, 2023

I agree on all of the benefits listed. This indeed looks like the better way to handle this :)

This does make me think though: If we have multiple "mutually exclusive" components, or basically enums disguised as components for significant benefits, do we want to add any type of guarding or checking functionality to this? Because the one thing we lose is rusts guarantee that an enum is one and only one value. In this case there are two components that are related, but in a game there could be many more cases, and then keeping them disjoint could get error-prone.
But that's a discussion for another place I think.
Edit: Looks like it might be related to #6556

@alice-i-cecile
Copy link
Member

This does make me think though: If we have multiple "mutually exclusive" components, or basically enums disguised as components for significant benefits, do we want to add any type of guarding or checking functionality to this?

Described in #1481 :) It's a good idea!

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 5, 2023
Merged via the queue into bevyengine:main with commit a962240 Oct 5, 2023
21 checks passed
@cart cart mentioned this pull request Oct 13, 2023
43 tasks
regnarock pushed a commit to regnarock/bevy that referenced this pull request Oct 13, 2023
# Objective

bevyengine#7328 introduced an API to
override the global wireframe config. I believe it is flawed for a few
reasons.

This PR uses a non-breaking API. Instead of making the `Wireframe` an
enum I introduced the `NeverRenderWireframe` component. Here's the
reason why I think this is better:
- Easier to migrate since it doesn't change the old behaviour.
Essentially nothing to migrate. Right now this PR is a breaking change
but I don't think it has to be.
- It's similar to other "per mesh" rendering features like
NotShadowCaster/NotShadowReceiver
- It doesn't force new users to also think about global vs not global if
all they want is to render a wireframe
- This would also let you filter at the query definition level instead
of filtering when running the query

## Solution

- Introduce a `NeverRenderWireframe` component that ignores the global
config

---

## Changelog

- Added a `NeverRenderWireframe` component that ignores the global
`WireframeConfig`
@IceSentry IceSentry deleted the non-breaking-wireframe-api branch October 18, 2023 19:37
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
# Objective

bevyengine#7328 introduced an API to
override the global wireframe config. I believe it is flawed for a few
reasons.

This PR uses a non-breaking API. Instead of making the `Wireframe` an
enum I introduced the `NeverRenderWireframe` component. Here's the
reason why I think this is better:
- Easier to migrate since it doesn't change the old behaviour.
Essentially nothing to migrate. Right now this PR is a breaking change
but I don't think it has to be.
- It's similar to other "per mesh" rendering features like
NotShadowCaster/NotShadowReceiver
- It doesn't force new users to also think about global vs not global if
all they want is to render a wireframe
- This would also let you filter at the query definition level instead
of filtering when running the query

## Solution

- Introduce a `NeverRenderWireframe` component that ignores the global
config

---

## Changelog

- Added a `NeverRenderWireframe` component that ignores the global
`WireframeConfig`
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

bevyengine#7328 introduced an API to
override the global wireframe config. I believe it is flawed for a few
reasons.

This PR uses a non-breaking API. Instead of making the `Wireframe` an
enum I introduced the `NeverRenderWireframe` component. Here's the
reason why I think this is better:
- Easier to migrate since it doesn't change the old behaviour.
Essentially nothing to migrate. Right now this PR is a breaking change
but I don't think it has to be.
- It's similar to other "per mesh" rendering features like
NotShadowCaster/NotShadowReceiver
- It doesn't force new users to also think about global vs not global if
all they want is to render a wireframe
- This would also let you filter at the query definition level instead
of filtering when running the query

## Solution

- Introduce a `NeverRenderWireframe` component that ignores the global
config

---

## Changelog

- Added a `NeverRenderWireframe` component that ignores the global
`WireframeConfig`
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-Performance A change motivated by improving speed, memory usage or compile times C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants