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 InputEvent-filtering in SubViewportContainer #78762

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Jun 27, 2023

Introduce an user overridable function _propagate_input_event, that allows filtering, if an InputEvent should be sent to SubViewport children.

implements #78759 (comment)
alternative to #78759
implements a more generic approach than the one in godotengine/godot-proposals#1249
related to #62421

MRP to showcase the new functionality: SvcInputEventFilter78762.zip

Updated 2023-08-03: rename _send_input_event to _propagate_input_event in order to adhere to the usual nomenclature. improve docstring. Rebase for CI-changes.

@Sauermann Sauermann force-pushed the fix-svc-event-filter branch from 633107b to 40a02c0 Compare June 28, 2023 05:10
@Sauermann
Copy link
Contributor Author

I have a small nitpick with this PR, namely, that the wording _block_input_event is negated.
I have updated the PR to use positive wording _send_input_event instead.

@Sauermann Sauermann changed the title Enable filtering InputEvent-blocking in SubViewportContainer Enable InputEvent-filtering in SubViewportContainer Jul 6, 2023
@YuriSizov YuriSizov requested a review from a team July 7, 2023 18:59
@Sauermann Sauermann force-pushed the fix-svc-event-filter branch 2 times, most recently from 4ffcd1b to 3c5a6d9 Compare August 3, 2023 22:39
@djpeach
Copy link

djpeach commented Aug 7, 2023

I wonder if _propogate_input would be more aligned with the current _input, _unhandled_input methods. The _event on this one makes me think it is more specific than those, but it is not

@Sauermann
Copy link
Contributor Author

I'm not entirely convinced, that these need to be aligned, because _Input, _unhandled_input and the other functions have a different intention (handle the event for this node) than _propagate_input_event (return a flag that indicates, if the event should be sent to a different node) does.
But I am happy to find the best possible name for that function.

@akien-mga akien-mga requested review from RandomShaper and a team August 17, 2023 09:54
@Sauermann Sauermann force-pushed the fix-svc-event-filter branch 2 times, most recently from 625d80b to a0e23d3 Compare August 22, 2023 07:44
scene/scene_string_names.h Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Oct 1, 2023

While this PR does close the mentioned proposal, there are multiple problems that the proposal's author did not cover.
This PR allows to discern between 2 joypad devices and between joypad and keyboard. It has a some caveats:

  • to use it effectively like suggested in the proposal, all players need to share the same actions
  • you can code your input only inside _unhandled_input()

This means that:

  • you can't rebind controls per controller
  • you can't have 2 players using keyboards (e.g. WSAD and Arrows)
  • you can't use Input methods like get_vector() (unless there is a way to use them in _unhandled_input()?)
  • you can only use it with split-screen inside viewport containers. Single screen or split screen with e.g. sprites doesn't support this feature

That seems quite limited to me.

@Sauermann
Copy link
Contributor Author

Sauermann commented Oct 2, 2023

Thank you for your thorough review.

While this PR does close the mentioned proposal, there are multiple problems that the proposal's author did not cover. This PR allows to discern between 2 joypad devices and between joypad and keyboard. It has a some caveats:

* to use it effectively like suggested in the proposal, all players need to share the same actions

The original idea for this PR comes from #78759 (comment)
Even when multiple players sharing the same action, it is possible to distinguish the events within _propagate_input_event. I provide an implementation for handling the WASD vs Arrow case.

* you can code your input only inside `_unhandled_input()`

The event is propagated as usual to SubViewports, so it is possible to use any input-handling function on nodes within SubViewports and not just _unhandled_input.

This means that:

* you can't rebind controls per controller

The user has the option to include any code into the overwritten _propagate_input_event function. So I don't see any problem with including functionality for rebinding controls there. Though I'm not entirely sure we are looking at the same problem.

* you can't have 2 players using keyboards (e.g. WSAD and Arrows)

This use-case is covered by the PR. The following overwritten function would handle that case:

func _propagate_input_event(event: InputEvent):
	if not event is InputEventKey:
		return true
	var e: InputEventKey = event
	if self == $"/root/FocusTest/SubViewportContainerWASD":
		return e.keycode < 255 # letters
	else:
		return e.keycode >= 255 # arrows

