-
Notifications
You must be signed in to change notification settings - Fork 329
FIX: MouseDownEvent is triggered when changing from Scene view to Game view (ISXB-1671) #2234
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
base: develop
Are you sure you want to change the base?
Changes from all commits
6157498
300896c
f78e947
f1ed615
9023b74
510730e
ac75141
6fb0bb3
8a5b40c
0eabdc2
4693865
82bfcc3
4009f9c
34925fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -641,6 +641,60 @@ public void Actions_DoNotGetTriggeredByEditorUpdates() | |
| } | ||
| } | ||
|
|
||
| [Test] | ||
| [Category("Actions")] | ||
jfreire-unity marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| [Description("Tests that that only the latest event after focus is regained is able to trigger the action." + | ||
| "Depends on background behavior. (ISXB-1671)")] | ||
| [TestCase(InputSettings.BackgroundBehavior.IgnoreFocus)] | ||
| [TestCase(InputSettings.BackgroundBehavior.ResetAndDisableNonBackgroundDevices)] | ||
| [TestCase(InputSettings.BackgroundBehavior.ResetAndDisableAllDevices)] | ||
| public void Actions_DoNotGetTriggeredByOutOfFocusEventInEditor(InputSettings.BackgroundBehavior backgroundBehavior) | ||
| { | ||
| InputSystem.settings.backgroundBehavior = backgroundBehavior; | ||
|
|
||
| var mouse = InputSystem.AddDevice<Mouse>(); | ||
| var mousePointAction = new InputAction(binding: "<Mouse>/position", type: InputActionType.PassThrough); | ||
| mousePointAction.Enable(); | ||
|
|
||
| using (var trace = new InputActionTrace(mousePointAction)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I would recommend adding that test as well.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| { | ||
| // Note: We currently test against timestamps otherwise the test fails. But, ideally, we wouldn't need to. | ||
| // If we ever reach a point of having all relevant input events in the queue (including focus events) we | ||
| // could just rely on order of event. Which means this test work for a fixed timestamp and it should | ||
| // changed accordingly. | ||
| currentTime += 1.0f; | ||
| runtime.PlayerFocusLost(); | ||
| currentTime += 1.0f; | ||
| // Queuing an event like it would be in the editor when the GameView is out of focus. | ||
| Set(mouse.position, new Vector2(0.234f, 0.345f) , queueEventOnly: true); | ||
| currentTime += 1.0f; | ||
| // Gaining focus like it would happen in the editor when the GameView regains focus. | ||
| runtime.PlayerFocusGained(); | ||
| currentTime += 1.0f; | ||
| // This emulates a device sync that happens when the player regains focus through an IOCTL command. | ||
| // That's why it also has it's time incremented. | ||
| Set(mouse.position, new Vector2(1.0f, 2.0f), queueEventOnly: true); | ||
| currentTime += 1.0f; | ||
| // This update should not trigger any ction as it's an editor update. | ||
| InputSystem.Update(InputUpdateType.Editor); | ||
| currentTime += 1.0f; | ||
|
|
||
| var actions = trace.ToArray(); | ||
| Assert.That(actions, Has.Length.EqualTo(0)); | ||
| // This update should trigger an action with regards to the event queued after focus was regained. | ||
| // The one queued while out of focus should have been ignored and we should expect only one action triggered. | ||
| // Unless background behavior is set to IgnoreFocus in which case both events should trigger the action. | ||
| InputSystem.Update(InputUpdateType.Dynamic); | ||
|
|
||
| actions = trace.ToArray(); | ||
| Assert.That(actions, Has.Length.EqualTo(backgroundBehavior == InputSettings.BackgroundBehavior.IgnoreFocus ? 2 : 1)); | ||
| Assert.That(actions[0].phase, Is.EqualTo(InputActionPhase.Performed)); | ||
| Vector2Control control = (Vector2Control)actions[0].control; | ||
| // Make sure the value is from the event after focus was regained. | ||
| Assert.That(control.value, Is.EqualTo(new Vector2(1.0f, 2.0f)).Using(Vector2EqualityComparer.Instance)); | ||
| } | ||
| } | ||
|
|
||
| [Test] | ||
| [Category("Actions")] | ||
| public void Actions_TimeoutsDoNotGetTriggeredInEditorUpdates() | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.