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

Using interpolateColor inside useDerivedValue creates colors glitches on iOS #6119

Closed
swansontec opened this issue Jun 13, 2024 · 6 comments
Closed
Labels
Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snippet of code, snack or repo is provided Reproducible 🎉

Comments

@swansontec
Copy link
Contributor

swansontec commented Jun 13, 2024

Description

Returning an interpolateColor value from a useDerivedValue will cause incorrect colors to appear on iOS. Here is the setup:

const blue = '#2080c0';
const derivedColor = useDerivedValue(() =>
  interpolateColor(0, [0, 1], [blue, blue]),
);

This should always render as blue if we put it in an animated style:

const animatedStyle = useAnimatedStyle(() => ({
  color: derivedColor.value,
}));

However, on iOS specifically, the text will glitch to pink after the first button press:

  // Cause a re-render on press:
  const [count, setCount] = React.useState(0);
  const handleCount = () => setCount(count + 1);

  return (
    <>
      <Animated.Text style={animatedStyle}>
        {count}
      </Animated.Text>
      <Button onPress={handleCount} title="Count" />
    </>
  );

Steps to reproduce

Here is what seems to be happening:

  1. First render
    a. JS thread: useDerivedValue returns the string "rgba(32, 128, 192, 1)"
    b. JS thread: useAnimatedStyle returns the style { color: "rgba(32, 128, 192, 1)" }
    c. The text renders as blue
    d. UI thread: useDerivedValue runs again, and returns the integer 0xff2080c0. This seems to be some sort of optimization, since this differs from what the JS thread returns.
  2. User presses the button
  3. Second render
    a. JS thread: useDerivedValue does not run, but returns the saved 0xff2080c0 from the UI thread.
    b. JS thread: useAnimatedStyle returns the style { color: 0xff2080c0 }
    c. The text renders as pink

