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

IllegalStateException: maximumVelocity should be a positive value #81

Open
SimonMarquis opened this issue Apr 9, 2024 · 16 comments
Open

Comments

@SimonMarquis
Copy link
Contributor

We are seeing a pretty substantial amount of errors in the latest version (0.9.0) that leads to this method:

val velocity = if (wasCancelled) Velocity.Zero else velocityTracker.calculateVelocity(maximumVelocity)

And all the stacktraces look like this (with value 0.0):

Fatal Exception: java.lang.IllegalStateException
maximumVelocity should be a positive value. You specified=0.0
androidx.compose.ui.input.pointer.util.VelocityTracker1D.calculateVelocity (VelocityTracker.kt:283)
androidx.compose.ui.input.pointer.util.VelocityTracker.calculateVelocity-AH228Gc (VelocityTracker.kt:102)
me.saket.telephoto.zoomable.internal.TransformableNode$pointerInputNode$1$1$2.invokeSuspend (transformable.kt:152)
kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith (ContinuationImpl.kt:33)
kotlinx.coroutines.DispatchedTaskKt.resume (DispatchedTask.kt:175)
kotlinx.coroutines.DispatchedTaskKt.dispatch (DispatchedTask.kt:164)

or this (with a tiny value, that looks like a 0 approximation in float):

Fatal Exception: java.lang.IllegalStateException
maximumVelocity should be a positive value. You specified=-1.3029473E-34
androidx.compose.ui.input.pointer.util.VelocityTracker1D.calculateVelocity (VelocityTracker.kt:283)
androidx.compose.ui.input.pointer.util.VelocityTracker.calculateVelocity-AH228Gc (VelocityTracker.kt:102)
me.saket.telephoto.zoomable.internal.TransformableNode$pointerInputNode$1$1$2.invokeSuspend (transformable.kt:152)
kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith (ContinuationImpl.kt:33)
kotlinx.coroutines.DispatchedTaskKt.resume (DispatchedTask.kt:175)
kotlinx.coroutines.DispatchedTaskKt.dispatch (DispatchedTask.kt:164)

I'm not sure why it would throw here to be honest, since we always provide sane values Velocity(Int.MAX_VALUE.toFloat(), Int.MAX_VALUE.toFloat()). Could this be an issue with the devices packing floats incorrectly?

The device repartition is as follow:

  • 70% Huawei
  • 20% Samsung
  • 6% Xiaomi
  • 1% Hisense
  • 3% Other (6)
@saket
Copy link
Owner

saket commented Apr 9, 2024

Just to confirm, were you not seeing any of these crashes on an older version of telephoto?

@saket
Copy link
Owner

saket commented Apr 10, 2024

Can you also share your version of Compose UI?

@SimonMarquis
Copy link
Contributor Author

Oh you're right, we did have them on previous versions as well (Crashlytics groupped them differently):

androidx.compose.ui.input.pointer.util.VelocityTracker1D.calculateVelocity (VelocityTracker.kt:283)
androidx.compose.ui.input.pointer.util.VelocityTracker.calculateVelocity-AH228Gc (VelocityTracker.kt:102)
androidx.compose.ui.input.pointer.util.VelocityTracker.calculateVelocity-9UxMQ8M (VelocityTracker.kt:82)
me.saket.telephoto.zoomable.internal.TransformableKt.calculateVelocity-OGnQdUo (transformable.kt:267)
me.saket.telephoto.zoomable.internal.TransformableKt.access$calculateVelocity-OGnQdUo (transformable.kt:1)
me.saket.telephoto.zoomable.internal.TransformableNode$pointerInputNode$1$1$2.invokeSuspend (transformable.kt:156)
kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith (ContinuationImpl.kt:33)
kotlinx.coroutines.DispatchedTaskKt.resume (DispatchedTask.kt:175)
kotlinx.coroutines.DispatchedTaskKt.dispatch (DispatchedTask.kt:164)

Our Compose UI version is currently at 1.6.4.

@saket
Copy link
Owner

saket commented Apr 10, 2024

I've replaced my manual calculation of maximum velocity with the framework one in 3230987. Let's see if this fixes the issue.

val maximumVelocity = currentValueOf(LocalViewConfiguration).let {
Velocity(it.maximumFlingVelocity, it.maximumFlingVelocity)
}

I'm not certain, but #76 could also help.

I'll cut a release soon. In the meantime, the changes are available in 0.10.0-SNAPSHOT if your project is comfortable using snapshots.

@SimonMarquis
Copy link
Contributor Author

Thanks for these updates. Unfortunately I won't be able to use such SNAPSHOT version in the main project for now 😖 I'll try to see if we can fork and test this patch.

@saket
Copy link
Owner

saket commented Apr 14, 2024

I just released https://github.com/saket/telephoto/releases/tag/0.10.0. Can you please give it a try?

