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

Check if input marked handled before processing additional CollisionObjects #48800

Merged
merged 1 commit into from Sep 26, 2023

Conversation

madmiraal
Copy link
Contributor

Currently, when processing picking, all CollisionObject2Ds under the mouse pointer are processed regardless of whether or not set_input_as_handled() was called. This PR checks whether set_input_as_handled() has been called before processing additional CollisionObject2Ds.

Fixes #48788

@madmiraal madmiraal added bug topic:input cherrypick:3.x Considered for cherry-picking into a future 3.x release labels May 17, 2021
@madmiraal madmiraal added this to the 4.0 milestone May 17, 2021
@madmiraal madmiraal requested a review from a team as a code owner May 17, 2021 18:26
@jeremyz
Copy link
Contributor

jeremyz commented May 26, 2021

you can't do this as Area2d's processing order is not determined see #23051

@madmiraal
Copy link
Contributor Author

@jeremyz This change doesn't affect any potential fix to #23051. The order in which they are processed doesn't change the need to not do further processing if one of them marks the event as handled.

@jeremyz
Copy link
Contributor

jeremyz commented May 26, 2021

@madmiraal sure, but this will change the status of a call to set_input_as_handled() from ignored to unreliable.
Without a predetermined processing order, it just adds confusion. (my thoughts).

@ajreckof
Copy link
Contributor

I think personally that this is still the right fix for this bug. The bug is that the call to set_input_as_handled() is ignored. The fact that it is randomly taken is it's own other bug which need an other fix i don't see any reason as to why not get this fix put right as if you don't want to be handled randomly you can just not set the input as handled.

@Sauermann
Copy link
Contributor

This approach is going to introduce a bug, because Physics Picking can happen in a later frame than the frame in which the event was created.

Here is a MRP, that shows this problem: BugPhysicsSetHandled.zip

extends Area2D

func _unhandled_input(event):
	if (randi_range(0, 1) == 0):
		get_viewport().set_input_as_handled()

func _on_input_event(viewport, event, shape_idx):
	if (get_viewport().is_input_handled()):
		print("ERROR")

The following happens:
Frame 1:

  • no call to set_input_as_handled() happens
  • The event is stored for asynchronous physics picking

Frame 2:

  • set_input_as_handled() is called during _unhandled_input()
  • The physics-picking event of Frame 1 is now processed and gets the information, that the event has been handled, even though it wasn't set to handled in Frame 1.

This PR would require, that during the evaluation of physics picking the event-handled-status of a previous frame would need to be available, but it isn't.

@madmiraal
Copy link
Contributor Author

@Sauermann The bug you describe is not introduced by this PR. It happens with beta 17 too. Therefore, it is a pre-existing bug that needs it's own issue.

@Sauermann
Copy link
Contributor

Because of the described behavior, this PR will introduce the problem, that sometimes InputEvents will be discarded for Physics-Picking, while they are expected to be used for Physics-Picking. So this PR depends on the described behavior to be changed.

@madmiraal
Copy link
Contributor Author

As per the documentation, physics _input_event() is called after _unhandled_input(); so marking an event as handled in _unhandled_input() should prevent it being processed by _input_event().

@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 16, 2023
@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jun 19, 2023
@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 19, 2023
Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

Approved in PR-review meeting

@akien-mga
Copy link
Member

Could you rebase for good measure? There doesn't seem to be conflicts but since the commit is now 2 years old, it's probably good to make sure it still passes CI and tests.

@Sauermann
Copy link
Contributor

The problem, that I wrote about in my previous message #48800 (comment) has been solved in #79546.

@akien-mga akien-mga merged commit 28b1678 into godotengine:master Sep 26, 2023
15 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
Development

Successfully merging this pull request may close these issues.

input_event method of CollisionObject2D ignore set_input_as_handled()
6 participants