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

On pointer move is recording coordinates too frequent, adding a throttling to optimise #163

Open
Louis116 opened this issue Aug 1, 2024 · 5 comments · Fixed by #164
Open

Comments

@Louis116
Copy link

Louis116 commented Aug 1, 2024

Is your feature request related to a problem? Please describe.

  • The current interval of recording the coordinates on pointer move is around 1ms, depending on how fast could the browser run
    (please correct me if the time is not correct)
  • which cause a problem of :

  1. recording too much , marking unnescecery coordinates
  2. the smoothing may not be working as much as expected due to the point is too close, the smoothing effects is reduced
  3. possible performance issue

Describe the solution you'd like
- i have tried to add a interval inside the handlePointerMove function of <Canvas/>

  • which tested throttling of around 10ms to 20ms works the best
  • which could reduce the coordinates point recorded and make the line smoother

Updated on 7 Aug

  • since the handleOnPointerMove handler is still triggering every ms
  • Tried another approach of storing reference of an interval for move event throttle
    const currentPointerDownEventInterval = React.useRef<NodeJS.Timeout | null>(
        null,
    )
    const handlePointerUp = useCallback(
        (event: React.PointerEvent<HTMLDivElement> | PointerEvent): void => {
            if (event.pointerType === 'mouse' && event.button !== 0) {
                return
            }

            // Allow only chosen pointer type
            if (
                allowOnlyPointerType !== 'all' &&
                event.pointerType !== allowOnlyPointerType
            ) {
                return
            }

            onPointerUp(event)
            if (currentPointerDownEventInterval.current !== null) {
                clearInterval(currentPointerDownEventInterval.current)
            }
        },
        [allowOnlyPointerType, onPointerUp],
    )
    const ON_POINTER_MOVE_THROTTLE_MS = 20
     const handlePointerMove = useCallback(
        (event: React.PointerEvent<HTMLDivElement>): void => {
            if (!isDrawing) {
                return
            }

            // Allow only chosen pointer type
            if (
                allowOnlyPointerType !== 'all' &&
                event.pointerType !== allowOnlyPointerType
            ) {
                return
            }

            if (currentPointerDownEventInterval.current !== null) {
                return
            }

            function handleOnPointerMove() {
                const point = getCoordinates(event)
                onPointerMove(point)
                currentPointerDownEventInterval.current = null
            }
            const interval = setTimeout(
                handleOnPointerMove,
                ON_POINTER_MOVE_THROTTLE_MS,
            )
            currentPointerDownEventInterval.current = interval
        },
        [allowOnlyPointerType, getCoordinates, isDrawing, onPointerMove],
    )

below is a result comparsion (please forgive my poor hand writing)

without throttling with throttling of 20ms
image image

Draw Back
however, this solution have some draw back:
the throtting ms is kind of limited if you are aiming for the best result.
when drawing too fast, the path will 'wiggle', especially when the throttling is above 50 ms, i am still studying the root
cause, would love to have some advise if anyone have any insight, but i think this draw back is not critial
1. the path is not going to change too much when wiggling (if throttling is under 20ms)
2. personally think it look like a fun visual effect of drawing on the board

  • draw back is gone with updated approach

Describe alternatives you've considered

  • tried to tune the the smoothing factor inside <Path/>, but result is not as good as expected due to the coordinates are too close

Additional context

  • this throttling ms could be a configable props, but due to the 'wiggle' issue, maybe a fixed number just like the smoothing inside <Path/> is also considerable
  • 'wiggle' not exist on new approach
@Louis116
Copy link
Author

Louis116 commented Aug 7, 2024

approach updated by using storing reference and early return

@Louis116
Copy link
Author

if adding dependency is fine,
using useThrottledCallback from use-debounce package could easily impletement the throttle
https://github.com/xnimorz/use-debounce

just add below handler to the onPointerMove, no other changes

    const throttledPointerMove = useThrottledCallback(
        handlePointerMove,
        ON_POINTER_MOVE_THROTTLE_MS,
    )

