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 68.14% 76.72% +8.57%
===========================================
Files 367 465 +98
Lines 53685 87993 +34308
===========================================
+ Hits 36584 67512 +30928
- Misses 17101 20481 +3380 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 99 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.
| } | ||
|
|
||
| [Test] | ||
| [Category("Actions")] |
There was a problem hiding this comment.
I still need to add a InputForUI specific test for this.
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 could you try looking at 6157498 first, since it has the least amount of changes? Let me know if it's better. |
|
|
||
| - Complexity: | ||
| - Halo Effect: | ||
| - Complexity: |
There was a problem hiding this comment.
Why is this PR modifying the template?
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.
| - Fixed the compilation warnings when used with Unity 6.4 (ISX-2349). | ||
| - Fixed an issue where `InputSystemUIInputModule.localMultiPlayerRoot` could not be set to `null` when using `MultiplayerEventSystem`. [ISXB-1610](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1610) | ||
| - Fixed an issue in `Keyboard` where the sub-script operator would return a `null` key control for the deprecated key `Key.IMESelected`. Now, an aliased `KeyControl`mapping to the IMESelected bit is returned for compability reasons. It is still strongly advised to not rely on this key since `IMESelected` bit isn't strictly a key and will be removed from the `Key` enumeration type in a future major revision. [ISXB-1541](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1541). | ||
| - An issue where a UITK MouseEvent was triggered when changing from Scene View to Game View in the Editor has been fixed. [ISXB-1671](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1671) |
There was a problem hiding this comment.
Nitpick. Double space before ISXB number.
| /// <param name="canFlushBuffer">Whether the buffer can be flushed</param> | ||
| /// <param name="updateType">The current update type</param> | ||
| /// <returns>True if we should exit early, false otherwise.</returns> | ||
| private bool ShouldExitEarlyFromEventProcessing(InputEventBuffer eventBuffer, bool canFlushBuffer, InputUpdateType updateType) |
There was a problem hiding this comment.
I would prefer that we do not pass canFlushBuffer to this function, nor eventBuffer. Those two is the obvious things to check in the outer scope. E.g.
var shouldExitEarly = eventBuffer.eventCount == 0 || canFlushBuffer || ShouldExitEarlyFromEventProcessing(updateType);
| 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?
| return false; | ||
| } | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
Thanks for documenting complex internal functions. I think its helpful.
| /// <summary> | ||
| /// Determines if an event should be discarded based on timing or focus state. | ||
| /// </summary> | ||
| /// <param name="eventType">The type of the current event</param> |
There was a problem hiding this comment.
Nitpick: I would advise against using the term "current" here. It is just an event being evaluated right?
| return false; | ||
| } | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
Generally this makes sense, but have you tested that the following scenario works.
Press a gamepad button in edit mode and keep it held. Enter play mode. Is the button still considered down?
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.
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.