Directly rendering <Text style={{ color: 0xff2080c0 }}>Hello</Text> produces the same pink color (although TypeScript complains that numbers aren't valid colors). Messing around with the values, it seems that useAnimatedStyle on the JS thread interprets integers as 0xrrggbbaa, whereas the Reanimated UI thread interprets integers as 0xaarrggbb.

Snack or a link to a repository

https://github.com/swansontec/PinkEyeTest

Reanimated version

3.12.0

React Native version

0.74.2

Platforms

iOS

JavaScript runtime

Hermes

Workflow

React Native

Architecture

Paper (Old Architecture)

Build type

Any

Device

Any

Device model

No response

Acknowledgements

Yes

@github-actions github-actions bot added Repro provided A reproduction with a snippet of code, snack or repo is provided Platform: iOS This issue is specific to iOS labels Jun 13, 2024
@swansontec
Copy link
Contributor Author

swansontec commented Jun 13, 2024

As a hacky work-around (since I don't know Reanimated well enough to fix this myself), we are using patch-package to edit node_modules/react-native-reanimated/src/reanimated2/Colors.ts, around line 520:

-  if (IS_WEB || !_WORKLET) {
+  if (!IS_ANDROID || !_WORKLET) {
     return `rgba(${r}, ${g}, ${b}, ${alpha})`;
   }

This removes a performance optimization, but it does solve the bug.

@szydlovsky
Copy link
Contributor

@swansontec Seems like a fully fledged bug but I have one question: why use interpolateColor inside of useDerivedValue, if it doesn't use any other shared value? (useDerivedValue is specifically used for creating shared values based on other shared values).

@swansontec
Copy link
Contributor Author

swansontec commented Jun 17, 2024

@szydlovsky Yeah, this code doesn't make any sense because it's a minimal repo case. Animations aren't really necessary to trigger the bug.

In our actual app, we do use animations to control the derived value where we found the bug:

const iconColor = useDerivedValue(() => interpolateIconColor(focusAnimation, disableAnimation))

@szydlovsky
Copy link
Contributor

Hey @swansontec I think I found a solution for you! It required some digging, but the performance optimisation you mentioned was indeed the culprit. It is there to speed up the way we pass colors on UI thread (hence _WORKLET check). Unfortunately, useDerivedValue runs some of its code on UI, which ends up giving us processed colors as numbers (meant for UI thread only). I found a kinda hacky workaround for you and in general I will discuss the topic with the team.

What I did is: made sure interpolation function runs on JS using runOnJS. This forces us to define the interpolation function on JS thread, but you mentioned that in your app it uses shared values as well, so I parametrised it. I also added a shared value that gets the return of mentioned interpolation and makes sure that useDerivedValue updates. Also added some small color change to test animations along with it. The changed code is here:

import * as React from 'react';
import { SafeAreaView, Button } from 'react-native';
import Animated, {
  interpolateColor,
  runOnJS,
  useAnimatedStyle,
  useDerivedValue,
  useSharedValue,
  withTiming,
} from 'react-native-reanimated';

const blue = '#2080c0';
const yellow = '#e0e63e';

export function RepoCase(): React.JSX.Element {
  // Shared values
  const changingValue = useSharedValue(0); // for animation
  const colorRef = useSharedValue(''); // for keeping the color

  // interpolate function defined on JS thread
  function interpolateFunc(someNumber: number) {
    colorRef.value = interpolateColor(someNumber, [0, 1], [yellow, blue]);
  }

  const derivedColor = useDerivedValue(() => {
    runOnJS(interpolateFunc)(changingValue.value);
    return colorRef.value;
  });

  const animatedStyle = useAnimatedStyle(() => {
    console.log(derivedColor.value);
    return {
      color: derivedColor.value,
      fontSize: 100,
      fontWeight: '700',
    };
  });

  // Re-render on press:
  const [count, setCount] = React.useState(0);
  const handleCount = () => setCount(count + 1);

  const toggleColors = () => {
    changingValue.value =
      changingValue.value > 0
        ? (changingValue.value = withTiming(0, { duration: 2000 }))
        : (changingValue.value = withTiming(1, { duration: 2000 }));
  };

  return (
    <SafeAreaView>
      <Animated.Text style={animatedStyle}>{count}</Animated.Text>
      <Button onPress={handleCount} title="Count" />
      <Button onPress={toggleColors} title="Toggle colors" />
    </SafeAreaView>
  );
}

function App(): React.JSX.Element {
  const [id, setId] = React.useState(0);
  const handleReset = () => setId(id + 1);

  return (
    <SafeAreaView>
      <RepoCase key={`id${id}`} />
      <Button onPress={handleReset} title="Reset" />
    </SafeAreaView>
  );
}

export default App;

Let me know what you think!

@szydlovsky
Copy link
Contributor

Hey @swansontec turns out this "optmialisation" you mentioned was unneeded whatsoever haha. Once the PR goes through review I'll send you a patch to apply 👍

github-merge-queue bot pushed a commit that referenced this issue Jul 5, 2024
## Summary

Our `interpolateColor` function seems to have some unnecessary color
processing in it. It was added some 3-4 years ago and should not be
needed by now.

## Why

There is a function called `rgbaColor` which is used in `hsvToColor`
(which in turn is used only in `interpolateColor`) as well as directly
in `interpolateColor`. So it's safe to say it is used ***only*** there.
All it does is take r,g,b,a values, runs a small check on them and then
returns a rgba string.
It has a check whether we are on UI thread or not. If we were on UI, it
returned a number-formatted color (formatted for native platforms)
instead of the usual rgba string. The problem is, these values are then
directly returned from the function. I believe it was needed back then,
but now we have a function called `processColorInProps` which does
exactly that + it doesn't interfere with returned values of
`interpolateColor`. Taking all these things into consideration, I
believe this check in `rgbaColor` function is not needed anymore.

Moreover, it creates problems. When `interpolateColor` gets called on UI
thread (for example by using it inside `useDerivedValue` hook as shown
by this issue:
#6119),
the user gets these number-formatted colors which don't work when passed
anywhere.

## Test plan

Copy `App.tsx` from snack linked to the mentioned issue and make sure
the text stays blue. Also, check out Color Interpolation example from
the example app.

Tested on iOS and Android (both Paper and Fabric) as well as Web. I've
noticed neither issues nor regressions.
@szydlovsky
Copy link
Contributor

Together with merge of the mentioned PR, the issue should be gone in the nearest nightly version (probably tomorrow morning) or in a next stable one - 3.14 or next iteration of 3.13.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snippet of code, snack or repo is provided Reproducible 🎉
Projects
None yet
Development

No branches or pull requests

2 participants