ZacSweers pushed a commit to ZacSweers/CatchUp that referenced this issue Apr 15, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[me.saket.telephoto:zoomable-image-coil](https://togithub.com/saket/telephoto)
| `0.9.0` -> `0.10.0` |
[![age](https://developer.mend.io/api/mc/badges/age/maven/me.saket.telephoto:zoomable-image-coil/0.10.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/maven/me.saket.telephoto:zoomable-image-coil/0.10.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/maven/me.saket.telephoto:zoomable-image-coil/0.9.0/0.10.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/maven/me.saket.telephoto:zoomable-image-coil/0.9.0/0.10.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
|
[me.saket.telephoto:zoomable-image](https://togithub.com/saket/telephoto)
| `0.9.0` -> `0.10.0` |
[![age](https://developer.mend.io/api/mc/badges/age/maven/me.saket.telephoto:zoomable-image/0.10.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/maven/me.saket.telephoto:zoomable-image/0.10.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/maven/me.saket.telephoto:zoomable-image/0.9.0/0.10.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/maven/me.saket.telephoto:zoomable-image/0.9.0/0.10.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
| [me.saket.telephoto:zoomable](https://togithub.com/saket/telephoto) |
`0.9.0` -> `0.10.0` |
[![age](https://developer.mend.io/api/mc/badges/age/maven/me.saket.telephoto:zoomable/0.10.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/maven/me.saket.telephoto:zoomable/0.10.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/maven/me.saket.telephoto:zoomable/0.9.0/0.10.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/maven/me.saket.telephoto:zoomable/0.9.0/0.10.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>saket/telephoto
(me.saket.telephoto:zoomable-image-coil)</summary>

###
[`v0.10.0`](https://togithub.com/saket/telephoto/releases/tag/0.10.0)

[Compare
Source](https://togithub.com/saket/telephoto/compare/0.9.0...0.10.0)

Bug fixes

-
[saket/telephoto#70,
[saket/telephoto#72:
Correctly update `ZoomableState` when `Modifier.zoomable()` is reused
-
[saket/telephoto#71:
Make sure velocity tracker tracks the same pointer
-
[saket/telephoto#81:
Read maximum fling velocity from composition locals

Dependency updates

-   Compose compiler: 1.5.11
-   Compose UI: 1.6.4
-   Compose multiplatform: 1.6.4

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these
updates again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/ZacSweers/CatchUp).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yOTMuMCIsInVwZGF0ZWRJblZlciI6IjM3LjI5My4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@SimonMarquis
Copy link
Contributor Author

Our app update will be rolled-out starting next monday 🚀

github-merge-queue bot pushed a commit to slackhq/circuit that referenced this issue Apr 16, 2024
…1339)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
|
[me.saket.telephoto:zoomable-image-coil](https://togithub.com/saket/telephoto)
| dependencies | minor | `0.9.0` -> `0.10.0` |

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>saket/telephoto
(me.saket.telephoto:zoomable-image-coil)</summary>

###
[`v0.10.0`](https://togithub.com/saket/telephoto/releases/tag/0.10.0)

[Compare
Source](https://togithub.com/saket/telephoto/compare/0.9.0...0.10.0)

Bug fixes

-
[saket/telephoto#70,
[saket/telephoto#72:
Correctly update `ZoomableState` when `Modifier.zoomable()` is reused
-
[saket/telephoto#71:
Make sure velocity tracker tracks the same pointer
-
[saket/telephoto#81:
Read maximum fling velocity from composition locals

Dependency updates

-   Compose compiler: 1.5.11
-   Compose UI: 1.6.4
-   Compose multiplatform: 1.6.4

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yOTYuMCIsInVwZGF0ZWRJblZlciI6IjM3LjI5Ni4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->
@saket
Copy link
Owner

saket commented Apr 28, 2024

@SimonMarquis were you able to verify if the frequency of crashes has decreased?

@SimonMarquis
Copy link
Contributor Author

It seems like it did not change anything. New reports are coming up with this new version.
To me, the only reason why there could be an error is if maximumFlingVelocity coming from the ViewConfiguration is already negative:

Velocity(it.maximumFlingVelocity, it.maximumFlingVelocity)

The packing/unpacking of 2 floats in a long seems fine.

Given the uncommon distribution of devices having this bug, this could be an OS bug. And from the docs, nothing really prevents it from being negative after all.

Do you think we could try clamping these values?

          val maximumVelocity = currentValueOf(LocalViewConfiguration)
              .maximumFlingVelocity
              .coerceAtLeast(0F)
              .let { Velocity(it, it) }

@saket
Copy link
Owner

saket commented Apr 30, 2024

Oof. Considering that Velocity(Int.MAX_VALUE.toFloat(), Int.MAX_VALUE.toFloat()) was also causing the same crash, I don't think the values provided by ViewConfiguration are invalid...

I'll file a bug report.

@saket
Copy link
Owner

saket commented May 1, 2024

@SimonMarquis can you share more data about these devices, including their API versions?

@SimonMarquis
Copy link
Contributor Author

Devices:

  • 57% Huawei
    • 6% Huawei P30 Lite
    • 6% Huawei P30
    • 5% P20 Lite
    • 40% Other (34)
  • 26% Samsung
    • 10% Galaxy S8
    • 2% Galaxy A8(2018)
    • 2% Galaxy A5(2017)
    • 12% Other (19)
  • 10% Xiaomi
    • 1% Redmi Note 9S
    • 1% Redmi Note 9 Pro
    • 1% Redmi Note 8 Pro
    • 6% Other (15)
  • 2% Wiko - FIH Foxconn
    • 1% View5
    • 1% View5 Plus
  • 5% Other (9)

OS:

  • 57% Android 10
  • 25% Android 9
  • 18% Android 8

This OS distribution makes it look like a platform bug that has been fixed in Android 11.

@saket
Copy link
Owner

saket commented May 2, 2024

It very likely is! Interestingly, @JakeWharton reminded me yesterday that we encountered a similar bug in Cash App a year ago. It's amazing that I've already run into it again.

screenshot_2024-05-01_at_1.35.17___pm.png

I'll file a bug report on D8, and think of a way to suppress these in telephoto.

Is Android 8 your min sdk version?

@SimonMarquis
Copy link
Contributor Author

We have minSdk set to 24 (Android 7)

@saket
Copy link
Owner

saket commented May 2, 2024

I've filed a bug report on D8: https://issuetracker.google.com/u/1/issues/338381315.

@saket
Copy link
Owner

saket commented May 3, 2024

@SimonMarquis will you be able to share your apk here? https://issuetracker.google.com/issues/338381315#comment2

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

No branches or pull requests

2 participants