FIX: MouseDownEvent is triggered when changing from Scene view to Game view (ISXB-1671) #2234
Conversation
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2234 +/- ##
===========================================
+ Coverage 76.70% 76.83% +0.12%
===========================================
Files 465 476 +11
Lines 87919 88828 +909
===========================================
+ Hits 67442 68247 +805
- Misses 20477 20581 +104 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 24 files with indirect coverage changes 🚀 New features to boost your workflow:
|
jfreire-unity
force-pushed
the
isxb-1671-outoffocus-events-trigger-actions
branch
2 times, most recently
from
dc01440 to
f21ad55
Compare
jfreire-unity
force-pushed
the
isxb-1671-outoffocus-events-trigger-actions
branch
from
f21ad55 to
6157498
Compare
It's tested against all background behavior settings.
The goal is here is to make those conditions self contained in "chunks" so that we can slightly reduce the cognitive load when reading through the OnUpdate() function. This is a personal preference and others in the team might disagree with these changes.
ritamerkl
left a comment
ritamerkl
left a comment
There was a problem hiding this comment.
I am hesitant introducing such a big change in the InputManager, also looking at it I wonder:
Isn't the wrong event the mousePosition sent from DeviceSync? Since the event before should not cause the MouseDown, but the second event (caused by the DeviceSync). Or did I get that wrong?
The wrong event is the one before the DeviceSync. That's why we should discard it and not process it.
I don't see it as a big change but maybe the small refactoring is "polluting" the PR in that sense? If you look at 6157498 there are a bit smaller changes and I just factored out some methods. |
ekcoh
left a comment
ekcoh
left a comment
There was a problem hiding this comment.
It's a bit difficult at the moment to judge what is refactoring and what is bug fixing / change in behavior on this PR. I will come back with additional comments once I have realised this.
@ekcoh could you try looking at 6157498 first, since it has the least amount of changes? Let me know if it's better. |
ekcoh
left a comment
ekcoh
left a comment
There was a problem hiding this comment.
Commenting for now since the PR states it will add more tests and I added some suggestions. Generally I believe this is an improvement and happy to see some refactoring happening on InputManager. Regarding the refactor I would just like to make sure we have test coverage on the conditions before and after to avoid regressing something. It looked quite similar though and I couldn't spot any mistake there.
My other concern is regarding whether the focus change callback is direct or indirect. If direct it may lead to out-of-order processing of focus change and events which is why I have mentioned previously that we should have our focus changes in the event queue to make it trivial to reason about. If indirect we have a problem since we do not have the focus events true timestamp. Getting a timestamp when the callback is invoked doesn't need to imply that its the true timestamp of the event on the event timeline.
| var mousePointAction = new InputAction(binding: "<Mouse>/position", type: InputActionType.PassThrough); | ||
| mousePointAction.Enable(); | ||
|
|
||
| using (var trace = new InputActionTrace(mousePointAction)) |
There was a problem hiding this comment.
I would recommend letting the time step parameter also be an input parameter to the test case. It should still be valid for timeStep = 0.0f, since our logic need to handle event order as the determining factor when time isn't sufficient to tell them apart.
There was a problem hiding this comment.
So I would recommend adding that test as well.
There was a problem hiding this comment.
In that case, this would not work. Time needs to move; otherwise, we can't know what events have occurred before and after the focus. This solution relies on event timestamps. Similar to what we do for transitions between Edit Mode and Entering Play Mode, see the conditions in ShouldDiscardEditModeTransitionEvent(). Let me know if I'm missing something.
In theory, if we had focus events in the queues, we wouldn't need to know about timestamps. However, as we discussed, there is a higher risk in introducing such a change. We should still conduct an exploration of the implementation, but I would prefer to land first a less risky solution for this particular bug.
I'm pappy to prototype a solution together if you like, after this fix. We can sync on Slack.
There was a problem hiding this comment.
I think it's better to use timestamps than what we had before and in practise it's likely not a problem. Still safe to land as is though and guess we need it in the queue to have a strictly accurate implementation.
There was a problem hiding this comment.
Maybe we can also add an inline comment to this test case that with a strictly defined event queue containing all relevant inputs without downsampling the test case should be expanded to be time-agnostic for a fixed timestamp?
| private bool m_NativeBeforeUpdateHooked; | ||
| private bool m_HaveDevicesWithStateCallbackReceivers; | ||
| private bool m_HasFocus; | ||
| private bool m_DiscardOutOfFocusEvents; |
There was a problem hiding this comment.
A bit of a corner case, but what happens with these if there is a domain reload while in playmode? Will they be reinitialised to the same values they had before the domain reload was initiated?
There was a problem hiding this comment.
Hm, good question. How can I trigger a domain reload in play mode so I can test that?
There was a problem hiding this comment.
You can either:
- Change a script manually, that will trigger a domain reload
- Make a script hat calls into EditorUtility.RequestScriptReload
There was a problem hiding this comment.
They are initialized to their default values (false for the boolean, 0.0 to the timestamp).
I'm not seeing a reason why they should have their same values as before the domain reload. The timestamp will always be "old". But maybe I'm missing some detail or a pattern.
|
@jfreire-unity Please update this branch with changes from develop to get critical CI fix from #2260. I do not dare updating it for you since the branch has conflicts. |
jfreire-unity
force-pushed
the
isxb-1671-outoffocus-events-trigger-actions
branch
from
05cf85f to
6fb0bb3
Compare
Pauliusd01
left a comment
Pauliusd01
left a comment
There was a problem hiding this comment.
Issue still occurs for me if I switch the resolution from free aspect to one of the predefined ones.
https://github.com/user-attachments/assets/9a530010-e9cb-432c-9c9a-b047a3a0b4e6
I can't access that image |
|
Just chiming in to bring back a point that was mentioned in our early discussions about this issue. In an ideal world, we'd have to be able to click inside the GameView directly and have the first click that happened before we changed focus be processed as though it had been a normal click after focus. Most software allow that, where you change your window focus and at the same time you click a button from the other window. In that case, if the coordinates of the click (as expressed in the new focused window) are valid, we should accept the click. I don't know how computing new coordinates after the fact is feasible in general though. |
Could you test the same on develop but by adding a I believe the issue you are reporting happens regardless of using InputForUI, so it wouldn't be a regression. |
Here's what it's looking to me: With the event system object:
Without the event system object (default user project)
So It's still an improvement and the custom resolution thing is not a new regression, if you want it to be a separate bug that sounds fine to me. Only thing though is I'm seeing a lot of CI failures |
Thanks for testing! Yes, I would raise it as different bug since it is something already happenin with Event System + UIInputModule and will likely require trunk changes. It is not a focus change issue. I'm looking into the CI issues and will ping reviewers once those are fixed |
Doesn't make sense to run them in the Player as they are editor specific.
ekcoh
left a comment
ekcoh
left a comment
There was a problem hiding this comment.
Thanks for doing this improvement/fix. I will approve now since I believe this makes it more correct than existing solution.
| var mousePointAction = new InputAction(binding: "<Mouse>/position", type: InputActionType.PassThrough); | ||
| mousePointAction.Enable(); | ||
|
|
||
| using (var trace = new InputActionTrace(mousePointAction)) |
There was a problem hiding this comment.
I think it's better to use timestamps than what we had before and in practise it's likely not a problem. Still safe to land as is though and guess we need it in the queue to have a strictly accurate implementation.
| var mousePointAction = new InputAction(binding: "<Mouse>/position", type: InputActionType.PassThrough); | ||
| mousePointAction.Enable(); | ||
|
|
||
| using (var trace = new InputActionTrace(mousePointAction)) |
There was a problem hiding this comment.
Maybe we can also add an inline comment to this test case that with a strictly defined event queue containing all relevant inputs without downsampling the test case should be expanded to be time-agnostic for a fixed timestamp?
| private bool m_NativeBeforeUpdateHooked; | ||
| private bool m_HaveDevicesWithStateCallbackReceivers; | ||
| private bool m_HasFocus; | ||
| private bool m_DiscardOutOfFocusEvents; |
Description
Proposed solution to fix ISXB-1671.
The bug reports a UITK event that shouldn't be triggered. This ocurrs due to a
InputSystemProviderUI Click action being performed once the Editor is in play-mode and we select Game view tab when previously in Scene View.The action that is triggered provides the wrong mouse position. Essentially, an action is being performed for the mouse click/position that happened while out of focus in the Editor and triggered the focus regain. However, when
InputManager.OnUpdate()is called, it assumes the game already is in focus, so there was no way to know if an event was triggered out of focus or not.The proposed solution is to filter and discard the events that happened immediately before focus was regained, by checking if they were received before the focus regain. In the diagram below, essentially we are discarding the "ofending" event - in red - that triggered an Input Action, which would then dispatch a UITK event through
InputSystemProvider.Unrelated changes to bug-fix
I also did a small refactoring to put some conditions in their own functions so that can be easier to read through
InputManager.OnUpdate. This is just a personal preference as it has been historically complicated to reason about all the conditions we have. This is still not perfect, but I hope it's a small improvement for others as well.If you want, you can have a look of the commit which contains the fix changes, 6157498. This doesn't have any refactoring yet and is easier to reason about.
PR template update
Removes a section that we no longer need to take into account.
Testing status & QA
Please describe the testing already done by you and what testing you request/recommend QA to execute. If you used or created any testing project please link them here too for QA.
Overall Product Risks
Comments to reviewers
Verify that the original bug doesn't happen anymore, both on Win and macOS.
Do some other tests with changing Background settings, to make sure nothing has regressed. We can likely use on of the Mouse Visualisers for it.
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.