It sends events, that are based on letters to one SubViewport, and events that are based on arrow keys to the other SubViewport.

* you can't use Input methods like `get_vector()` (unless there is a way to use them in `_unhandled_input()`?)

Thank you for pointing me to get_vector.
get_vector accesses a global state, so if different Controllers are mapped to the same Action, then they will interfere with each other. I am not sure, if I should expand the scope of this PR to make get_vector multi-controller aware, or if this should be rather done in a separate PR.
Also please see my previous argument regarding _unhandled_input.

* you can only use it with split-screen inside viewport containers. Single screen or split screen with e.g. sprites doesn't support this feature

I agree with this statement. This implementation is based on the concept of using SubViewports for handling events for different Players. It ties nicely with #79480 which allows each player to have a focused Control in their SubViewport.

That seems quite limited to me.

I hope, that I have addressed most of your concerns.

@Sauermann Sauermann force-pushed the fix-svc-event-filter branch from a0e23d3 to c31c085 Compare October 2, 2023 16:23
@KoBeWi
Copy link
Member

KoBeWi commented Oct 2, 2023

The event is propagated as usual to SubViewports, so it is possible to use any input-handling function on nodes within SubViewports and not just _unhandled_input.

Right, what I meant is that you can't use _process() and the global state input method. All _input_*() functions are pretty equivalent.

The user has the option to include any code into the overwritten _propagate_input_event function. So I don't see any problem with including functionality for rebinding controls there.

The whole idea behind the proposal was to use a single action set for every player. If you have multiple action sets, _propagate_input_event() is not needed for anything and it doesn't really make things easier.

But that's more problem with the proposal itself than the implementation. Not every game will have control rebinding, but as soon as you want to add one, you will run into godotengine/godot-proposals#3555 and the solution proposed here, while it's for a similar problem, it doesn't solve all issues. (also in that proposal I commented with a convenient way to solve pretty much every problem raised here).

This use-case is covered by the PR. The following overwritten function would handle that case:

It doesn't handle other control schemes like IKJL. Though nowadays they aren't used I think, as most people will just play with controllers.

I hope, that I have addressed most of your concerns.

As a person who always writes input code inside _process(), I wouldn't use the new functionality, as the _input() limitation makes it inconvenient. idk how most people write their code though, but Input methods exist for a reason.

Introduce an user overridable function, that allows filtering, if
an `InputEvent` should be sent to `SubViewport` children.
@Sauermann Sauermann force-pushed the fix-svc-event-filter branch from c31c085 to 781cecd Compare October 2, 2023 17:52
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Not sure about the feature itself, but the implementation looks fine.

@YeldhamDev
Copy link
Member

@KoBeWi If it adds any weight, my game needs this feature as well (which was possible in a different way in 3.*). It's a mini-game collection, and this would be used to dictated when said mini-games would start/stop listening to inputs.

@akien-mga akien-mga merged commit 9e8a93a into godotengine:master Oct 3, 2023
15 checks passed
@akien-mga
Copy link
Member

akien-mga commented Oct 3, 2023

Thanks!

Edit: lol, bad copy paste ;)

@Sauermann Sauermann deleted the fix-svc-event-filter branch October 3, 2023 19:41
@TisFoolish
Copy link

you can't rebind controls per controller

Wait, that seems pretty bad, especially for those of us that are more disability focused as rebinding controllers is one of the most basic of forms of accessibility. It'd suck for a disabled user to be penalized in a split screen game or the non-disabled user having to deal with the whatever esoteric setup the disabled user.

@Sauermann
Copy link
Contributor Author

Right, what I meant is that you can't use _process() and the global state input method.

As a person who always writes input code inside _process(), I wouldn't use the new functionality, as the _input() limitation makes it inconvenient. idk how most people write their code though, but Input methods exist for a reason.

Using Input in _process() is something, that is not addressed by this PR. It focuses solely on input event propagation.
I haven't yet found a solution, that is better that your solution in the other proposal.

you can't rebind controls per controller

Wait, that seems pretty bad

I don't see any restrictions for rebinding controls per controller while using the functionality of this PR. But as stated above, _process() is not covered by this PR.

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.

6 participants