usuiat / Zoomable

Jetpack Compose library that enables contents zooming with pinch gesture.
https://usuiat.github.io/Zoomable/
Apache License 2.0
342 stars 16 forks source link

code fix suggestion #136

Closed pioChoi closed 7 months ago

pioChoi commented 7 months ago

Hi ! I have a question. applyGesture function in ZoomState class:

as-is

velocityTracker.addPosition(timeMillis, position)

to-be

velocityTracker.addPosition(timeMillis, newOffset)

Isn't this correct?

usuiat commented 7 months ago

@pioChoi Thank you for your question.

The correct value to pass to velocityTracker is position. This is because what we want to calculate with velocityTracker is the velocity at which the finger is moving.

In fact, since we are calling velocityTracker.addPosition() only when zoom == 1, the result is the same whether we use position or newOffset. However, in applyGesture(), position represents the position of the finger and newOffset represents the upper-left position of the image. Therefore, the correct intention of the code is position.

pioChoi commented 7 months ago

@usuiat Thank you for the detailed explanation! πŸ™

But When I logged the value, the newOffset and position values ​​were different. ( when zoom == 1 in applyGesture() ) So I tested it by inserting the newOffset value instead. If you move the image with one finger touch while it is zoomed, you can see that it moves more smoothly.

case: position

https://github.com/usuiat/Zoomable/assets/112914358/902badbe-519a-49d9-9c21-0f5b41ee4e4a

case: newOffset

https://github.com/usuiat/Zoomable/assets/112914358/d287a37c-9754-4538-bf81-31d032f4671f

usuiat commented 7 months ago

I have also logged. As a result, position was lagging behind newOffset by one frame.

The code is below. I logged the amount of change from the previous coordinate because it affects the velocityTracker calculation.

    var prevOffset = Offset.Zero
    var prevPosition = Offset.Zero

    internal suspend fun applyGesture() = coroutineScope {
        ...
        if (zoom == 1f) {
            val offsetDelta = newOffset - prevOffset
            val positionDelta = position - prevPosition
            println("position=$position delta=$positionDelta, newOffset=$newOffset delta=$offsetDelta")
            velocityTracker.addPosition(timeMillis, position)
            prevOffset = newOffset
            prevPosition = position
        }
        ...
    }

The result is below, showing that position was lagging behind newOffset by one frame.

image

The cause of this was that the position was calculated using one previous PointerEvent. I had specified useCurrent=false when calling calculateCentroid() in PointerInputScope.detectTransformGestures(). When useCurrent=true was specified, the changes of position and newOffset is the same.

image

image

usuiat commented 7 months ago

@pioChoi I will consider merging this change into the main branch. Thank you πŸ˜„

pioChoi commented 7 months ago

@usuiat I 100% understood what happened! Thank you !! πŸ‘

Then, should that code in TouchSlop also be changed to event.calculateCentroidSize(useCurrent = true)?