vinothpandian added a commit that referenced this issue Aug 17, 2024
Fixes #163

Add throttling to pointer move events in the Canvas component to optimize coordinate recording.

- Modify `packages/react-sketch-canvas/src/Canvas/index.tsx` to add throttling to the `handlePointerMove` function using a custom `useThrottledCallback` function.
- Add `throttleTime` to `CanvasProps` in `packages/react-sketch-canvas/src/Canvas/types.ts` with a default value of 20.
- Add `throttleTime` to `ReactSketchCanvasProps` in `packages/react-sketch-canvas/src/ReactSketchCanvas/types.ts` with a default value of 20.
- Add `useThrottledCallback` function in `packages/react-sketch-canvas/src/Canvas/utils.tsx` to throttle a callback function.
- Update `README.md` to include documentation on the new `throttleTime` configuration option.
- Add tests to verify throttling behavior in `packages/tests/src/actions/throttling.spec.ts`.
- Write vitest tests for `useThrottledCallback` function in `packages/react-sketch-canvas/src/Canvas/__test__/utils.test.tsx`.

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/vinothpandian/react-sketch-canvas/issues/163?shareId=XXXX-XXXX-XXXX-XXXX).
vinothpandian added a commit that referenced this issue Aug 17, 2024
* Add throttling to pointer move event

Fixes #163

Add throttling to pointer move events in the Canvas component to optimize coordinate recording.

- Modify `packages/react-sketch-canvas/src/Canvas/index.tsx` to add throttling to the `handlePointerMove` function using a custom `useThrottledCallback` function.
- Add `throttleTime` to `CanvasProps` in `packages/react-sketch-canvas/src/Canvas/types.ts` with a default value of 20.
- Add `throttleTime` to `ReactSketchCanvasProps` in `packages/react-sketch-canvas/src/ReactSketchCanvas/types.ts` with a default value of 20.
- Add `useThrottledCallback` function in `packages/react-sketch-canvas/src/Canvas/utils.tsx` to throttle a callback function.
- Update `README.md` to include documentation on the new `throttleTime` configuration option.
- Add tests to verify throttling behavior in `packages/tests/src/actions/throttling.spec.ts`.
- Write vitest tests for `useThrottledCallback` function in `packages/react-sketch-canvas/src/Canvas/__test__/utils.test.tsx`.

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/vinothpandian/react-sketch-canvas/issues/163?shareId=XXXX-XXXX-XXXX-XXXX).

* fix: tests

* fix: set default throttleTime to 0

* chore: remove lint in ci

* fix: workflow
@heruiwoniou
Copy link

I had the same problem, but adding throttle function would cause some other problem if moving quickly. My current solution is to filter out the extra points when they get too close

@Louis116
Copy link
Author

Louis116 commented Sep 10, 2024

I had the same problem, but adding throttle function would cause some other problem if moving quickly. My current solution is to filter out the extra points when they get too close

would you mind elaborate what is that problem caused by moving too fast?
and how fast are we talking?

filtering the point if they get too close is also an viable approach, but you may also want to check if the Math.sqrt / sin cos tan do not have performance issue, i am not too sure about this part, but imo looks quite intense for an every ms operation, especially for mid to low end devices
line
control point

@heruiwoniou
Copy link

I had the same problem, but adding throttle function would cause some other problem if moving quickly. My current solution is to filter out the extra points when they get too close

would you mind elaborate what is that problem caused by moving too fast? and how fast are we talking?

filtering the point if they get too close is also an viable approach, but you may also want to check if the Math.sqrt / sin cos tan do not have performance issue, i am not too sure about this part, but imo looks quite intense for an every ms operation, especially for mid to low end devices line control point

Don't worry fast problem, the main problem I have encountered is still the problem you described. I just simply judged the distance between two points, and ignored the point when it was less than a certain value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants