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

[Regression] SubViewportContainer doesn't call _CanDropData event #99155

Closed
conde2 opened this issue Nov 13, 2024 · 8 comments · Fixed by #99270
Closed

[Regression] SubViewportContainer doesn't call _CanDropData event #99155

conde2 opened this issue Nov 13, 2024 · 8 comments · Fixed by #99270

Comments

@conde2
Copy link

conde2 commented Nov 13, 2024

Tested versions

v4.4.dev4.mono.official [36e6207]

System information

Godot v4.4.dev4.mono - Windows 10.0.19045 - Multi-window, 1 monitor - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3060 Ti (NVIDIA; 32.0.15.6094) - AMD Ryzen 7 3700X 8-Core Processor (16 threads)

Issue description

Overriding _CanDropData on a SubViewportContainer doesn't work, the event is never called.

Steps to reproduce

Just create a SubViewportContainer and add the _CanDropData and check that the event is not called when trying to drop something.

Minimal reproduction project (MRP)

I don't have any at the moment. Will work on adding one later.

@matheusmdx
Copy link
Contributor

Bisected to #67531, @Sauermann

image

@Sauermann
Copy link
Contributor

#67531 introduced a conceptional change for how Drag&Drop works.
One effect of that PR was, that SubViewportContainer was put more into the form of a pure interface for SubViewport nodes, reducing their functionality, especially as noted in #99155 that it is no longer available as a Drop-target in Drag&Drop operations.

This reduction in functionality for SubViewportContainer change was necessary to make application-wide Drag&Drop possible and boils down to the following question: In the scene tree

  • SubViewportContainer
    • SubViewport
      • Control

the two nodes SubViewportContainer and Control are potential Drop-targets and both occupy the same position. Which of these two should be considered as Drop-target, when the mouse is over both of them?

One basic axiom/assumption is, that only a single Control node will receive the Drop event. In order to enable, that more than one Control node receives that Drop event, we would need a meaningful proposal or problem/project description, why that would be necessary, because it would make the code much more complex. I don't see the argumentation within this issue as sufficient reason for such a change.

So there are currently two possible solutions:
(A) SubViewportContainer receives the Drop-event
(B) Control receives the Drop-event

In #67531, I discarded (A) as a potential solution, because that would make it much more work-intensive for users to drop on Control nodes within SubViewport. So I went with (B) and discarded SubViewportContainer as a potential drop target, whenever it contains a SubViewport.

A potential solution could be a compromise, that whenever the mouse is over a Control in the the SubViewport, that Control will be considered as Drop-target and if the mouse isn't over a Control within the SubViewport, then the SubViewportContainer is used as Drop-target. But I don't find such a solution ideal, because the functionality of the SubViewportContainer Drop-status depends on how the SubViewport is structured and it seems potentially very unpredictable to me.

Personally I would prefer that users should consider SubViewportContainer as a minimal interface for SubViewport nodes and try not to pack additional functionality (like Drag&Drop) into these nodes, so potentially in my opinion this issue could be resolved by improving the documentation of SubViewportContainer.

@matheusmdx
Copy link
Contributor

With the explanation that wasn't a bug and with the chance of renabling this can lead to bugs i think we should keep this behavior and update SubViewportContainer docs to inform about this limitation, even being a "break compatibility" change, the solution is easy to apply and probably won't affect a large amount of projects.

@conde2
Copy link
Author

conde2 commented Nov 14, 2024

If that's the case I can probably workaround this issue by adding a control over the entire screen.

I chose the SubViewportContainer just because I needed to drag and drop items from my inventory to the world map, and SubViewportContainer covers the whole viewport.

So it seemed easier doing it like that. Tomorrow I can test and see if I can easily change to another control node.

Anyway this is a regression and should've been added to the Known issues in the blog post for 4.4dev4 release, or noted somewhere so people can know about this issue.

@conde2
Copy link
Author

conde2 commented Nov 14, 2024

UPDATE: Moving to a Control under the SubViewport fixes the problem. It was a easy fix, but I still belive people will report this as a problem in the future as SubViewportContainer is a Control and its expected that the drag events works on Controls.

Not sure what is the best solution here anyways will leave to the Godot Team to decide.

@jopheno
Copy link

jopheno commented Nov 15, 2024

I had the exact same problem as @conde2, when attempting to throw items on the world map from the inventory I used to connect on the SubViewportContainer; But not it no longer works... So I came here and saw that a solution would be to use a Control node that is the whole screen size, so I did it... But it doesn't work for my game (although it kind works)...

