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 ChoreographerCallback double execution #4579

Merged
merged 3 commits into from
Jun 16, 2023

Conversation

piaskowyk
Copy link
Member

@piaskowyk piaskowyk commented Jun 15, 2023

Summary

This PR fixes an issue with long animations. On Android, it was possible to execute ChoreographerCallback twice during the same frame (after frame drop). In effect, existing animations were canceled due to this if-condition in requestAnimationFrame method:

nativeRequestAnimationFrame((timestamp) => {
  if (lastNativeAnimationFrameTimestamp >= timestamp) {
    // Make sure we only execute the callbacks once for a given frame
    return;
  }
...

To fix it, I moved this check to the native code. When double execution happens I skip the execution of the JS callback, but I still need to schedule another choreographer callback.

How to test it?

final long[] lastTimestamp = {0};
new GuardedFrameCallback(context) {
  @Override
  protected void doFrameGuarded(long frameTimeNanos) {
    if (lastTimestamp[0] == frameTimeNanos) {
      Log.w("test");
    }
    onAnimationFrame(frameTimeNanos);
    lastTimestamp[0] = frameTimeNanos;
  }
}

Before

  • Open SVGExample
  • Set a breakpoint in Android Studio at line Log.w("test");
  • Wait some time until the breakpoint hit
  • Pass breakpoint
  • Animation will stop

After

  • Open SVGExample
  • Set a breakpoint in Android Studio at line Log.w("test");
  • Wait some time until the breakpoint hit
  • Pass breakpoint
  • Animation will keep going

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

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

Looks good but please take a look at my comments prior to merging.

src/reanimated2/initializers.ts Outdated Show resolved Hide resolved
@piaskowyk piaskowyk added this pull request to the merge queue Jun 16, 2023
Merged via the queue into main with commit 08d570e Jun 16, 2023
5 checks passed
@piaskowyk piaskowyk deleted the @piaskowyk/fix-long-animations branch June 16, 2023 09:06
Latropos pushed a commit that referenced this pull request Jun 20, 2023
## Summary

This PR fixes an issue with long animations. On Android, it was possible
to execute ChoreographerCallback twice during the same frame (after
frame drop). In effect, existing animations were canceled due to this
if-condition in `requestAnimationFrame` method:
```js
nativeRequestAnimationFrame((timestamp) => {
  if (lastNativeAnimationFrameTimestamp >= timestamp) {
    // Make sure we only execute the callbacks once for a given frame
    return;
  }
...
```
To fix it, I moved this check to the native code. When double execution
happens I skip the execution of the JS callback, but I still need to
schedule another choreographer callback.

## How to test it?
```java
final long[] lastTimestamp = {0};
new GuardedFrameCallback(context) {
  @OverRide
  protected void doFrameGuarded(long frameTimeNanos) {
    if (lastTimestamp[0] == frameTimeNanos) {
      Log.w("test");
    }
    onAnimationFrame(frameTimeNanos);
    lastTimestamp[0] = frameTimeNanos;
  }
}
```
### Before
- Open SVGExample
- Set a breakpoint in Android Studio at line `Log.w("test");`
- Wait some time until the breakpoint hit
- Pass breakpoint
- Animation will **stop** ❌

### After
- Open SVGExample
- Set a breakpoint in Android Studio at line `Log.w("test");`
- Wait some time until the breakpoint hit
- Pass breakpoint
- Animation will **keep going**  ✅
@tlow92
Copy link

tlow92 commented Jun 23, 2023

I'm not 100% sure if this really is related but I want to inform you nonetheless:
I faced this issue on latest stable 3.3.0 and applied your fix. It seemed to have fixed my initial issue of canceled animations.
But I also use gorhom/bottom-sheet (uses reanimated) and when the animation for the sheet is opened during an already active animation running it seems to cancel the other animations.

After some investigating I think the behaviour is like this:

  • So if sheet opening is triggered before other animations started everything is fine.
  • If sheet is opened after other animations finished it's also fine.
  • If sheet is opened after other animations started but not finished yet they will be canceled.

Unfortunately I can not provide a reproducible demo right now due to lack of time and my project is very complex, but maybe it helps in case there are other hints to a possible issue.

Edit: To be clear this issue is not happening on 2.14.4

@piaskowyk
Copy link
Member Author

Hey @tlow92 👋 Thanks for your report! It seems like a different problem, could you open a new issue with it?

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.

None yet

3 participants