Change main screen plugins into docks - #113051
Conversation
|
This would genuinely address many miscellaneous proposals and make the editor almost fully modular. Let's make it more than a proof-of-concept. |
|
I'm really looking forward to this! I think the positioning of the opened scenes tabs should be more like in this proposal: #12471 |
0ff9f83 to
b9a0b07
Compare
b9a0b07 to
8a6d6c7
Compare
|
I pushed compatibility code. It's somewhat awkward, because the usual workflow with main screens is that you add the dock as a child of a dedicated node and its data is available via virtual methods in EditorPlugin. I replaced that special control with a new one that intercepts added children and moves them to a dock. Unfortunately there are edge cases that are not 100% compatible:
|
8a6d6c7 to
88d5118
Compare
|
So after more changes there is yet another compatibility breakage, and this time a rather major one. Since the main screen docks are no longer associated with a specific plugin, changing main screen tab no longer calls |
c97df15 to
bbcff61
Compare
bbcff61 to
6aa1e6c
Compare
5914a67 to
a9afdc8
Compare
cc303d3 to
ab481c4
Compare
KoBeWi
left a comment
KoBeWi
left a comment
There was a problem hiding this comment.
Should be finished now. This is definitely the most intrusive part of the dock rework (compared to bottom panel and other docks), but it also allowed to simplify some parts of the code, like the screens are no longer accessed by name or index, but directly as the docks.
Definitely needs testing, I'm not 100% sure that all signals and callbacks work the same as before (at least the exposed ones; some internals were changed and are expected to be different). There is some minor compatibility breakage, see my previous comment.
| virtual String get_plugin_name() const override { return TTRC("2D"); } | ||
| bool has_main_screen() const override { return true; } | ||
| virtual String get_plugin_name() const override { return "2D"; } | ||
| bool has_main_screen() const override { return canvas_item_editor->get_current_layout() == EditorDock::DOCK_LAYOUT_MAIN_SCREEN; } |
There was a problem hiding this comment.
I'm not sure how much has_main_screen() is relevant anymore. There is some difference between main screen plugins and regular plugins, but I think it does not matter that much?
| Button *button = Object::cast_to<Button>(button_hb->get_child(i)); | ||
| if (button->get_text() == "Script") { | ||
| // Selected button is at or after the Script button. | ||
| // Only allow auto-switching if the selected tab is to the left of the Script tab. |
There was a problem hiding this comment.
This code is so arbitrary and probably not relevant anymore, when you can reorder the tabs 🤔
There was a problem hiding this comment.
Yeah, The intent was to allow adding plugins that should switch in #104010, even though inserting them there wasn't completely supported.
There should be some kind of flag on the dock can_auto_switch or something that is true for 2D/3D and false for others.
Though its only used when its in the main screen.
| } | ||
| if (button->get_text() == selected_plugin->get_plugin_name()) { | ||
| // Selected button is before the Script button. | ||
| if (dock->get_display_title() == selected_plugin->get_plugin_name()) { |
There was a problem hiding this comment.
This is not really true anymore .-. (although happens to work for the default main screens)
| virtual void set_plugin_version(const String &p_version); | ||
| virtual bool has_main_screen() const; | ||
| virtual void make_visible(bool p_visible); | ||
| virtual void selected_notify() {} //notify that it was raised by the user, not the editor |
There was a problem hiding this comment.
This can't be reasonably called anymore.
Also the comment wasn't really true I think. I used make_visible() instead.
2dab5ca to
f2da3f5
Compare
AdriaandeJongh
left a comment
AdriaandeJongh
left a comment
There was a problem hiding this comment.
The last remaining issue I had was fixed! This PR looks good from a usability standpoint:
- If you close a main dock, then you can reopen it using the Editor > Editor Docks menu.
- If the Game dock is set to float on run, you can leave it out of the tabs at the top. Nice and clean.
- Ability to reordering the main docks is nice.
Very soon, users will want to be able to split the main dock..... 😏
In my review, I didn't look at the code. I also didn't look at how backward compatibility is maintained for plugins, and I suspect that'll be important with this PR.
|
Apparently we have a tutorial for main screen plugins: https://docs.godotengine.org/en/stable/tutorials/plugins/editor/making_main_screen_plugins.html It's going to be outdated once this PR is merged. |
bruvzg
left a comment
bruvzg
left a comment
There was a problem hiding this comment.
Breaks window dragging on macOS:
Previously red areas were usable as title bar to drag window, with this PR, only green part can be used. The rest is likely covered by tab control.
|
Would it work if the mouse filter of the TabBar was set to PASS? |
No, because then you drag the window and the tab at the same time, which is totally borked behavior. |
kitbdev
left a comment
kitbdev
left a comment
There was a problem hiding this comment.
ScriptEditor and AssetStore need margins. (only when not floating)
If 2D and 3D is closed then switching to a scene with a 2d root node causes errors like:
ERROR: Node not found: "/root/@EditorNode@20438/@Panel@14/@Control@67/2D/@VBoxContainer@9920/@VSplitContainer@9925/@HSplitContainer@9927/@HSplitContainer@9929/@Control@9930/@SubViewportContainer@9931/@SubViewport@9932/Node2D" (absolute path attempted from "/root/@EditorNode@20438/@Panel@14/@VBoxContainer@16/DockVSplitMain/DockHSplitMain/DockVSplitLeftR/DockSlotLeftUR/Scene/@VBoxContainer@5305/@MarginContainer@5368/@SceneTreeEditor@5412").
When reloading the editor and the open scene has a 2d/3d root node, the 2d/3d editor gets opened even if a different one was last open.
DaveTheEggman
left a comment
DaveTheEggman
left a comment
There was a problem hiding this comment.
A few nitpicks, will try to review the implementation & test it later on
It will work if diff --git a/editor/editor_node.cpp b/editor/editor_node.cpp
index 1383c6a314..96b2e6b8c1 100644
--- a/editor/editor_node.cpp
+++ b/editor/editor_node.cpp
@@ -8913,6 +8913,7 @@ EditorNode::EditorNode() {
editor_main_screen = memnew(EditorMainScreen);
srt->add_child(editor_main_screen);
title_bar->set_center_control(editor_main_screen->get_internal_container());
+ editor_main_screen->get_tab_bar()->set_mouse_filter(Control::MOUSE_FILTER_PASS);
editor_dock_manager->register_dock_slot(editor_main_screen);
diff --git a/scene/gui/tab_bar.cpp b/scene/gui/tab_bar.cpp
index 17f7cdff87..e447ac975c 100644
--- a/scene/gui/tab_bar.cpp
+++ b/scene/gui/tab_bar.cpp
@@ -220,6 +220,10 @@ void TabBar::gui_input(const Ref<InputEvent> &p_event) {
}
}
+ if (hover != -1 && mb->get_button_index() == MouseButton::LEFT) {
+ accept_event();
+ }
+
if (rb_pressing && !mb->is_pressed() && mb->get_button_index() == MouseButton::LEFT) {
if (rb_hover != -1) {
emit_signal(SNAME("tab_button_pressed"), rb_hover); |
f2da3f5 to
2e57c61
Compare
YeldhamDev
left a comment
YeldhamDev
left a comment
There was a problem hiding this comment.
Seems that everything is working fine.
Shader editor also has this button, despite being a dock. I think it's because they got unified. I can remove it.
It would be for the better.
kitbdev
left a comment
kitbdev
left a comment
There was a problem hiding this comment.
Clicking on a script error in the Output dock does not switch to or focus the ScriptEditor (even if it isn't floating).
Multiple more places don't focus the ScriptEditor when it is floating, previously they did.
Opening a script from the filesystem or scene dock.
Clicking on debugger breakpoint, or from breakpoint hit when running.
Or EditorInterface.set_main_screen_editor("Script").
Should be fixed now. |
kitbdev
left a comment
kitbdev
left a comment
There was a problem hiding this comment.
Trying to open ProjectManager crashes.
|
Looking like a great change ,though weirdly enough while the latest merged pr ( specular directional lightmap) works, i can't seem to be able to open godot with this pr in both mac and in the android editor ( i am using the artifacts from the check tab). It might not be this pr's fault , but it's weird that the master branch opens correctly. |
| if (selected_plugin == new_editor) { | ||
| void EditorMainScreen::select_next() { | ||
| if (get_tab_count() == 0) { | ||
| return; |
There was a problem hiding this comment.
The auto switch isn't working, clicking 2D and 3D nodes doesn't switch between 2D and 3D on default layout. It seems like it is only working for tabs after Script.
I think the "before script" thing can be dropped, it should be based on the type of editor not the current order. And any plugins that used it would have inserted it as a button or something that won't work now anyway.
Edit: Github doesn't like commenting in this file or something, I meant this to be on EditorMainScreen::can_auto_switch_screens.
There was a problem hiding this comment.
The return value for was_script_tab was reversed.
it should be based on the type of editor not the current order.
How would that work?
I think this hack is fine for now, as it's closest to the previous behavior. It can be tweaked later; it sounds like something more involved. There is also https://github.com/godotengine/godot/pull/113051/changes#r2821748038
There was a problem hiding this comment.
Now all tabs can auto switch except for Script.
Sure, it can be fixed later.
I was thinking EditorDock would have a bool for it, false for everything but 2D and 3D.
Not sure if it needs to be exposed immediately.
syntaxerror247
left a comment
syntaxerror247
left a comment
There was a problem hiding this comment.
TitleBar is still broken in Android editor.
|
Try now. I couldn't reproduce it, but I changed the TabBar centering to be more independent of the layout. macOS may need re-testing too. |
TitleBar looks good now but |
What is that exactly? The docks are in a TabContainer, so floating or closed docks are not going to show. You can open such docks from Editor > Docks menu. If it really has to be a tab, and the dock can't be added to TabContainer, then it needs some fake dock that makes the actual Game screen appear. Or it can be part of touch actions panel, idk. EDIT: |
It's a panel rendered using Android views. 
Yeah, creating a fake dock sounds good to me. We just need |
Co-authored-by: =?UTF-8?q?Pa=CC=84vels=20Nadtoc=CC=8Cajevs?= <7645683+bruvzg@users.noreply.github.com>
|
Ok, added the fake dock. The implementation might not be the best, but it works. |
19 participants
As a continuation of the new dock system, this PR changes main screen editors into docks.
godot.windows.editor.dev.x86_64_bTnIQTeSQ3.mp4
As a result:
_has_main_screen()still exists in EditorPlugin and plugins that have a main screen are still "separate". However I'm not sure how much difference does it make. Like, ideally the method should returntrueif the plugin has a dock located in the main screen (so not floating or moved elsewhere), but from my testing it does not really matter.EDIT:
Closes godotengine/godot-proposals#14807