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

useSharedValue initial double initialization #3199

Closed
wants to merge 2 commits into from

Conversation

piaskowyk
Copy link
Member

Description

// todo

@ranaavneet
Copy link
Sponsor

Any activity on this? This should be merged in the main branch

@piaskowyk piaskowyk changed the title useSharedValue initial value useSharedValue initial double initialization Dec 19, 2022
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.

As far as I remember this has been added to help with hot reload scenarios. I'm fine with changing this but we need to verify that this change does not break the hot reload stuff. Worst case we can make it use DEV flag to only use null in release (another possibility is by using init function instead of providing value directly, but this may also impact that behavior).

The goal is for the useShareValue work the same way useState does when it comes to hot reload. Specifically, say that you have a component that look sth like:

function Something() {
  const vv = useSharedValue(0.5);
  const stylez = useAnimatedStyle(() => { return { opacity: vv.value } });
  return (
    <Animated.View style={[{width:50, height: 50, backgroundColor: 'red'}, stylez]}>
      <Button title="go" onPress={() => { vv.value = 1 }}/>
    </Animated.View>
  )
}

Then after rendering it, you click a button which should make the value update to 1 and the opacity of the view should change. Now, when you change implementation of useAnimatedStyle such that it uses vv to also update a transform, for example:

useAnimatedStyle(() => { return { opacity: vv.value, transform: [{scale: 2 * vv.value }] } });

And if you have hot reload enabled, this should cause the UI to updates as soon as you hit save. As a result you should see the styles being updated while the value remains at 1.

However, if you now change the initial value provided to useSharedValue like so:

const vv = useSharedValue(0.2);

And hit save, the hot reload should reset the shared value to the newly provided initial value and UI should update such that the view will became transparent again and it will scale down to 0.4 of the original scale.

The above matches the behavior of useState hook and in general is pretty convinient in development mode when you often try to adjust initial value – in such a scenario you expect the new initial value to be taken into account, however, when you adjust other parts of the component you don't expect hot reload to reset the value state to initial.

@piaskowyk piaskowyk assigned piaskowyk and unassigned piaskowyk Dec 20, 2022
@amadeus
Copy link
Contributor

amadeus commented Nov 14, 2023

I was pointed to this PR from another Software Mansion dev, and one general concern I have is that calling makeMutable in every render call could really add up in cost in cases where lots of useSharedValues are being used.

What about an alternate of using the initializer for useState instead? Something like:

const [mutable] = useState(() => makeMutable(init, oneWayReadsOnly));

I assume this will solve both the hot reloading problems and prevent unnecessary JSI calls in big app re-renders. I'd be happy to make a PR that tests this out.

(here's a quick playground that shows you can change the default value of useState and it will blow away the existing value if it's been modified: https://codesandbox.io/s/musing-zeh-7rs8f6?file=/src/App.tsx )

@amadeus
Copy link
Contributor

amadeus commented Nov 14, 2023

Follow up note: We (Discord) are working on some stuff that uses MANY shared values, and i have a hunch we are seeing some effects in re-renders from this. I'm going to test an change like this internally and see how it goes, but yeah, in the meantime I'll go ahead and create a PR for this shortly.

@ranaavneet
Copy link
Sponsor

ranaavneet commented Nov 23, 2023

Hey @piaskowyk @amadeus @kmagiera just bumping this up, as our app also uses a lot of shared values in a single screen. And this could really help in performance of lower end Androids

@amadeus
Copy link
Contributor

amadeus commented Dec 2, 2023

Sorry, been a bit busy lately, I just created a PR here though: #5458

@piaskowyk
Copy link
Member Author

Closed due to: #5458

@piaskowyk piaskowyk closed this Dec 5, 2023
github-merge-queue bot pushed a commit that referenced this pull request Dec 6, 2023
## Summary
The general idea here is that we can avoid the `makeMutable` call on
every single render. This change is also compatible with hot reloading.

We (at Discord) have been running this patch on top of our own
reanimated now for a few weeks and haven't noticed any side effects of
the change.

There has been a bit of additional discussion I've seen surrounding this
issue here:
#3199

## Test plan
N/A
Latropos pushed a commit that referenced this pull request Dec 12, 2023
## Summary
The general idea here is that we can avoid the `makeMutable` call on
every single render. This change is also compatible with hot reloading.

We (at Discord) have been running this patch on top of our own
reanimated now for a few weeks and haven't noticed any side effects of
the change.

There has been a bit of additional discussion I've seen surrounding this
issue here:
#3199

## Test plan
N/A
@piaskowyk piaskowyk deleted the @piaskowyk/sharedValue-init branch June 20, 2024 13:40
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

4 participants