undefined velocities are passed to onTouch's onEnd event

[gaishimo]

SpringBackTouch example freezes if you release your finger immediately after tapping without dragging. It seems to be caused by undefined velocity values being passed to runSpring in onEnd.

I fixed the example to return if the values are undefined.
I fixed useTouchHandler.

fixes #788

Example

[chrfalch]

It might be better to find out why the touch handler passes null for these values - maybe that could help others as well.

[chrfalch]

This might be solved by looking in the file useTouchHandler.ts on lines 59..60?

[chrfalch]

And thanks so much for submitting a PR, @gaishimo !! We really appreciate it!

[gaishimo]

Thanks for the review 👍

I fixed onTouchHandler.
83b8482

If the finger is released immediately, only one touch record is sent as below.

touches: [{"force": 0, "id": 5126886480, "timestamp": 1660266755.592, "type": 2, "x": 69, "y": 147}]

In this case, since velocity cannot be calculated, I think it is preferable to assume that it was not moving and should return zero.

[chrfalch]

Thanks again :) I'll merge very soon!

[chrfalch]

Ideally we don't want changes in formatting and ordering in a PR, so this block could be left out since it is not really related to the fix. We'll leave it for now, just wanted to let you know :)

[chrfalch]

Perfect :) Thanks for finding this and providing a fix!

[wcandillon]

There might be a small clean-up there.

1. The velocity is computed above and might be needed to be set to 0 if timeDiffseconds === 0. If should be done at the same place.
2. The velocity is computed and stored in x and y property of the previous touch, could it be velocityX and velocityY? Maybe it shouldn't be stored in the ref at all? It looks like we are calculating the velocity based on a velocity in the next frame (might be wrong).

[chrfalch]

1: So something like:

if (timeDiffseconds > 0) {
  prevVelocityRef.current[touch.id] = {
    x: distX / timeDiffseconds / PixelRatio.get(),
    y: distY / timeDiffseconds / PixelRatio.get(),
  };
} else {
  prevVelocityRef.current[touch.id] = { x: 0, y: 0 }; // <- new
}

But wouldn't this be covered by the fix proposed in this PR?

2: We're using a ref to store the previous one so that we only calculate velocities when we move - on end/cancel we should use the previously calculated value - since end/cancel are called immediately (timeDiffSeconds === 0) after the last active.

[wcandillon]

1. Exactly and this is part of this PR right? Since the goal is to have this value always defined and the types to be able to statically catch such "bug"?
2. Got it and it makes sense.

[chrfalch]

Yes and yes :) So then we're good to go with this one?

[gaishimo]

I understand. Thanks for your review.

[wcandillon]

@gaishimo We think the fix should look like this: #794 could you update this PR accordingly? We will merge it and release it. Thank you for your contribution.

[wcandillon]

awesome thks :)

[gaishimo]

@wcandillon OK, thanks for the opportunity to contribute to this good library 👍

[wcandillon]

Thank you for fixing this issue, let us know if you have any questions about Skia 🙌🏼

[thespacemanatee]

@chrfalch This change is causing this crash for us:

My TouchHandler:

const onTouch = useTouchHandler({
  onStart: () => {
    if (!disabled) {
      runTiming(pressed, 1, { duration: 150 })
    }
  },
  onEnd: () => {
    if (!disabled) {
      runTiming(pressed, 0, { duration: 150 })
    }
  },
})

[wcandillon]

sorry about that, we have a hot fix for it at #798

[gaishimo]

Sorry, I used the code of reference PR as is when updating the PR... I should have checked after modifying it 🙇🙏

[wcandillon]

@gaishimo not at all, this was our mistake, but it was also a blessing in disguise as it helped us find a serious bug related to velocity computation.