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

Enable Drag and Drop between SubViewports and Windows #67531

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Oct 17, 2022

Make Drag and Drop an application-wide operation.
This allows to drop on Controls in other Viewports/Windows.

Also now all nodes in the SceneTree are notified about the Drag and Drop operation, with the exception of SubViewports that are not child-nodes of SubViewportContainers. This distinction seemed prudent, because these SubViewports need to be displayed manually and this requires manual setup also for Drag and Drop.

Unit-tests for Drag and Drop in different Windows & SubViewports are also added.

In order to achieve this, the following changes were made:

  • Viewport::_update_mouse_over from Refactor mouse_entered and mouse_exited signals #67791 is used to identify the drop-target (even within nested viewports)
  • This Control is used as a basis for the Drop-operation, which replaces the previous algorithm, that was only aware of topmost Viewports.
  • In order to handle DnD within non-default SubViewports (see for an example Rewrite Gui in 3D demo to use Physics Picking for mouse events godot-demo-projects#925), it becomes necessary to track drag-operations in each (for the lack of a better name) section in the scene-tree.
  • is_directly_attached_to_screen is replaced by get_section_rootviewport. which is of the same complexity, but additionally returns the topmost Viewport within the section
  • _propagate_viewport_notification had to be adjusted to account for sections
  • DisplayServerX11::delete_sub_window needed to be adjusted similar to Fix crash on Windows when closing Window #80142, since the event-callback is also needed on linuxbsd during Window-deletion

Relations:

MRP: BugDragNDrop.zip (updated 2023-01-20)

The Drag-Preview in the following Video is outdated, because that will be done in a different PR.

BugDragNDrop.mp4

Updated 2023-01-28: Fix merge conflict
Updated 2023-03-20: Fix merge conflict
Updated 2023-05-30: Linked another bug-report, that this PR fixes. Resolve problematic testcases. Fix merge conflict.
Updated 2023-06-07: Fix merge conflict
Updated 2023-06-08: Fix issue with docks
Updated 2023-06-19: Fix issue with simultaneous drag starts
Updated 2023-07-06: Fix issue with Gui in 3D Demo, Add Unit-tests
Updated 2023-07-19: Fix merge conflict
Updated 2023-08-02: Fix merge conflict with #67791 and #79805
Updated 2023-08-12: Fix merge conflict
Updated 2023-09-18: Fix merge conflict with #81551
Updated 2024-01-03: Fix merge conflict with #86511 and verified that #86493 doesn't get reintroduced.
Updated 2024-05-03: Rebased after #91425 and verified that #91387 doesn't get reintroduced.
Updated 2024-09-15: Rebased after 4.3-release for good measure and verified the functionality in the MRP.

@rokiyo
Copy link

rokiyo commented Oct 17, 2022

Fixes #67186
Potentially supersedes #64949 ?

@Sauermann
Copy link
Contributor Author

@rokiyo Thanks, I did not see these.

@Sauermann Sauermann force-pushed the fix-drag-n-drop branch 4 times, most recently from efe0752 to 1f07643 Compare October 19, 2022 23:20
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Seems to be working fine.

Note: At least on macOS, mouse cursor is changed to <-> when crossing resizable window border, and not restored to DnD cursor afterwards (until drop target is hovered).

scene/main/viewport.cpp Outdated Show resolved Hide resolved
@Sauermann Sauermann force-pushed the fix-drag-n-drop branch 2 times, most recently from 6b25c96 to a52d416 Compare October 20, 2022 09:07
@Sauermann
Copy link
Contributor Author

Sauermann commented Oct 20, 2022

Note: At least on macOS, mouse cursor is changed to <-> when crossing resizable window border, and not restored to DnD cursor afterwards (until drop target is hovered).

That sounds like Godot believes that the mouse cursor is still FORBIDDEN while macOS changed it to RESIZE.
I believe that this is a platform-specific problem, that should be fixed in the macOS-DisplayServer?

scene/main/viewport.cpp Outdated Show resolved Hide resolved
@Sauermann
Copy link
Contributor Author

This implementation could be greatly simplified, if the functionality of #67791 was available.
Converting to draft, until a decision on #67791 is made.

@Chaosus
Copy link
Member

Chaosus commented Jan 14, 2023

@Sauermann how is your progress with this PR?

@Sauermann
Copy link
Contributor Author

Sauermann commented Jan 14, 2023

@Chaosus
This PR is blocked by progress on #67791, which is waiting on reviews.
Both implement the same algorithm in different locations and #67791 implements it in a location, where it additionally fixes several bugs (which can not be fixed by this PR). So my recommendation would be to review and merge #67791 before I continue working on this PR.

Also #58334 will cause a few small adjustments to this PR, but I consider them minor. Ideally I would prefer, that #58334 should be merged before this PR, but that is not strictly necessary.

@reduz
Copy link
Member

reduz commented Jan 17, 2023

I wrote #71509 as a far simpler way to solve this. I believe It fixes drags/drops accross viewports more elegantly. What is missing is proper drag previews across windows, but the correct solution to this in my mind is to use borderless, transparent, passthrough windows similar to what we use with tooltips.

@Sauermann
Copy link
Contributor Author

The complexity in this PR comes from the fact that is solves three problems:

  1. Drag and Drop in different Windows/SubViewports
  2. Drag-Preview in different Viewports (The idea of using borderless, transparent, passthrough windows is superior to my own current approach)
  3. Fix mouse-enter-exit-signals during Drag and Drop

In order to be able to compare it with #71509 (which aims to solve 1), I believe, that I should extract the part of this PR, that solves 1, so that they can be compared better. It looks like I am struggling again with packing too much content into a single PR.

From a quick comparison, this approach makes Drag and Drop an application-wide operation, while #71509 uses a heuristic to determine a Viewport to use as a base for the Drag and Drop operation.

@akien-mga
Copy link
Member

Could use testing on macOS (and review if you can) @bruvzg

@EiTaNBaRiBoA
Copy link

This is if not the most important feature to distinguish between flutter and godot.
If godot will support it, it literally ends flutter, wpf, Qt and many more. I work as a software engineer and multi window communication is a deal breaker for many companies. This feature solves it and bring plenty of possibilities in the future. Please add this

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let's merge if you think now would be a good time.

doc/classes/Viewport.xml Outdated Show resolved Hide resolved
scene/main/viewport.cpp Outdated Show resolved Hide resolved
scene/main/viewport.cpp Outdated Show resolved Hide resolved
scene/main/viewport.h Outdated Show resolved Hide resolved
scene/main/viewport.h Outdated Show resolved Hide resolved
tests/scene/test_viewport.h Outdated Show resolved Hide resolved
@akien-mga akien-mga modified the milestones: 4.3, 4.4 Feb 14, 2024
@akien-mga
Copy link
Member

akien-mga commented Feb 14, 2024

We decided to keep this for the 4.4 cycle, as it's pretty late now in the 4.3 cycle and this PR changes behavior in a way that may clash with user expectations. So we don't want to rush it / have to reconsider the design during the 4.3 beta/RC stage.

Make Drag and Drop an application-wide operation.
This allows do drop on Controls in other Viewports/Windows.

In order to achieve this, `Viewport::_update_mouse_over` is adjusted to
remember the Control, that the mouse is over (possibly within nested
viewports). This Control is used as a basis for the Drop-operation, which
replaces the previous algorithm, which was only aware of the topmost
Viewport.

Also now all nodes in the SceneTree are notified about the Drag and Drop
operation, with the exception of SubViewports that are not children of
SubViewportContainers.
@akien-mga
Copy link
Member

So now would be a good time to merge this for 4.4 I think?

@akien-mga akien-mga merged commit b92fe32 into godotengine:master Sep 16, 2024
20 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