Skip to content
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

APP-538: Make base key and mouse events more robust #1528

Merged
merged 5 commits into from
Oct 25, 2022

Conversation

micheal-parks
Copy link
Member

@micheal-parks micheal-parks commented Oct 25, 2022

This PR refactors how we handle mouse and key events from the <KeyboardInput> component, making them behave more predictably, with fewer cases in which a stop signal will not be sent if the user is not interacting anymore. More details in comments.

@micheal-parks micheal-parks self-assigned this Oct 25, 2022
@micheal-parks micheal-parks changed the title Make base key and mouse events more robust APP-538: Make base key and mouse events more robust Oct 25, 2022
import type { ServiceError } from '../gen/proto/stream/v1/stream_pb_service.esm';

interface Props {
name: string;
resources: Resource[];
}

// eslint-disable-next-line no-shadow
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, for some reason ESLint thinks that we're shadowing another variable with const enums. That's weird, we'll need to investigate more.

@@ -45,16 +53,14 @@ const angle = ref(0);
const videoStreamStates = new Map<string, boolean>();
const selectCameras = ref('');

const pressed = new Set<Keys>();
Copy link
Member Author

@micheal-parks micheal-parks Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now add all pressed keys to a stack of unique items, and then digest those items from oldest to newest input. The key is deleted from the stack when a keyup / mouseup event occurs. This allows us to better handle multiple keys pressed at the same time by a keyboard, or even a touchscreen!

let linearValue = 0;
let angularValue = 0;

for (const item of pressed) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we loop through the stack of pressed keys, and apply weights for each key. We don't need to worry about values growing larger than 1, because all items in a Set can be guaranteed to be unique.

The result of this behavior is that pressing the left and right keys (or the up and down keys) will cancel each other out.

new commonApi.Vector3(),
displayError
);
const req = new baseApi.SetVelocityRequest();
Copy link
Member Author

@micheal-parks micheal-parks Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BaseControlHelper isn't providing much abstraction value anymore since we've componentized everything, so I've moved all methods into the component. Switching to protobuf-ts will simplify this even more.

/>
</div>
<KeyboardInput
@keydown="handleKeyDown"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've simplified the internals of <KeyboardInput> so that it's now just emitting keydown and keyup events with normalized input.


