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 profiler autostart indicator to EditorRunBar #97492

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

Geometror
Copy link
Member

@Geometror Geometror commented Sep 26, 2024

Followup to #96759

Addresses concerns that making the autostart of profilers persistent between sessions could lead to unexpected performance issues if users forget it's enabled.

Adds a small indicator near the EditorRunBar which shows up when autostart is enabled for any profiler.
Hovering shows the tooltip below.
Clicking opens the debugger view of the bottom panel and selects the first profiler for which autostart is enabled.

{08AD3E20-A18B-4A22-A8BB-324E6FC4EBC7}

@Geometror Geometror added this to the 4.4 milestone Sep 26, 2024
@Geometror Geometror requested review from a team as code owners September 26, 2024 14:17
@Geometror Geometror force-pushed the profiler-autostart-indicator branch 2 times, most recently from 3732717 to 30102c1 Compare September 26, 2024 14:31
@arkology
Copy link
Contributor

arkology commented Sep 26, 2024

Just some thoughts.
Maybe making this button yellow color is too much (may look like "something terrible happened, you forgot to turn off the kettle"😄). Maybe something more neutral? I mean, you enable autostart and will be always "warned" even if it is inside single session.
How about to show it only when launching project and running it? And with less "distractive" color. For not to warn but notify.

UPD 2025-03-19.
After using it for a while I thinks presence of such button is good overall as it allows to switch to active profiler really fast. But it takes some place at top bar and its color is still distracting.

@akien-mga
Copy link
Member

Looks pretty good to me overall!

I agree it might seem too distracting in the long run though, especially if some users want to always profile and are fine with the performance impact. But I'm also fine test driving it as is and seeing if users ask for changes.

@FoxyFox909
Copy link

FoxyFox909 commented Sep 26, 2024

Just some thoughts. Maybe making this button yellow color is too much (may look like "something terrible happened, you forgot to turn off the kettle"😄). Maybe something more neutral? I mean, you enable autostart and will be always "warned" even if it is inside single session. How about to show it only when launching project and running it? And with less "distractive" color. For not to warn but notify.

Maybe a softer yellow sure, but I agree with yellow in general since yellow = warning, and red = error in Godot already, and I do feel that this is more a light warning than a notification.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 26, 2024

I see some hard-coded values for the tab indices and profiler names. Ideally the profilers should register themselves to avoid hard-coding them. This can be refactored later though.

if (any_profiler_active) {
String tooltip = TTR("Autostart is enabled for the following profilers, which can have a performance impact:");
if (profiler_active) {
tooltip += "\n- " + TTR("Profiler");
Copy link
Member

@KoBeWi KoBeWi Sep 26, 2024

Choose a reason for hiding this comment

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

Not important, but it's better to use PackedStringArray or StringBuilder instead for building the tooltip.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

The implementation is ok, though as I mentioned, the hard-coding should be eventually removed.

@Geometror
Copy link
Member Author

Regarding the button's color and style: Initially, I went with a yellow icon and a flat button, but it looked off and wasn’t noticeable, so I inversed it. I realize this might feel a bit bold to some, but it provides a clear visual cue that your brain associates with "profiling mode." This can also work in reverse - you quickly notice when the indicator is absent, helping you spot when autostart isn't active but wanted. As for the color, if you can give me some suggestions I can try those, but IMO the default-warning-yellow works quite well here.

@KoBeWi I forgot to add a note on the hardcoded values :) Yep, that felt horrible, but I fear it's currently the only way to achieve this. (however, I've added this to my profiler refactoring list)

@akien-mga
Copy link
Member

Needs rebase.

@Geometror Geometror force-pushed the profiler-autostart-indicator branch from 30102c1 to 9f8bbe4 Compare December 16, 2024 09:15
@Geometror Geometror requested a review from a team as a code owner December 16, 2024 09:15
@Geometror
Copy link
Member Author

Rebased.

@Repiteo Repiteo merged commit 7efe038 into godotengine:master Dec 16, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 16, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants