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

fix(pinch): cap the movement of wheel events based on the provided bounds #549

Merged
merged 2 commits into from
Oct 30, 2022

Conversation

Andarist
Copy link
Contributor

I've noticed that with the code like scale is correctly capped:

  useGesture(
    {
      onPinch: ({ origin: [ox, oy], offset: [scale] }) => {
        // scale is correctly capped to the given bounds
      },
    },
    {
      target: svgRef,
      pinch: {
        scaleBounds: { min: 0.1, max: 10 },
        from: [zoom, 0],
        modifierKey: 'metaKey',
      },
    },
  );

However, if we go way outside of the bounds and start going back then nothing happens, for a considerable amount of time. This is because we are not capping the _movement in the wheelChange within the PinchEngine. I think that it makes sense to cap this here, in the same way, that it's done in the WheelEngine:

// _movement rolls back to when it passed the bounds.
const [ox, oy] = state.overflow
const [dx, dy] = state._delta
const [dirx, diry] = state._direction
if ((ox < 0 && dx > 0 && dirx < 0) || (ox > 0 && dx < 0 && dirx > 0)) {
state._movement[0] = state._movementBound[0] as number
}
if ((oy < 0 && dy > 0 && diry < 0) || (oy > 0 && dy < 0 && diry > 0)) {
state._movement[1] = state._movementBound[1] as number
}

I'm not sure if touch and pointer handlers should also implement the same logic though - so, for now, I left this untouched.

@changeset-bot
Copy link

changeset-bot bot commented Oct 26, 2022

🦋 Changeset detected

Latest commit: 0167274

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@use-gesture/core Patch
@use-gesture/react Patch
@use-gesture/vanilla Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Andarist Andarist changed the title fix(pinch): cap the movemenet of wheel events based on the provided bounds fix(pinch): cap the movement of wheel events based on the provided bounds Oct 26, 2022
Comment on lines 268 to 279
// _movement rolls back to when it passed the bounds.
const [ox, oy] = state.overflow
const [dx, dy] = state._delta
const [dirx, diry] = state._direction

if ((ox < 0 && dx > 0 && dirx < 0) || (ox > 0 && dx < 0 && dirx > 0)) {
state._movement[0] = state._movementBound[0] as number
}

if ((oy < 0 && dy > 0 && diry < 0) || (oy > 0 && dy < 0 && diry > 0)) {
state._movement[1] = state._movementBound[1] as number
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

copy-paste from the WheelEngine:

// _movement rolls back to when it passed the bounds.
const [ox, oy] = state.overflow
const [dx, dy] = state._delta
const [dirx, diry] = state._direction
if ((ox < 0 && dx > 0 && dirx < 0) || (ox > 0 && dx < 0 && dirx > 0)) {
state._movement[0] = state._movementBound[0] as number
}
if ((oy < 0 && dy > 0 && diry < 0) || (oy > 0 && dy < 0 && diry > 0)) {
state._movement[1] = state._movementBound[1] as number
}

I could move this to a shared helper or something if you want.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No worries, just did it!

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 26, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0167274:

Sandbox Source
gesture-drag Configuration
gesture-drag-target Configuration
gesture-nested Configuration
gesture-drag-vanilla Configuration
gesture-move Configuration
gesture-pinch Configuration
gesture-multiple-pinch Configuration
gesture-three Configuration
gesture-card-zoom Configuration
gesture-viewpager Configuration

@Andarist
Copy link
Contributor Author

Actually, I'm not exactly sure what I'm observing right now. After using this patch locally, I had a first impression that it fixed the issue. Now I think that it got better, but sometimes I'm still able to observe the original issue (I think?).

@dbismut
Copy link
Collaborator

dbismut commented Oct 26, 2022

Thanks for reporting this.

Seems weird since I think I had fixed that non-capping issue a while ago, and since gesture engines share the same logic I'm surprised to see it resurface but you never know!

And as far as I remember my own code, attributes starting with a _ should reflect the raw data of a gesture and shouldn't be capped.

@Andarist
Copy link
Contributor Author

@dbismut this PR is directly based on your fix that you have mentioned :P

c5067dc#diff-68df9df6068723ef860d52a8f182549f42ba07bed3991e77a9da07ec5c1cb541

@Andarist
Copy link
Contributor Author

Note that I was thinking if this capping should also happen for actual touch-based pinching. But I came to the conclusion that maybe it shouldn't. The difference is that if you physically pinch then you see a visual difference between the bounds and your fingers, so it might actually be desirable to only start "reversing" when your pinch back to the bounds. With wheel-based "pinching" this is not as obvious and you have no visual indicator that you are out of bounds and how close are to the bounds, so I think that it makes sense to "reverse" immediately.

@dbismut dbismut merged commit 6f4c09b into pmndrs:main Oct 30, 2022
@dbismut
Copy link
Collaborator

dbismut commented Oct 30, 2022

@Andarist Sorry for the delay! Yeah I had the same temptation when I first implemented bounds, but indeed you'd rather have the elements follow your fingers on their way back. I'm not sure why you can still observe a bug, might be because of the inertia somehow.

Thanks again for your help on this.

@Andarist Andarist deleted the fix/pinch-bounds branch October 30, 2022 21:26
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 this pull request may close these issues.

2 participants