const handleKeyDown = (key: Keys) => {
pressed.add(key);
digestInput();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We re-digest input whenever a keydown happens, or a keyup happens and the length of pressed keys is still greater than zero.

@micheal-parks micheal-parks added the safe to test This pull request is marked safe to test from a trusted zone label Oct 25, 2022

const handlePointerDown = (key: Keys) => {
emitKeyDown(key);
window.addEventListener('pointerup', () => emitKeyUp(key), { once: true });
Copy link
Member Author

@micheal-parks micheal-parks Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specifically fixes one of the issues people are encountering: dragging a mouse away from a button and releasing the mouse button causes the robot to drive forever, off into the sunset, never to be seen again.

If we instead apply the keyup event to the window, the mouse can be anywhere and the input will still be canceled.

Copy link
Member

@mrloureed mrloureed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳 LGTM!

@viambot viambot removed the safe to test This pull request is marked safe to test from a trusted zone label Oct 25, 2022
@micheal-parks micheal-parks added the safe to test This pull request is marked safe to test from a trusted zone label Oct 25, 2022
@viambot viambot removed the safe to test This pull request is marked safe to test from a trusted zone label Oct 25, 2022
@micheal-parks micheal-parks added the safe to test This pull request is marked safe to test from a trusted zone label Oct 25, 2022
@github-actions
Copy link
Contributor

Code Coverage

Package Line Rate Health
go.viam.com/rdk/components/arm 59%
go.viam.com/rdk/components/arm/universalrobots 12%
go.viam.com/rdk/components/arm/xarm 2%
go.viam.com/rdk/components/arm/yahboom 7%
go.viam.com/rdk/components/audioinput 55%
go.viam.com/rdk/components/base 68%
go.viam.com/rdk/components/base/agilex 62%
go.viam.com/rdk/components/base/boat 41%
go.viam.com/rdk/components/base/wheeled 76%
go.viam.com/rdk/components/board 69%
go.viam.com/rdk/components/board/arduino 10%
go.viam.com/rdk/components/board/commonsysfs 47%
go.viam.com/rdk/components/board/fake 39%
go.viam.com/rdk/components/board/numato 19%
go.viam.com/rdk/components/board/pi 50%
go.viam.com/rdk/components/camera 66%
go.viam.com/rdk/components/camera/fake 67%
go.viam.com/rdk/components/camera/ffmpeg 72%
go.viam.com/rdk/components/camera/transformpipeline 81%
go.viam.com/rdk/components/camera/videosource 56%
go.viam.com/rdk/components/encoder/fake 77%
go.viam.com/rdk/components/gantry 68%
go.viam.com/rdk/components/gantry/multiaxis 84%
go.viam.com/rdk/components/gantry/oneaxis 86%
go.viam.com/rdk/components/generic 84%
go.viam.com/rdk/components/gripper 82%
go.viam.com/rdk/components/input 86%
go.viam.com/rdk/components/input/gpio 87%
go.viam.com/rdk/components/motor 82%
go.viam.com/rdk/components/motor/dmc4000 69%
go.viam.com/rdk/components/motor/fake 60%
go.viam.com/rdk/components/motor/gpio 65%
go.viam.com/rdk/components/motor/gpiostepper 59%
go.viam.com/rdk/components/motor/tmcstepper 66%
go.viam.com/rdk/components/movementsensor 67%
go.viam.com/rdk/components/movementsensor/cameramono 39%
go.viam.com/rdk/components/movementsensor/gpsnmea 37%
go.viam.com/rdk/components/movementsensor/gpsrtk 28%
go.viam.com/rdk/components/posetracker 88%
go.viam.com/rdk/components/sensor 88%
go.viam.com/rdk/components/sensor/ultrasonic 31%
go.viam.com/rdk/components/servo 77%
go.viam.com/rdk/config 77%
go.viam.com/rdk/control 57%
go.viam.com/rdk/data 78%
go.viam.com/rdk/grpc 25%
go.viam.com/rdk/ml 67%
go.viam.com/rdk/ml/inference 70%
go.viam.com/rdk/motionplan 71%
go.viam.com/rdk/operation 84%
go.viam.com/rdk/pointcloud 71%
go.viam.com/rdk/protoutils 62%
go.viam.com/rdk/referenceframe 78%
go.viam.com/rdk/registry 88%
go.viam.com/rdk/resource 85%
go.viam.com/rdk/rimage 78%
go.viam.com/rdk/rimage/depthadapter 94%
go.viam.com/rdk/rimage/transform 73%
go.viam.com/rdk/rimage/transform/cmd/extrinsic_calibration 67%
go.viam.com/rdk/robot 93%
go.viam.com/rdk/robot/client 79%
go.viam.com/rdk/robot/framesystem 68%
go.viam.com/rdk/robot/impl 79%
go.viam.com/rdk/robot/server 58%
go.viam.com/rdk/robot/web 60%
go.viam.com/rdk/robot/web/stream 87%
go.viam.com/rdk/services/armremotecontrol 75%
go.viam.com/rdk/services/armremotecontrol/builtin 25%
go.viam.com/rdk/services/baseremotecontrol 75%
go.viam.com/rdk/services/baseremotecontrol/builtin 71%
go.viam.com/rdk/services/datamanager 62%
go.viam.com/rdk/services/datamanager/builtin 78%
go.viam.com/rdk/services/datamanager/datacapture 34%
go.viam.com/rdk/services/datamanager/datasync 70%
go.viam.com/rdk/services/motion 68%
go.viam.com/rdk/services/motion/builtin 89%
go.viam.com/rdk/services/navigation 54%
go.viam.com/rdk/services/sensors 78%
go.viam.com/rdk/services/sensors/builtin 97%
go.viam.com/rdk/services/shell 15%
go.viam.com/rdk/services/slam 86%
go.viam.com/rdk/services/slam/builtin 73%
go.viam.com/rdk/services/vision 82%
go.viam.com/rdk/services/vision/builtin 74%
go.viam.com/rdk/spatialmath 85%
go.viam.com/rdk/subtype 96%
go.viam.com/rdk/utils 71%
go.viam.com/rdk/vision 26%
go.viam.com/rdk/vision/chess 80%
go.viam.com/rdk/vision/delaunay 87%
go.viam.com/rdk/vision/keypoints 92%
go.viam.com/rdk/vision/objectdetection 82%
go.viam.com/rdk/vision/odometry 60%
go.viam.com/rdk/vision/odometry/cmd 0%
go.viam.com/rdk/vision/segmentation 49%
go.viam.com/rdk/web/server 26%
Summary 66% (19081 / 28773)

@micheal-parks micheal-parks merged commit 4d13776 into viamrobotics:main Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants