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

Make Editor dock resizing consistent #83359

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kitbdev
Copy link
Contributor

@kitbdev kitbdev commented Oct 15, 2023

When adjusting the split between docks it will now only adjust the areas to the immediate sides of it, unless the minimum size is reached, then it affects the next area over until it cannot move anymore.

godot_dock_resizing_issue_fix

Implementation details:
Added push_parent bool in SplitContainer

  • When dragging has hit the minimum size, try to push its parent SplitContainers so that dragging can continue in them.
  • If the parent does not have 2 splits or is not the same orientation, then its parent is checked. This allows for more complex hierarchies.
  • Pushing the parent will emit the dragged signal in the parent.

Added resize_separately bool in SplitContainer

  • This adjusts the split offset of its children when dragging, so splits not immediately next to the drag area do not get resized.
  • If push_parent is false, resizing does move past the opposite split, even if could otherwise.

Also fixed dragging not stopping when focus is lost, and dragging while in a moving SplitContainer that has its child's size flag set.

With only resize_separately the SplitContainers work like Blender's dock system does when moving splits.
With both push_ancestors and resize_separately enabled, the SplitContainer hierarchy works together to have a better experience when resizing.

edit: fixed minimum size not handled correctly
edit: fixed rtl layout, scaling issue, dragging with both expand flags set, and other problems

@kitbdev kitbdev requested review from a team as code owners October 15, 2023 00:36
@Chaosus Chaosus added this to the 4.3 milestone Oct 15, 2023
@kitbdev kitbdev marked this pull request as draft October 17, 2023 04:04
@kitbdev kitbdev force-pushed the fix-dock-resizing branch 3 times, most recently from 354ad72 to c17921e Compare October 18, 2023 17:55
@kitbdev kitbdev marked this pull request as ready for review January 11, 2024 20:44
@kitbdev kitbdev requested a review from a team as a code owner January 16, 2024 20:45
@kitbdev kitbdev force-pushed the fix-dock-resizing branch 2 times, most recently from f1e1ab1 to 6a30364 Compare April 2, 2024 20:46
@kitbdev kitbdev requested a review from a team as a code owner April 2, 2024 20:46
@kitbdev
Copy link
Contributor Author

kitbdev commented Apr 2, 2024

Rebased
Updated the Debugger Stack Trace (Script Editor Debugger)
Before:

godot sc stack trace not fixed

After:

godot sc stack trace fixed

I could have this for the File Dialog when there are three panels, like the export scene to gLTF, but its not as much of a problem there.
There may be other places where this could be used though.

Changed it to not emit the dragged signal in the parent, since it wouldn't work well with signals like drag_started in #72680.
If a signal is wanted, a new one can be used.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 3, 2024

Noticed some weird behavior with this setup:
image

godot.windows.editor.dev.x86_64_cyKIT8o1q9.mp4

Both containers have resize_separately enabled.

scene/gui/split_container.h Outdated Show resolved Hide resolved
scene/gui/split_container.h Outdated Show resolved Hide resolved
scene/gui/split_container.cpp Outdated Show resolved Hide resolved
scene/gui/split_container.cpp Outdated Show resolved Hide resolved
scene/gui/split_container.cpp Outdated Show resolved Hide resolved
scene/gui/split_container.cpp Outdated Show resolved Hide resolved
scene/gui/split_container.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Apr 3, 2024

This seems to be working correctly in the editor, but the SplitContainer changes are rather complex.
CC @groud I remember you liked dealing with this class :v

@kitbdev kitbdev force-pushed the fix-dock-resizing branch from 6a30364 to 3382981 Compare April 3, 2024 22:15
@kitbdev kitbdev force-pushed the fix-dock-resizing branch from 3382981 to c6dc192 Compare April 3, 2024 22:16
@kitbdev
Copy link
Contributor Author

kitbdev commented Apr 3, 2024

The issue with the nested second SplitContainer moving strangely when clamping was caused by the children updating their splits and minimum sizes before the parent was resorted. It should be fixed now, I made sure that sorting happens before resizing child splits. This meant I could also remove a workaround I had in rtl mode.
Also applied above suggestions.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 4, 2024

Something still seems not right 🤔

godot.windows.editor.dev.x86_64_XFBOfVMS3F.mp4

Same setup as before, both have push_nested disabled.

@groud
Copy link
Member

groud commented Apr 4, 2024

Thanks for the contribution!

Sooooo, from a quick look the API seems a bit convoluted and a bit confusing IMO.
I think the simplest way to solve the original problem would instead to allow a SplitContainer to have more than two children. Have you considered this solution too?

@kitbdev
Copy link
Contributor Author

kitbdev commented Apr 4, 2024

I think the simplest way to solve the original problem would instead to allow a SplitContainer to have more than two children. Have you considered this solution too?

Allowing more than two children would work for the current system, but if the layout was changed to be any more complex then it wouldn't. For example, it wouldn't work for a layout like:

image

(godotengine/godot-proposals#4565) or other dock customization proposals with functionality like:

Screenshot 2024-04-04 140926

Also using SplitContainer with multiple children would probably break compatibility with dock layouts, since the split offset(s) may need to be different.

That said, this implementation is pretty complex and I would like it to be simpler.
Maybe instead of each SplitContainer pushing each other, we could find the top-level SplitContainer and have it handle dragging might be simpler? Or simplest way to handle it might be to make a new node.
I'm not sure what the best way would be.

Same setup as before, both have push_nested disabled.

I found a fix for this, but I found an another issue where the nested SplitContainer isn't being resized separately.

@groud
Copy link
Member

groud commented Apr 4, 2024

Allowing more than two children would work for the current system, but if the layout was changed to be any more complex then it wouldn't. For example, it wouldn't work for a layout like:

Hmm I am not sure why it would not. Like, it would work 99% of the time anyway. The only thing I'd say cannot be solved would likely be a "smaller separator" (like those in the top left), pushing a bigger one (on the right of your mockup). But it seems prettyh doable to me.

Also using SplitContainer with multiple children would probably break compatibility with dock layouts, since the split offset(s) may need to be different.

That's not a big issue, the TileMap node already does it by using a special setter (in the set(...) function). In such case, you can reinterpret the old value and save it to the new format I believe. So, well, it should not be too difficult to keep compatibility there.

@Geometror
Copy link
Member

I'm in favor of adapting SplitContainer to handle multiple children; it's something I've wanted to implement for a long time but never got to it.

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.

Resizing Editor Docks is not consistent.
6 participants