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 editor setting to keep bottom panel state on play and stop game #91033

Merged

Conversation

Hilderin
Copy link
Contributor

@Hilderin Hilderin commented Apr 22, 2024

This PR adds the editor settings run/output/keep_current_bottom_panel_on_play. The setting is false by default keep the old behavior.
It should close the proposal: godotengine/godot-proposals#9513

image

The objective is to override the settings run/output/always_close_output_on_stop and run/output/keep_current_bottom_panel_on_play so the bottom panel stay the way it was when starting and stopping the game.

Production edit:

@Hilderin Hilderin requested review from a team as code owners April 22, 2024 23:31
@Calinou Calinou added this to the 4.x milestone Apr 22, 2024
@Hilderin Hilderin force-pushed the keep_current_bottom_panel_on_play branch 3 times, most recently from 1e7e4b7 to e78ac72 Compare April 25, 2024 23:48
@Hilderin
Copy link
Contributor Author

Following the suggestions of @passivestar and @Calinou, I made the following modifications: I removed the new settting and the old settings Action On Play and Action On Stop and replaced them with 2 new settings:
Action On Play: Open Output (default), Open Debugger and Do Nothing
Action On Stop: Close Bottom Panel and Do Nothing (default)

I also created a migrate_settings method in EditorSettings to convert old settings into the new ones.

This is what it looks like:
image
image
image

With these modifications, we should be able to close the proposal godotengine/godot-proposals#8507 and godotengine/godot-proposals#9513

Comment on lines 821 to 822
EDITOR_SETTING(Variant::INT, PROPERTY_HINT_ENUM, "run/output/action_on_play", 0, "Open Output,Open Debugger,Do Nothing")
EDITOR_SETTING(Variant::INT, PROPERTY_HINT_ENUM, "run/output/action_on_stop", 1, "Close Bottom Panel,Do Nothing")
Copy link
Member

Choose a reason for hiding this comment

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

Since this is no longer specific to the Output panel, we should maybe rename the category.

