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

Add option to disable gizmo rendering for specific cameras #8952

Merged
merged 2 commits into from
Jun 29, 2023

Conversation

tim-blackbird
Copy link
Contributor

@tim-blackbird tim-blackbird commented Jun 25, 2023

Added GizmoConfig::render_layers, which will ensure Gizmos are only rendered on cameras that can see those RenderLayers.

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

Bevy usually has additive components (ie: component that enables gizmo) rather than subtractive components.

But here is the catch:

  • I'm not sure how a RenderGizmos component would work. Surely, it would be added to the CameraBundle, but then how do you remove the RenderGizmos component?
  • gizmos exist for debugging, so API ergonomics trump all.

@mockersf
Copy link
Member

couldn't render layers be used?

@Selene-Amanita Selene-Amanita added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen A-Gizmos Visual editor and debug gizmos labels Jun 25, 2023
@tim-blackbird
Copy link
Contributor Author

couldn't render layers be used?

This was originally my plan but render layers are not extracted to the render world so using that would require additional bookkeeping.

@mockersf
Copy link
Member

render layers are not extracted to the render world so using that would require additional bookkeeping.

Would this similar to what is done in #8332 for UI?

@tim-blackbird
Copy link
Contributor Author

Would this similar to what is done in #8332 for UI?

Yeah, would you prefer I grab the RenderLayers extraction code from that PR and go with that?

@JMS55 JMS55 added this to the 0.11 milestone Jun 26, 2023
@mockersf
Copy link
Member

mockersf commented Jun 26, 2023

Yeah, would you prefer I grab the RenderLayers extraction code from that PR and go with that?

Yes. @james7132 seemed to not be a fan to add those to the render app, but if ECS/renderer experts are OK with that it would have my preference

@alice-i-cecile
Copy link
Member

My preference is also for reusing the RenderLayers mechanism here. No strong opinions on exactly how that should be done though.

@tim-blackbird
Copy link
Contributor Author

@james7132 probably didn't like paying the cost of extracting RenderLayers for every ui node, but we only need them per camera/view here.

Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

I would prefer to be able to set the render layer per gizmo to decide which goes to which camera, but that seems already useful as is

@tim-blackbird
Copy link
Contributor Author

We definitely want more granular settings. I'll open some issues for future improvements to bevy_gizmos soon.

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

@devil-ira can you update the PR description to reflect the new changes?

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I updated the description and reverted the split_screen "renderlayer gizmo" changes. split_screen exists to show a minimal split screen setup. Adding gizmo config to it adds unnecessary noise. We should create a new example if we want that.

@cart cart enabled auto-merge June 29, 2023 00:42
@cart cart added this pull request to the merge queue Jun 29, 2023
Merged via the queue into bevyengine:main with commit e29981d Jun 29, 2023
20 checks passed
@tim-blackbird
Copy link
Contributor Author

Oops, didn't mean to commit the split_screen changes.

@tim-blackbird tim-blackbird deleted the disable-gizmos branch July 10, 2023 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Gizmos Visual editor and debug gizmos A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible 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.

7 participants