refactor(overscroll): Make springback effect closer to Xiaomi's native behavior. #164
Conversation
HChenX
changed the title
This commit refactors the overscroll implementation with several key improvements: * Simplifies the spring equilibrium check by using `sign` to detect when the position crosses zero, removing the previous complex logic. * Streamlines the `applyDrag` function to only update the offset without returning an overflow value, simplifying the drag calculation. * Refactors `onPreScroll` and `onPostScroll` to use this simplified drag logic, making the code more concise and removing redundant calculations. * Optimizes the `onPreFling` behavior by consuming a portion of the fling velocity when the fling is in the opposite direction of the overscroll, improving the feel of the animation.
Passes the consumed offsets from the parent nested scroll connection through the `Overscroll` handler. This ensures that scroll and fling events consumed by parent composables are correctly accounted for, preventing them from being ignored by the overscroll effect.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| /** | ||
| * Calculate the new velocity using Euler's formula |
There was a problem hiding this comment.
The comment says "Calculate the new velocity using Euler's formula" but this is actually Euler's method (also known as Euler integration or forward Euler), not Euler's formula. Euler's formula refers to e^(ix) = cos(x) + i*sin(x).
Suggested correction:
/**
* Calculate the new velocity using Euler's method (forward Euler integration)
* */| * Calculate the new velocity using Euler's formula | |
| * Calculate the new velocity using Euler's method (forward Euler integration) |
| } | ||
| } | ||
|
|
||
| private class SpringOperator(dampingRatio: Float, naturalPeriod: Float) { |
There was a problem hiding this comment.
The naturalPeriod parameter in the SpringOperator is in seconds, but this is not clearly documented. The constants STANDARD_SPRING_PERIOD = 0.4f and SLOWER_SPRING_PERIOD_FOR_HIGH_VELOCITY = 0.55f would benefit from documentation indicating the unit.
Consider adding a KDoc comment:
/**
* @param dampingRatio The damping ratio (1.0 = critical damping)
* @param naturalPeriod The natural period of oscillation in seconds
*/
private class SpringOperator(dampingRatio: Float, naturalPeriod: Float) {| currentPosition: Double, | ||
| targetPosition: Double | ||
| ): Double { | ||
| val velocityDecayFactor = 1.0 - dampingCoefficient * deltaTime |
There was a problem hiding this comment.
The velocityDecayFactor calculation can potentially become negative if dampingCoefficient * deltaTime > 1.0, which would cause the velocity to flip sign incorrectly. While deltaTime is clamped to MAX_FRAME_DELTA_SECONDS, under certain combinations of damping ratio and natural period, this could still occur.
Consider adding safety bounds:
val velocityDecayFactor = (1.0 - dampingCoefficient * deltaTime).coerceAtLeast(0.0)| val velocityDecayFactor = 1.0 - dampingCoefficient * deltaTime | |
| val velocityDecayFactor = (1.0 - dampingCoefficient * deltaTime).coerceAtLeast(0.0) |
| val scrollRange = with(LocalDensity.current) { | ||
| (if (isVertical) windowSize.height.toDp() else windowSize.width.toDp()).toPx() | ||
| } |
There was a problem hiding this comment.
The scrollRange calculation unnecessarily converts physical pixels to Dp and back to pixels. The getWindowSize() function already returns physical pixel dimensions (as seen in the Android implementation using windowMetrics.bounds.width()), so converting to Dp and back is redundant and may introduce precision loss.
Simplify to:
val scrollRange = if (isVertical) windowSize.height.toFloat() else windowSize.width.toFloat()| val scrollRange = with(LocalDensity.current) { | |
| (if (isVertical) windowSize.height.toDp() else windowSize.width.toDp()).toPx() | |
| } | |
| val scrollRange = if (isVertical) windowSize.height.toFloat() else windowSize.width.toFloat() |
| if (abs(offset) <= offsetThreshold || sign(delta) == sign(rawTouchAccumulation)) { | ||
| return parentConsumed | ||
| } | ||
| // If a fling animation is running, request it to stop to blend with user input. | ||
| if (flingAnimatable.isRunning) { | ||
| coroutineScope.launch { | ||
| // stop() is suspend; we call it in coroutine context to stop smoothly. | ||
| flingAnimatable.stop() | ||
|
|
||
| if (sign(delta) != sign(rawTouchAccumulation)) { // opposite direction |
There was a problem hiding this comment.
The condition on line 295 returns early if sign(delta) == sign(rawTouchAccumulation), but then line 299 checks if (sign(delta) != sign(rawTouchAccumulation)) which will always be true at that point. The second condition is redundant and should be removed.
Additionally, after the condition on line 295, the only possible case is when the signs are opposite, so the check on line 299 is unnecessary. Consider restructuring:
if (abs(offset) <= offsetThreshold) {
return parentConsumed
}
if (sign(delta) == sign(rawTouchAccumulation)) {
return parentConsumed
}
// At this point, signs are opposite
val actualConsumed = if (abs(rawTouchAccumulation) <= abs(delta)) {
-rawTouchAccumulation
} else {
delta
}
// ... rest of logic| if (absPixelOffset >= absMaxOffset) return absMaxOffset | ||
| if (absPixelOffset <= 0f) return 0f | ||
|
|
||
| val part2 = range.toDouble().pow(2.0 / 3.0) * (range - (3.0 * absPixelOffset)).pow(1.0 / 3.0) |
There was a problem hiding this comment.
The formula (range - (3.0 * absPixelOffset)).pow(1.0 / 3.0) can produce a negative value when absPixelOffset > range / 3, which would then attempt to take the cube root of a negative number. While Kotlin's pow function with fractional exponents on negative numbers returns NaN, this might not be the intended behavior.
Consider adding validation or adjusting the formula:
val innerValue = range - (3.0 * absPixelOffset)
if (innerValue < 0.0) return range.toFloat() // or handle appropriately
val part2 = range.toDouble().pow(2.0 / 3.0) * innerValue.pow(1.0 / 3.0)| val part2 = range.toDouble().pow(2.0 / 3.0) * (range - (3.0 * absPixelOffset)).pow(1.0 / 3.0) | |
| val innerValue = range - (3.0 * absPixelOffset) | |
| if (innerValue < 0.0) return range.toFloat() | |
| val part2 = range.toDouble().pow(2.0 / 3.0) * innerValue.pow(1.0 / 3.0) |
2 participants
No description provided.