Maybe run/bottom_panel/ ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Was about to suggest this. The 2 old output-related ones can be put into an "Output" subsection (because you woudn't know what "Font Size" refers to otherwise)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved them in a new section "Bottom panel":
image

@akien-mga
Copy link
Member

I think this is a good approach 👍

Given the popularity of godotengine/godot-proposals#8507 though, I wonder if we should consider making the default for "Action on Play" also to do nothing?

We could trial it in 4.3 beta, and see if the feedback we get is mostly that people miss having the Output dock automatically displayed, or actually like this new behavior.

One potential concern would be for new users to outright miss the fact that there's an Output panel where messages from their running game are displayed. There might be other ways to raise awareness about it, like adding a (*) in the title to show that there are unread messages or something? Ideas welcome.

@passivestar
Copy link
Contributor

"Open Debugger" currently opens the output panel for me

@passivestar
Copy link
Contributor

Also I just randomly found another PR that can be closed when this one is merged: #66503

@akien-mga
Copy link
Member

akien-mga commented Apr 26, 2024

Also I just randomly found another PR that can be closed when this one is merged: #66503

And so this PR should also:

And while at it, see also:

@passivestar
Copy link
Contributor

One potential concern would be for new users to outright miss the fact that there's an Output panel where messages from their running game are displayed

@akien-mga Was it ever discussed to add an option to overlay output in the game itself like unreal does? I feel like godot might benefit from that a lot since it runs the game in a separate window and it can be inconvenient to see the output in a single-monitor setup

@Hilderin
Copy link
Contributor Author

"Open Debugger" currently opens the output panel for me

Effectively, I used the wrong value in EditorNode::_project_run_started
It will be fixed in my next commit.

@Hilderin
Copy link
Contributor Author

Given the popularity of godotengine/godot-proposals#8507 though, I wonder if we should consider making the default for "Action on Play" also to do nothing?

I changed the defaut value for "Action on Play" to "Do nothing". I'll wait for more feedback to see if we change it back to "Open Output".

@Hilderin Hilderin force-pushed the keep_current_bottom_panel_on_play branch from e78ac72 to 047eefb Compare April 26, 2024 09:37
@passivestar
Copy link
Contributor

Opening debugger works now

I have this in Output though:

image

If you make the debugger keep the currently opened tab instead of switching to stack trace this PR will also close godotengine/godot-proposals#7848. But I don't know how easy it is to do, so it's your call

@Hilderin
Copy link
Contributor Author

I have this in Output though:

Hum... very weird... I don't have this behavior and I don't see how it is even possible, this setting has been removed from editor_settings.cpp!! Someone have a suggestion to fix this?!?!
image

@Hilderin
Copy link
Contributor Author

Hilderin commented Apr 26, 2024

If you make the debugger keep the currently opened tab instead of switching to stack trace this PR will also close godotengine/godot-proposals#7848. But I don't know how easy it is to do, so it's your call

Ok, it was kinda too easy:
image

@Hilderin Hilderin force-pushed the keep_current_bottom_panel_on_play branch from 047eefb to 46d0b29 Compare April 26, 2024 10:17
@passivestar
Copy link
Contributor

Hum... very weird... I don't have this behavior and I don't see how it is even possible, this setting has been removed from editor_settings.cpp!! Someone have a suggestion to fix this?!?!

If I manually remove it from my editor-settings-4.tres it's gone, I don't know if it's something that will happen automatically when people update editor versions. iirc there are now also minor versions of editor settings (editor-settings-4.3.tres, etc), but I don't know how it migrates, if it's blindly copies it or actually checks individual settings. Somebody who knows will need to clarify

@passivestar
Copy link
Contributor

passivestar commented Apr 26, 2024

Ok, it was kinda too easy

Tested, works as expected

@akien-mga This PR now also closes both of godotengine/godot-proposals#7848 and godotengine/godot-proposals#3480

Probably also closes #60898 because it's now possible to keep the Animation tab opened when working on animations

@Hilderin Hilderin force-pushed the keep_current_bottom_panel_on_play branch from 46d0b29 to b8466f0 Compare April 28, 2024 12:52
@Hilderin
Copy link
Contributor Author

Hilderin commented Apr 28, 2024

I placed "Do nothing" first in both settings, that way, the default option is the first and it will be easier to add new options in the future without having "Do nothing" in the middle.

Also, I found a problem with the "Close bottom panel" on stop, it was closing the bottom panel only if current panel was Output. It now always closes the bottom panel whatever panel you are on. This changes the behavior of the previous option "Always close on stop". I think it's ok and makes more sense??

image
image

@passivestar
Copy link
Contributor

Also, I found a problem with the "Close bottom panel" on stop, it was closing the bottom panel only if current panel was Output. It now always closes the bottom panel whatever panel you are on. This changes the behavior of the previous option "Always close on stop". I think it's ok and makes more sense??

This makes sense to me, I could imagine somebody for example messing with the debugger when the game is running and wanting it to close when the game stops

If we dive deep we'd probably find an even better design for all of this but tbh I think it's good enough at this point. This PR solves a bunch of actual reported problems, including mine

@KoBeWi
Copy link
Member

KoBeWi commented May 24, 2024

I don't know if it's something that will happen automatically when people update editor versions.

It won't. See godotengine/godot-proposals#4444

@Hilderin Hilderin reopened this May 24, 2024
@Hilderin Hilderin force-pushed the keep_current_bottom_panel_on_play branch from b8466f0 to e4b2dde Compare May 24, 2024 16:28
@Hilderin
Copy link
Contributor Author

This last commit adds 2 enums and move the code from editor_debugger_node to editor_node which effectively is much cleaner.
thanks @KoBeWi

editor/editor_node.h Outdated Show resolved Hide resolved
@Hilderin Hilderin force-pushed the keep_current_bottom_panel_on_play branch 2 times, most recently from 907e1e0 to ee2fac0 Compare May 24, 2024 16:40
editor/editor_node.h Outdated Show resolved Hide resolved
editor/editor_settings.cpp Outdated Show resolved Hide resolved
@Hilderin Hilderin force-pushed the keep_current_bottom_panel_on_play branch from ee2fac0 to 813fc9f Compare May 24, 2024 17:46
@akien-mga akien-mga modified the milestones: 4.x, 4.4 May 24, 2024
@akien-mga akien-mga modified the milestones: 4.4, 4.3 May 24, 2024
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

I think this new default makes sense, but I'm worried about users potentially not noticing where their print() output goes (particularly for beginners). I wonder if we should make it so the Output panel is opened if you currently don't have any panel open.

Perhaps this could also be alleviated by adding an "unread messages count" to the Output panel button's text, like Debugger already gets when warnings/errors are printed. I'd prefer this solution personally, but it should probably be implemented in a separate PR.

@Hilderin
Copy link
Contributor Author

Hilderin commented May 27, 2024

I understand your concern and like your idea of unread count like the debugger. I think when you start Godot for the first time the default layout has the Output panel open. In fact, I remember as a new user searching how to close this panel at the bottom. My point is if the Ouput panel is there by default, the user should see the output by default because the default setting is Do nothing.

@Hilderin
Copy link
Contributor Author

I'm thinking about what @Calinou said and I don't see a lot of possibilities. The easy solution is to change the default value for Action on play to be Open output, which will be equivalent to Godot 4.2.

I'm not sure about the suggestion "I wonder if we should make it so the Output panel is opened if you currently don't have any panel open." because I personally don't use the builtin print. I have a custom print that prints output directly in the game (similar to Unreal) and Godot prints thinks that I don't need.

@KoBeWi
Copy link
Member

KoBeWi commented May 27, 2024

I'm not sure about the suggestion "I wonder if we should make it so the Output panel is opened if you currently don't have any panel open." because I personally don't use the builtin print.

It could be one of the available options, but IMO it's too specific.

@akien-mga
Copy link
Member

Given the above discussion, I think we can still merge this now to include in 4.3 (despite feature freeze - this is a relatively safe change and with a lot of demand), but given the timeline I would opt for using defaults that match the pre-existing behavior for now.

We won't have much time during the short beta period for collecting user feedback about potential new defaults, so I think it's safer to provide the extra features while keeping the same defaults for 4.3. And we can try out changing the defaults for 4.4 early on the dev cycle, so we have several months to gather and address user feedback.

@Hilderin Hilderin force-pushed the keep_current_bottom_panel_on_play branch from 813fc9f to 76205d4 Compare May 29, 2024 09:45
@Hilderin
Copy link
Contributor Author

Wonderful! I changed the default value for Action on play to be Open output, which should match the pre-existing behavior.

@akien-mga akien-mga merged commit c42751c into godotengine:master May 29, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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