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

Allow multiple SubViewports within a Window to have a focused Control #79480

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

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Jul 14, 2023

This PR lifts the restriction, that only a single SubViewport within a Window can have a focused Control-node (focus-stealing via Viewport::_gui_remove_focus_for_window).
Now every Viewport tracks its own focused Control, allowing multiple focused Controls within a single Window.

This change makes it necessary to introduce some kind of InputEvent-routing, to ensure, that the event reaches the correct focused Control first. This is done in the following way:
Whenever a Control receives focus, automatically all containing SubViewportContainer-Nodes also receive focus.
With the priorized InputEvent-propagation from #79248, this ensures, that the focused Control receives the InputEvent first.

MRP to showcase the functionality: FocusGroupTestSVC.zip (it achieves the same as the demo-project in #62421 (comment))

Behavior-change: This PR changes a core-functionality (method for focusing Control-nodes)
Since there are so many different use-cases, unfortunately i cannot rule out the possibility of regressions.

Updated 2023-08-04: Adhere to DEV_ASSERT conventions and remove DEV_ASSERT. Added Code-comments
Updated 2023-08-22: Fix merge conflict
Updated 2024-01-19: Fix merge conflict

@TheFoxKnocks
Copy link

Is there any chance you can provide a modified version of your example project that'd allow for simultaneous input? Such as navigating the menu on the left with arrow keys and perhaps the menu on the right with WASD? I was poking around your code as is and this looks pretty interesting, but I'm not entirely sure how that would work, or if it's even in the scope of what you're doing here.

Not asking for huge elaborate tutorials, just wanting to see what the implementation of this would look like with your system. If you don't mind, of course.

@Sauermann
Copy link
Contributor Author

@TheFoxKnocks Can't dive into this deeper at the moment, but from a quick glance, for what you want to achieve, you need event-routing to different SubViewports. Please have a look at #78762 for that.

@TheFoxKnocks
Copy link

@TheFoxKnocks Can't dive into this deeper at the moment, but from a quick glance, for what you want to achieve, you need event-routing to different SubViewports. Please have a look at #78762 for that.

Thanks for letting me know. I'm down to check that out, but it's saying the artifacts have expired, so I'm not able to download it in this manner. I'm not sure if this is something you need to do on your end or if it's mine, but I wanted to pass it on just in case.

@Sauermann Sauermann marked this pull request as ready for review July 27, 2023 14:43
@Sauermann Sauermann requested a review from a team as a code owner July 27, 2023 14:43
@Sauermann
Copy link
Contributor Author

Sauermann commented Jul 27, 2023

@TheFoxKnocks unfortunately it is difficult to provide a downloadable version containing both PRs, so that you can test the combination of them (besides compiling it yourself).
So the best I can provide at the moment is a GDscript-demo, of how #79480 and #78762 would work together and a video of how it would look like:

Demo: FocusGroupTestRouting.zip (updated: 2023-08-04)

KeyRouting.mp4

@TheFoxKnocks
Copy link

Ah, damn. I'm glad to know it's at least in the works, but I understand that combining the two would be a hassle and a half. Unfortunately in my case I really do need both, but I'm not about to make demands or be an ass about it. Thank you for the work you've done.

@djpeach
Copy link

djpeach commented Aug 2, 2023

From what I can understand, this will allow for splitscreen UI's in godot finally. I have been using a hacky manual workaround with custom buttons up until now. Super excited for this!

@djpeach
Copy link

djpeach commented Aug 2, 2023

@Sauermann tomorrow I am going to try to merge #79480 and #78762 together locally and and compiling, to try these features out together. If I understand correctly, I will want to create a UI with a couple svc's with subviewports and then register a device with each svc and display a menu in each subviewport. I will then want to override the new _send_input_event() method from #78762 to check the event device againt the svc's registered device. And together with the ability to have multiple focussed controls from #79480, I can use the native godot focus behavioiur and button presses on a splitscreen menu with two local players on controllers!

I think this will work just fine since I am only using controllers, but this would not work directly if there were also a local player with kbm, as they and the first controller player would both have device id 0. I really do think godot should make those unique, as right now the only way I can think to support that would be to use the device type somehow if possible to create your own device identifier and match on that..

@djpeach
Copy link

djpeach commented Aug 2, 2023

@Sauermann Amazing! My first time doing something like this, so took a bit, but I cloned your version and merged the input event filtering branch and this one, and compiled locally. I then took your demo for @TheFoxKnocks and modified it for multiple local devices and it works like a dream with just this simple little script:

(You simply assign each SVC a device)

func _send_input_event(event: InputEvent):
	if not event is InputEventJoypadButton:
		return true
	return event.device == assigned_device

I removed it for the demo project to simplify, but I had this setup with the SVCs saved as a scene and dynamically created them and assigned a device as devices connected and disconnected!

Here is the demo project:
LocalSplitScreenMenu.zip

Awesome work on this. Going to use my local build for now, but really hope this gets merged into Godot stable release soon! This intuitive approach to local splitscreen UI is something Godot has been sorely missing for a while. I'll try to build a more complex realistic UI and I'll report back if I find any bugs.

@Marenz
Copy link

Marenz commented Dec 29, 2023

I think this will work just fine since I am only using controllers, but this would not work directly if there were also a local player with kbm, as they and the first controller player would both have device id 0. I really do think godot should make those unique, as right now the only way I can think to support that would be to use the device type somehow if possible to create your own device identifier and match on that..

Yep, that's exactly what I did after I discovered that they both have device id 0

I want to encourage people who want this to vote for godotengine/godot-proposals#7161 as well.

Here is a windows build containing both PRs (the first one has been merged at this point) rebased on top of current master:

https://github.com/Marenz/godot/blob/pr/79480/bin/4.3-based.zip

Note for new readers: The _send_input_event function that @djpeach talks about has since been renamed to _propagate_input_event (I was trying to find documentation on the function and got confused as to where he got it from)

Let's test this so it can be merged soon :)

I've been using the built above for my project and found now problems so far.

@AThousandShips
Copy link
Member

Please edit your previous comments instead of making several in short time when no one else is engaging, it just unnecessarily pings people 🙂

This PR lifts the restriction, that only a Single `SubViewport`
within a `Window` can have a focused `Control`-node.
Whenever a `Control` receives focus, automatically all containing
`SubViewportContainer` also receive focus.
@djpeach
Copy link

djpeach commented Mar 14, 2024

Sad to see no movement on this or #62421 :(

@reduz
Copy link
Member

reduz commented Mar 19, 2024

@Sauermann this makes sense, but I am not understanding, should this not be optional?

@Sauermann
Copy link
Contributor Author

@reduz I'm open to the idea of making this optional. Currently I see three potential ways to make this configurable:

  1. A Window.allow_multiple_subviewports_with_focused_control_in_this_window setting
  2. A SubViewport.grab_focus_from_other_subviewports_in_same_window setting
  3. A SubViewport.allow_focus_grab_from_other_subviewports_in_same_window setting

Without having investigated this in more detail, I would probably go with Nr 3, because of its flexibility or with Nr 1, because it is simpler than the other variants.

Do you have a preference?

@reduz
Copy link
Member

reduz commented Mar 20, 2024

@Sauermann I was thinking about something kind of the reverse, more explicit to the user on how its supposed to work, like a setting allowing it to have its own focus.

@Sauermann
Copy link
Contributor Author

@reduz
Do I understand you correctly, that you are thinking along the idea of: Viewport.force_independend_focus (defaults to false)

with a description of:

By default, when a Control grabs focus, all other Control lose focus, that are in the same Window, even if they are in other Viewports.
If true, the focused Control of the Viewport keeps its focused status, when a different Control in the same Window (possibly in a different Viewport) grabs focus.

@reduz
Copy link
Member

reduz commented Mar 20, 2024

yeah, as I mentioned, I would not like to break compatibility on this as it has worked like this for a decade and users sort of got used to it. I think an option to have independent focus (maybe use_own_focus = false as default) may work.

Another part of this that I remember we discussed with @Faless is that even with independent focus, you may want to eventually support filtering events so say, one gamepad events go to an viewport, and another gamepad events go to another, though it shouds like if this is ever implemented it should be a separate PR.

@Sauermann
Copy link
Contributor Author

@reduz An initial attempt to implement your suggestion is available in #89772.
After testing this simple implementation, it looks like this will introduce several edge cases, that need to be addressed.

you may want to eventually support filtering events so say, one gamepad events go to an viewport, and another gamepad events go to another

Filtering input events for input functions _input, ..., _unhandled_input at the SubViewportContainer level was introduced in #78762.

@Sauermann
Copy link
Contributor Author

@reduz
Unfortunately the encountered edge cases became so complex, that the implementation for this functionality became very bloated.

I have updated #89772 with a simpler second approach that introduces a Window option, if all its SubViewports should keep their own focus or if all its SubViewports can have only a single focused Control.

This second approach is not as flexible, as giving each SubViewport its own configuration option, but I believe, that it is sufficient for the goal of improving splitscreen handling.

I believe, that the better user-visibility of the first approach is not good enough to justify the increased implementation complexity.

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