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

Fix drag notifications across windows. #71509

Closed

Conversation

reduz
Copy link
Member

@reduz reduz commented Jan 16, 2023

Fixes #44390.

@reduz reduz requested a review from a team as a code owner January 16, 2023 11:35
@reduz reduz force-pushed the drag-notifications-across-windows branch from 6ad09ee to 0d70640 Compare January 16, 2023 11:36
@akien-mga akien-mga added this to the 4.0 milestone Jan 16, 2023
@akien-mga akien-mga requested a review from bruvzg January 16, 2023 12:10
@Sauermann
Copy link
Contributor

Sauermann commented Jan 16, 2023

This is an alternative approach to #67531.

From what I can tell, this PR correctly solves a single problem (highlighting valid Drop locations of child Windows and SubViewports) of the many issues related to Drag and Drop with Windows and SubViewports, while the other PR intends to solve all of them.

Since the other PR no longer makes use of _propagate_viewport_notification it could be considered as superseding this PR.

Here is a MRP for stress-testing Drag and Drop with Windows and SubViewports BugDragNDrop.zip

@reduz
Copy link
Member Author

reduz commented Jan 17, 2023

@Sauermann This fixes sub-viewports too (although i realized a problem in this PR that needs to be further fixed) done. The proper handling of the drag preview needs to be done by moving it to a transparent window with passthrough (like tooltips), but that should be implemented in a separate PR.

while (n) {
Viewport *vp = Object::cast_to<Viewport>(n);
if (vp) {
base = this;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
base = this;
base = vp;

if (Object::cast_to<Viewport>(c)) {
continue;
Viewport *vp = Object::cast_to<Viewport>(c);
if (vp != nullptr) {
Copy link
Member

Choose a reason for hiding this comment

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

Not an issue but do we try to be consistent with the checks style?:thinking: I mean I get there are different preferences but you're doing it both ways in this PR (if (vp) and if (vp != nullptr)) so that's kinda awkward. 😄

@Sauermann
Copy link
Contributor

I wrote #71509 as a far simpler way to solve this. I believe It fixes drags/drops accross viewports more elegantly.

After investigating this topic more deeply, one problem area, that I was able to identify is, that the Viewport, that the mouse is over, is identified in the following code section:

godot/scene/main/viewport.cpp

Lines 1830 to 1845 in 23e10d1

// Use DisplayServer logic.
Vector2i screen_mouse_pos = DisplayServer::get_singleton()->mouse_get_position();
DisplayServer::WindowID window_id = DisplayServer::get_singleton()->get_window_at_screen_position(screen_mouse_pos);
if (window_id != DisplayServer::INVALID_WINDOW_ID) {
ObjectID object_under = DisplayServer::get_singleton()->window_get_attached_instance_id(window_id);
if (object_under != ObjectID()) { // Fetch window.
Window *w = Object::cast_to<Window>(ObjectDB::get_instance(object_under));
if (w) {
viewport_under = w;
viewport_pos = screen_mouse_pos - w->get_position();
}
}
}

And this code section doesn't account for SubViewports and embedded Windows.
My conclusion is, that in order to fix Drag and Drop for Viewports, it is necessary make this code-section aware of SubViewports and embedded Windows. This conclusion aligns with my tests of this PR with the previously linked MRP, which show that Drag and Drop with Viewports is not yet solved completely.

@reduz reduz force-pushed the drag-notifications-across-windows branch from 551d6b4 to d6a6876 Compare January 18, 2023 15:11
@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 17, 2023
@YuriSizov
Copy link
Contributor

Needs some consensus and conclusion. We can pick it for 4.0.x though, so bumping for now.

@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 19, 2023
@Sauermann Sauermann modified the milestones: 4.2, 4.3 Sep 26, 2023
@akien-mga akien-mga modified the milestones: 4.3, 4.4 Feb 14, 2024
@akien-mga
Copy link
Member

Superseded by #67531.

@akien-mga akien-mga closed this Feb 14, 2024
@AThousandShips AThousandShips removed this from the 4.4 milestone Feb 14, 2024
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.

Valid drop locations are not highlighted when inspector is made floating.
6 participants