fix: button release on blur#1101
Conversation
imaginarny
left a comment
There was a problem hiding this comment.
This was on todo so it would be great help again as well. 🙂
Will test some more once its updated and could merge it before #1106 so you have less work merging conflicts. Plus, I have one another in progress that could cause troubles as well, so would make sense to merge this one first.
| releaseHeldInputs(); | ||
| queueReleaseHeldInputs(); |
There was a problem hiding this comment.
What is the reason releaseHeldInputs() is called twice? Once right away then again in input event queue. Wouldn't it be enough with the queued one? 🤔
There was a problem hiding this comment.
That's why I wanted you to take a look 😅 but from what I gathered keeping only the queue would leave held inputs stale until the next input tick. And in the case of visibilitychange to hidden that tick may not run until the tab is visible again. So the synchronous one triggers before the hide event and makes sure it's event handlers don't see stale state.
I added a comment in the code to try to explain it better.
But I would love your input on it.
There was a problem hiding this comment.
There are 2 scenarios happening:
- Input was not held down yet, it was pressed at the same time when blur/hide happens
- When you call release immediately, the input events weren't processed yet so you try to release before they are registered and then they won't release and stay pressed. This can happen in the same frame. So here it's not like the queued one catches the rest, the first one never releases any.
- Input was held down already when blur/hide happens
- When you call release immediately, it releases "mid" frame. If it was some different use case, it could cause issues, that's why all inputs are processed in input event I think, like e.g. you could trigger input rightfully and clear it before some other api would detect it was pressed. 🤔 But doesn't matter much here anyway, I wouldn't call it stale state since by design it clears in the beginning of the frame, but would have to verify the timing of the rest of input apis.
So it runs 4x on hide, 3x on win blur, 2x on canvas blur only. Hide and complete win blur don't matter and even if you used html UI outside of canvas, one intermediate array that gathers all inputs is negligible. Also no need to distinguish between those scenarios, not even reliably possible/would be heavier to. Just to include some insight. I think in this case it's fine then. Would like @lajbel to take a look if there was something overlooked.
There was a problem hiding this comment.
Sounds good, but let's wait for lajbel to chime in.
107a45d to
62d75eb
Compare
Relates to #1094
I would like you folks to take a look before I convert it to a "real" pull request.
I added "release all" buttons for keyboard and mouse. These events can "drop" when focus leaves the canvas. On the other hand gamepad is polling for events so I made sure to skip those (if there are any).