Skip to content

Remove the "bevy_ui_debug" feature gate #21091

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
Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Remove the "bevy_ui_debug" feature gate #21091

wants to merge 12 commits into from
+50 −63

Conversation

ickshonpe
Copy link
Contributor

Objective

The "bevy_ui_debug" feature gate doesn't achieve much, the UI debug overlay implementation consists of just a resource and an extraction system that early returns if the overlay is disabled. But it does make the debug overlay harder to discover and use, and it's often broken by changes to the UI rendering systems that forget to update it.

Solution

Remove the "bevy_ui_debug" feature gate.

@ickshonpe
Removed the "bevy_ui_debug" feature gate.
1a24032 
There isn't doesn't seem to be much point to this feature gate, the implementation is just a resource and a single extraction function that early returns if the debug overlay is disabled.
@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets 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 Sep 16, 2025
@github-actions GitHub Actions
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

alice-i-cecile
alice-i-cecile requested changes Sep 16, 2025
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.

I think I agree with this, but we should leave a quick migration guide for the removed feature. Just explain that it's always enabled.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Sep 16, 2025
@ickshonpe
Copy link
Contributor Author

I think I agree with this, but we should leave a quick migration guide for the removed feature. Just explain that it's always enabled.

I left the resource optional, not sure if it should be added automatically or not.

alice-i-cecile
alice-i-cecile reviewed Sep 16, 2025
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.

Code LGTM now, but I'm currently on the fence over whether or not this is the design that we should be using for these debug features in general.

On the one hand, features suck, and they're annoying to test and make it harder for users to discover these dev tools.
On the other hand, this really is a dev tool, and we should have a way to ensure that users aren't paying for them in their production apps.

@mockersf, I'm sure you have opinions.

@alice-i-cecile alice-i-cecile added S-Needs-Design This issue requires design work to think about how it would best be accomplished and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 16, 2025
@ickshonpe ickshonpe added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 16, 2025
@alice-i-cecile alice-i-cecile self-requested a review September 16, 2025 16:32
@ickshonpe
Copy link
Contributor Author

ickshonpe commented Sep 16, 2025 •
edited
Loading

On the one hand, features suck, and they're annoying to test and make it harder for users to discover these dev tools. On the other hand, this really is a dev tool, and we should have a way to ensure that users aren't paying for them in their production apps.

The other alternative, to ensure that this doesn't get broken so often, is that we add a module testing debug outlines to the testbed_ui example and enable the "bevy_ui_debug" feature for the screenshot CI.

@ickshonpe
Copy link
Contributor Author

Actually, I guess it should be done either way.

mgi388
mgi388 reviewed Sep 16, 2025
View reviewed changes
Copy link
Contributor

@mgi388 mgi388 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

If we remove this feature gate doesn’t this mean I can’t compile out this dev/debug feature in my release builds?

Edit: For my own part I'd prefer more debug gates than less gates, so I'd want to keep the gate in rather than removing it per this PR. I understand gates come with complexity but I think Bevy being lean and modular is the better direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile Awaiting requested review from alice-i-cecile

1 more reviewer

@mgi388 mgi388 mgi388 left review comments

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ickshonpe @alice-i-cecile @mgi388