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

Deprecate Viewport::handle_input_locally #77926

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

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Jun 6, 2023

Deprecate Viewport::handle_input_locally in favor of:

  • an automatism in SubViewportContainer, that propagates the event handled state to the parent viewport
  • a parameter in SubViewportContainer, that decides, if input events should be sent to additional child SubViewports, when the event is set to handled in one of the SubViewports.

The use-case, that SubViewport.handle_input_locally = true achieves, is to allow a single input event to be handled in multiple SubViewports of a SubViewportContainer. However, this functionality is very inaccessible.
This PR implements a different method to allow this functionality by adding SubViewportContainer.send_input_to_all_viewports.

This change solves the problem of the automatic change of SubViewport.handle_input_locally (resolve #56502 (#46982))
By doing this, it makes the mentioned functionality more reliable and accessible (resolve #89073)

revert #77730 and make sure, that #76439 is not reintroduced
revert #87385 and make sure, that #84258 is not reintroduced
takes ideas of #46994

Updated 2023-09-02: Changes to documentation.
Updated 2024-01-24: Fix merge conflict with #77730
Updated 2024-01-25: Fix merge conflict with #87385
Updated 2024-03-07: Rebase after pre-commit-hook changes, Implement a replacement for the handle_input_locally use-case.
Updated 2024-09-15: Fix merge conflict

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master 329652b), it works. With this PR, I get the following output when running the MRP from #76439:

InputEventMouseButton: button_index=0, mods=none, pressed=false, canceled=false, position=((0, 0)), button_mask=0, double_click=false
InputEventMouseButton: button_index=0, mods=none, pressed=true, canceled=false, position=((0, 0)), button_mask=0, double_click=false
InputEventMouseButton: button_index=0, mods=none, pressed=false, canceled=false, position=((0, 0)), button_mask=0, double_click=false
InputEventKey: keycode=(Unset), mods=none, physical=false, pressed=false, echo=false

That said, since the existing behavior is removed entirely (the property no longer does anything), this is a compatibility-breaking change. I would prefer keeping the existing behavior available if at all possible. If this can't be done, this is not necessarily a blocker, but it needs further discussion on communicating this change to users.

@Sauermann
Copy link
Contributor Author

Sauermann commented Jun 21, 2023

If you want to keep the existing behavior of handle_input_locally, then #77730 is the way to go.

However I believe, that this PR is the better alternative, because:

I did ask on RC, what the intention of handle_input_locally is, but unfortunately I didn't receive an answer to that question. So it took me a while to identify one bug, that would get introduced by removing handle_input_locally.
This bug is, that when an event gets accepted in a SubViewport, then the accepted state is not propagated to the parent viewport, which would allow a single event to get accepted multiple times.
The addition in _send_event_to_viewports in this PR prevents this bug.
So even though this PR removes the functionality of handle_input_locally, the underlying behavior stays the same.

As #56502 shows, the current behavior of handle_input_locally is confusing for users. This confusion is not solved by #77730.
The current description of handle_input_locally is very abstract and it is difficult to identify its intention. This PR solves this by removing its functionality.

handle_input_locally solves an engine-internal problem (propagation of event-handled-state from SubViewport to parent viewport) and I believe, that it should not have been exposed to gdscript in the first place.

This PR is in accordance to the "Prefer local solutions"-best practice by replacing a core-solution with a peripheral-solution, while #77730 extends the complexity of the current core-solution. A bonus is, that the peripheral-solution of this PR requires much less code, than the current core-solution.

@Jummit
Copy link
Contributor

Jummit commented Jun 21, 2023

I just wanted to chime in since I opened #77730:

I agree with Sauermann that the API is confusing and that this PR is a more sustainable solution. I doubt that many users are consciously using the handle_input_locally flag, I found it hard to understand and reason about.

@Sauermann Sauermann changed the title Propagate input handled state from SubViewport automatically Deprecate Viewport::handle_input_locally Jun 21, 2023
@akien-mga
Copy link
Member

Moving to 4.2 as this doesn't seem urgent, and might be a bit rushed to deprecate an API just before 4.1 RC 1.

Note that deprecating usually means keep the functionality working, but recommending not to use it (and recommending an alternative). This actually makes the API a no-op so it might as well remove it and break compatibility fully, if that's what we decide to do. Otherwise we could keep the local input handling functionality as deprecated and opt-in.

@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 23, 2023
@Sauermann Sauermann force-pushed the fix-handle-input-locally branch 2 times, most recently from 3d70b97 to 9788937 Compare September 2, 2023 09:49
@L2750558108
Copy link
Contributor

What confuses me is that the description of this API in the documentation is "when it's false, the current viewport doesn't mark this input as processed, but the parent viewport with this flag set to do so", but the viewport's code only sees this code (picture) for the handle_input_locally part of the inspection, it only checks the root viewport or window, and doesn't check the handle_input_locally of the parent viewport

image

@Sauermann
Copy link
Contributor Author

@L2750558108 I have noticed this inconsistency as well, but since this is such a negligible behavior, that apparently no user has noticed yet, my intention was to remove the whole thing instead of consolidating the code with the documentation.
Looks like a rebase is needed - will take care of this.

@L2750558108
Copy link
Contributor

L2750558108 commented Sep 13, 2024

@L2750558108 I have noticed this inconsistency as well, but since this is such a negligible behavior, that apparently no user has noticed yet, my intention was to remove the whole thing instead of consolidating the code with the documentation.
Looks like a rebase is needed - will take care of this.

I agree. This feature is unknown-significance. Wholely remove it maybe better.

Deprecate `Viewport::handle_input_locally` in favor of
- an automatism in `SubViewportContainer`, that propagates the event
handled state to the parent viewport
- a parameter in `SubViewportContainer`, that decides, if input events
should be sent to additional child SubViewports, when the event is set to
handled in one of the SubViewports.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants