Conversation
|
This isn't an issue with this PR, but since we're changing how the dock customization works would it be possible to take #106428 into account? In my opinion it is better to have the brittle Texture2D to be the priority rather than the fallback because it would allow for overrides and the more dynamic StringName (it is able to be drawn in dark mode, for example) to be the fallback. |
KoBeWi
force-pushed
the
docker_exe
branch
5 times, most recently
from
441e53c to
bb9c614
Compare
|
Should be ready now. I included #106428 too. |
|
I still think the icon_name has a use because it is a lot more flexible (it can be used in light or dark mode), maybe it could be |
|
I mentioned this already in the Godot Contributors Chat, but I'm concerned about every dock being added to the menubar under "Editor Docks". The problem is that this shows every possible context-sensitive dock to the user, even when they wouldn't normally be visible. For example, the "Tilemap" dock usually only appears when you are editing a Tilemap. I'm not exactly sure under what circumstances you'd want to access that dock when you're not editing a Tilemap. As goofy as the Microsoft Office Ribbon is, I feel that this is one aspect they did get right at least. In versions of Microsoft Office before 2007 the UI was crammed with a lot of toolbars, docks, selectable windows, that sort of thing, for features that aren't even active so they're just taking up space. I could see the need to be able to access a dock that isn't context-relevant if you want to adjust where it is in the layout, but I'm not sure how best to approach that. Some programs temporarily show any stuff that would normally be hidden when you go into the "layout edit mode" but Godot doesn't really have that. I do like the idea of being able to move docks that would normally be bottom-locked, and put them in other parts of the UI. But also a lot of bottom docks are situational, and them being at the bottom communicates that. So moving them elsewhere doesn't really fit the established paradigm. |
|
That's unrelated to this PR though. It only changes internals, the editor so far works the same as before. |
There was a problem hiding this comment.
The main content looked good from what I could see, although there were quite a few object leaks:
WARNING: 5 RIDs of type "CanvasItem" were leaked.
at: _free_rids (servers/rendering/renderer_canvas_cull.cpp:2678)
ERROR: 6 RID allocations of type 'N10RendererRD14TextureStorage7TextureE' were leaked at exit.
WARNING: 8 RIDs of type "Texture" were leaked.
at: finalize (servers/rendering/rendering_device.cpp:7164)
ERROR: 73 RID allocations of type 'PN18TextServerAdvanced22ShapedTextDataAdvancedE' were leaked at exit.
ERROR: 18 RID allocations of type 'PN18TextServerAdvanced12FontAdvancedE' were leaked at exit.
ERROR: 1 RID allocations of type 'PN18TextServerAdvanced27FontAdvancedLinkedVariationE' were leaked at exit.
WARNING: ObjectDB instances leaked at exit (run with --verbose for details).
at: cleanup (core/object/object.cpp:2489)
ERROR: 2 resources still in use at exit (run with --verbose for details).
at: clear (core/io/resource.cpp:637)
It might be an issue with master. I'll check. Yeah it has to do with master. Disregard above.
KoBeWi
force-pushed
the
docker_exe
branch
2 times, most recently
from
45c3698 to
dda9767
Compare
lodetrick
left a comment
lodetrick
left a comment
There was a problem hiding this comment.
It looks good now from what I can see. Thanks for your effort!
Mickeon
left a comment
Mickeon
left a comment
There was a problem hiding this comment.
I do still find a bit weird that available_layouts exists as a future-proof measure. Docs are fine, now it's a matter to see if the PR is good to go.
KoBeWi
force-pushed
the
docker_exe
branch
2 times, most recently
from
5ab5179 to
b6454cb
Compare
|
Rebased. I also marked EditorDock as experimental for now. |
| <return type="void" /> | ||
| <param index="0" name="dock" type="EditorDock" /> | ||
| <description> | ||
| Adds a new dock. |
There was a problem hiding this comment.
| Adds a new dock. | |
| Adds a new dock. Use [member DockSlot.default_slot] first to set the dock slot it will open to. |
Maybe we should add a mention of the default slot here to ease the transition? Not sure.
| const String section_name = p_section + "/" + dock->get_effective_layout_key(); | ||
| dock->save_layout_to_config(p_layout, section_name); |
There was a problem hiding this comment.
There is a problem with saving an Editor Layout using different section names, each section is detected as a separate layout. This causes extra layouts:
So either they shouldn't be separate sections or some of the code that gets the layout list at EditorNode::_update_layouts_menu() and EditorLayoutsDialog::_post_popup() needs to be updated to account for this.
There was a problem hiding this comment.
Fixed, but IMO keeping all layouts in one config file is a bad idea.
There was a problem hiding this comment.
If a layout can have multiple sections like here, then it makes sense to put them in separate files.
Getting the list works now.
Deleting doesn't delete the full layout, it leaves [custom1/FileSystem] and etc sections in the file. It doesn't really cause issues so we can fix this later if we want.
|
Thanks! |
7 participants
Closes godotengine/godot-proposals#12435
Fixes #106927
There are some differences from the proposal. The new class is called EditorDock and inherits MarginContainer.
I still need to expose it. For now I changed all docks to use the new system.
EDIT:
Done. I made an example plugin that uses the new system:
dock-test.zip
It can be moved to bottom and supports saving layout.
Also supersedes #106428