Base input fix #1636
Base input fix #1636
Conversation
viambot
added
safe to test
| onMounted(() => { | ||
| initStreamState(); | ||
| window.addEventListener('visibilitychange', handleVisibilityChange); |
There was a problem hiding this comment.
Might as well stop anything that is going on if someone navigates away from the window. Just some extra fallbacks.
viambot
added
safe to test
|
I have a few worries about this approach: what we're implementing here should have similar architecture to a networked multiplayer game, and it's not recommended practice there to have an across-the-stack event loop where digesting a stack of user inputs is network-bound. With this solution I worry that we'll often start seeing perceptible out of sync commands. I'd recommend doing what is often done with multiplayer games: we send the client's current state (with regard to which input is pressed) at a 16.6ms interval without waiting for responses and let the robot figure out what to do with the given information. I remember briefly discussing something like this idea with Eric and Matt, and it being mentioned that this above approach might mess with people who are also scripting their robots while controlling? Regardless, I'm not sure how we give a good long term UX without implementing some form of the solution I just mentioned. Would love to hear thoughts or counterpoints. |
I see what you mean but I am not sure how we would implement that without some sort of service on the robot that receives those client calls on the robot side. As it stands the RC is basically acting as that service. If we are worried about out of sync commands then we need to cancel and stop whatever current digestion is happen and then start a new process whenever a new input is pressed. That would mean if you change directions the robot would have a brief stop between changing directions. |
viambot
added
safe to test
|
@edaniels @micheal-parks @stevebriskin Ok so tried to simplify this a bit. All we do now is just check if there are no pressed keys after a |
viambot
added
safe to test
viambot
added
safe to test
|
@edaniels @AdamMagaluk Seeing differences in |
viambot
added
safe to test
|
Note: merge base coverage results not available, comparing against closest 75aa7fd~1=(d29588c) instead
|
This PR updates how we process base input events to make sure they are processed in order and to avoid race conditions. Instead of immediately triggering events, we add them to a queue and resolve them one-by-one. The
stopanddigestInputfunctions were refactored to return promises that did not resolve until the callback for their respectivegRPCcalls is invoked. This way if inputs are pressed rapidly, we make sure a set power event is resolved before calling the subsequent stop call.Here is my console log after spamming the
dkey:@micheal-parks @edaniels @stevebriskin If we know how to reproduce this issue it would be great to test out this PR with an actual bot. I am hoping this will be a better solution than having to set a timeout to wait for
stops.