-
-
Notifications
You must be signed in to change notification settings - Fork 334
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: flicker when starting pan: panOffset
value race condition
#576
Conversation
🦋 Changeset detectedLatest commit: a99f069 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
panOffset
value race conditionpanOffset
value race condition
@nmassey have you found a workaround while you wait this to be merged? I have a release blocked because of this (I guess) |
@BrodaNoel wrote:
I use yarn patch to get this in (along with a couple of other tweaks I have). Are you on the Discord? |
@nmassey I just joined that Discord, but if you can share the patch here, will be awesome! |
I had created and applied this patch, but it doesn't fix the issue I mention in #590 (which I thiiiink it's the same as you are trying to fix here)
|
In addition to patching the source code, you'll also need to patch the relevant built You can either patch the source code by hand (possible, but annoying), or patch the target plugin in its own repo and rebuild it to generate the relevant Note that you can confirm that your patch is running by adding some easily-recognizable code to the relevant I hope this helps! 🤓🙏 And, catch you on Discord! |
@nmassey can you please share the versions of all the packages you are using? |
when it would be released? 😄 |
Incredible work, thank you! But before merging, could you gen a changeset for this PR?
|
so it is now ready to merge imho |
^up |
I also see this problem on Alpha 12, on both emulator and real devices, but only android. |
When will this patch be released? |
Waiting for this patch sooo much too! |
Any update for this issue? Would be nice to start using 4.0 but Alpha 12 still has flickering issues. |
@dohooo any chance this can be merged soon? |
Subscribing to know when this is merged. Going back to old RNCarousel temporarily 🥺 |
hi @Crizzooo - I've been using yarn patch to unblock me while waiting for these PRs to be merged 😕 |
One of our engineers just joined and is applying the patch for us :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
* (PC-31740) chore: upgrade reanimated to 3.15 * fix: issues with carousel * fix: patch react-native-reanimated-carousel (dohooo/react-native-reanimated-carousel#576) * test: fix tests * fix: OfferPreviewModal .web file --------- Co-authored-by: Mathieu Meissonnier <[email protected]>
* (PC-31740) chore: upgrade reanimated to 3.15 * fix: issues with carousel * fix: patch react-native-reanimated-carousel (dohooo/react-native-reanimated-carousel#576) * test: fix tests * fix: OfferPreviewModal .web file --------- Co-authored-by: Mathieu Meissonnier <[email protected]>
What: the bug
When
onGestureUpdate
is called, it may be using the previous value forpanOffset
(rather than the value just set inonGestureStart
). This seems to cause a flicker on slow devices since atranslation
is shown that is from a different screen.I see this flicker most obviously on my Android simulator (please see video demonstration below), but the problem exists on other devices as well.
Why
This is due to updates to shared values created via
useSharedValue()
updating asynchronously (see https://docs.swmansion.com/react-native-reanimated/docs/core/useSharedValue/#remarks ).This flicker seemed to get worse after this commit b654f43 - "feat: replaced onScrollBegin with onScrollStart" (I'm not sure if there's a corresponding PR?).
This is due to the order of the handlers being triggered:
onBegin
thenonStart
thenonUpdate
...panOffset
was saved duringonBegin
-- which would often happen much longer beforeonUpdate
.panOffset
is saved duringonStart
-- which might happen immediately beforeonUpdate
and thus may not allow enough time for the asynchronous update to occur.What: proposed fix
Let's add a check to make sure that
panOffset
has a valid value before we try to use it.panOffset
will have a value ofundefined
.onGestureStart
, setpanOffset
to its correct value. This is not a change from existing code.)onGestureUpdate
andonGestureEnd
, check to ensure thatpanOffset
is notundefined
before attempting to use it.onGestureEnd
, resetpanOffset
toundefined
(since pan is no longer active).Screengrab video recordings
These recordings were taken on an Android simulator.
buggy behavior on
4.0.0-alpha.10
(unpatched)Notice the flicker to a different "page" when starting pan (i.e. starting touch).
Screen.Recording.2024-03-25.at.09.35.52.mov
improved behavior with patch
Screen.Recording.2024-03-25.at.09.44.35.mov