-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Remove non-layout style and prop updates path via synchronouslyUpdatePropsOnUIThread
#7014
Remove non-layout style and prop updates path via synchronouslyUpdatePropsOnUIThread
#7014
Conversation
synchronouslyUpdateUIPropsFunction_
call in performOperations
synchronouslyUpdatePropsOnUIThread
path for non-layout prop updates
synchronouslyUpdatePropsOnUIThread
path for non-layout prop updatessynchronouslyUpdatePropsOnUIThread
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.
LGTM, should we also merge/rework props lists in TS?
packages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.cpp
Outdated
Show resolved
Hide resolved
@tjzel We'll need to discuss this. There's no need to keep separate sets for UI and native props (since I've merged the logic of applying UI and native props) but I think we still need to expose |
@tomekzaw We can keep both these methods and mark one of them as deprecated. |
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please follow the template so that the reviewers can easily understand what the code changes affect. --> ## Summary This PR is very annoying. Until now we assumed that when `componentWillUnmount` is called, the component will never reappear. This is false when using `<Suspense>` ([docs](https://react.dev/reference/react/Suspense#caveats)), which is exactly what `react-freeze` does. To fix the issue I had to move our animation cleanup functions to be called only when we are sure that the component will not reappear. This is how the new approach works for each registry: - `AnimatedPropsRegistry`: we remove a record from this registry only when we fail to apply it onto the ShadowTree (in the commit hook or `perfromOperations`) - `CSSAnimationsRegistry`: we remove a record from this registry only when we fail to apply it onto the ShadowTree (in the commit hook or `perfromOperations`) - `CSSTransitionsRegistry`: we remove a record from this registry when `componentWillUnmount` is called and there is no ongoing transition OR we failed to apply it onto the ShadowTree (in the commit hook or `perfromOperations`) - `StaticPropsRegistry` remains the same as it doesn't store any state that needs to be persisted when freeze is used One problem with this approach is that we are running animations on components that are freezed. This is a waste of resources. This will be addressed in a separate PR, but the main idea is to pause animations in `componentWillUnmount` and resume them (with the new timestamp) in `compnentDidMount`. Also currently the leak check will say that `AnimatedPropsRegistry` leaked if we don't reload it. This is because an animation frame comes after the cleanup from the commit hook. Normally this would be cleaned up in `performOperations`, but because of the `sunchronouslyUpdateProps` fast-path it won't. The cleanup will still happen on the next commit hook invocation (i.e. if you hit reload on the leak check). This issue soon will be gone as we are removing the fast path in #7014. | Before | After | | -------- | ------- | | <video src="https://github.com/user-attachments/assets/249acaac-f1f0-4846-a38e-4dab54cfb49b"/> | <video src="https://github.com/user-attachments/assets/5d3c578b-fb80-45e8-9955-542bf47d0573"/> | ## Test plan Go through the app, and then visit the `FreezeExample`. Use the `check for registry leaks` button.
Motivation
Currently, there are two ways to update native view props and styles in Reanimated. The default path (so-called slow path) is to apply all props changes to the ShadowTree via C++ API and let React Native mount the changes. However, if all props updated in given batch are non-layout props (i.e. those that don't require layout recalculation, like background color or opacity) we use a fast path that calls
synchronouslyUpdatePropsOnUIThread
from React Native and applies the changes directly to platform views, without making changes to ShadowTree in C++. Turns out, some features like view measurement or touch detection system use C++ ShadowTree which is not consistent with what's currently on the screen. Because of that, we're removing the fast path (turns out it's not that fast, especially on iOS) to restore the correctness of view measurement and touch detection for animated components.Benchmarks
transform
prop usinguseAnimatedStyle
App.tsx
Summary
Test plan