The main reason for it to not work for my game is because it completely breaks the Physics Pickup functionality... as soon as I have a control that takes the whole screen it no longer triggers the Area2D input_event...
I created a minimal project which I will upload here as well; Before the system worked just fine with physics pickup (mouse events passing to CollisionObjects) because when adding to the SubViewportContainer it allowed physics pickup, now it no longer does once the mouse event is picked by the Control node and no longer goes to the viewport for the Physics Pickup process...

@Sauermann, please explain how we can fix this issue using this approach, otherwise this reduction in functionality must be postponed until this has a definition...

Also, I did some research and it seems that this issue might be related with this issue over here to some degree: #95116

On the documentation it says that Objects Physics is the last of the things executed and taken into consideration on the input cascade... On the issue which @AdamLearns points out, that doesn't seem to fit what is being explained in the docs, it is somehow marking inputs as handled for some nodes on his example, so maybe the issue starts here?

I also did an example of my own to try reproducing his issue and added the UnhandledInput event on the FullScreenControl:

    public override void _UnhandledInput(InputEvent @event)
    {
        GD.Print($"FullScreenControl: _UnhandledInput {@event}");
    }

As soon as I enable the Object Picking in the Viewport it no longer prints the UnhandledInput event execution, which means that the documentation is not being respected what so ever (as it says it is the last thing considered, but it is impacting the early in the input cascade inputs)... Is it just a problem of documentation? Maybe it is, or maybe the Physics Picking Event is just wrong and #67531 was worked on thinking it is working just as documentation suggests, but it is not? Idk...

I tried to find a workaround, but couldn't find any; My only possibility here is to rollback to stable version to get both the Drag & Drop and the Physics Picking features to work together;

Tested versions

v4.4.dev4.mono.official [36e6207]

System information

Godot v4.4.dev4.mono - macOS 15.1.0 - Multi-window, 1 monitor - Metal (Forward+) - integrated Apple M1 Pro (Apple7) - Apple M1 Pro (8 threads)

Issue description

Solution proposed on this issue #99155 doesn't work with Object Physics Picking functionality, need a way to implement both Physics Picking and Drag & Drop functionality together... Seems to have started here: #67531 According to @Sauermann...

Steps to reproduce

image

If the FullScreenControl Mouse Filter is set to Pass (Propagate Up), Physics Picking stops working; If I set it to Ignore, then the Physics Picking starts working again (but drag & drop functionality depends on it)...

With FullScreenControl Mouse Filter set to Ignore If I disable the Object Picking option in the Viewport the UnhandledInput event starts printing (when the mouse is inside the application window - outside it prints for both cases...); This happens only when disabling the Object Picking, so even if there is not collisions (you can remove the ItemExample) it still disables UnhandledInput from FullScreenControl to work inside the application window;

In other words, there seems to be no way to make Physics Picking and Drag & Drop (which requires fullscreen Control) for the same viewport, which in IMO is a VERY big deal;

minimalproject.zip

@conde2
Copy link
Author

conde2 commented Nov 15, 2024

Tested and I can confirm this problem, there is no way to enable Physics Picking Object and Drag and Drop in the same viewport.

@Sauermann
Copy link
Contributor

Sauermann commented Nov 15, 2024

@jopheno thanks for the detailed reproduction steps for physics picking, which I would consider a separate bug than the issue of opening post. I have implemented an alternative approach to solve both problems. It is available in #99270.

As soon as I enable the Object Picking in the Viewport it no longer prints the UnhandledInput event execution, which means that the documentation is not being respected what so ever (as it says it is the last thing considered, but it is impacting the early in the input cascade inputs)... Is it just a problem of documentation?

The order of execution for input events is guaranteed within a Viewport, but not globally.

What happens here is the following:

  1. During gui-input in the main viewport, the mouse input event is sent from the SubViewportContainer to the SubViewport.
  2. Inside the SubViewport, physics picking is enabled and the input event is stored for physics picking.
  3. Since physics picking happens asynchronously later during the pyhsics-evaluation-step, it is unknown, if that event will be used, so for the moment it has to be assumed, that it will be used later, so the event is set as handled.
  4. Event evaulation goes back from the SubViewport to the main Viewport.
  5. Since the event is already set as handled during gui-input, UnhandledInput is no longer triggered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment