-
-
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
Add new spring feature - clamp #5195
Conversation
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.
Left some syntax-related comments.
) { | ||
'worklet'; | ||
const { zeta, toValue, startValue } = animation; | ||
if (Number(toValue) === startValue) { |
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.
Why do we convert toValue
into a number here? Does it mean that we no longer support colors?
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.
I've just tested and it looks that color handling is a bit buggy with our main code 😱
Something like
const style = useAnimatedStyle(() => {
return {
backgroundColor: withSpring(randomColor.value, config),
};
});
where randomColor is a colorname like 'red' or 'blue' doesn't always run (on main!)
Anyway toValue is expected to be a number, since in src/reanimated2/animation/util.ts
we have a lot of functions like colorOnStart
and numberOnStart
that are expected to convert input to numbers and then join it again.
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.
Also if toValue
weren't number
it would be difficult wo handle it, since value is always a number.
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.
Btw. logic inside src/reanimated2/animation/util.ts
is very important and complicated, I think I'd suggest splitting it into multiple files, WDYT?
3a27ddd
to
7b5f66f
Compare
a475be0
to
2d533eb
Compare
Co-authored-by: Tomek Zawadzki <tomasz.zawadzki@swmansion.com>
This PR aims to fix the issue when Release builds would fail on Paper on iOS. The reason this regression happened is #5113. XCode ships by default to new projects `DEBUG` macro for Debug Configuration. Due to some issues (#4902) we decided to switch over to `NDEBUG` macro. However, the `NDEBUG` is not provided by XCode out of the box. Incidentally, it's provided by React Native which puts its install script into project's Podfile - however, [only in Fabric](facebook/react-native@421df9f). What is more, some time ago React Native made it so `PRODUCTION=1 pod install` is [no longer a necessary step](facebook/react-native@93fdcba) to make a release build, therefore we cannot rely on it. Until React Native defines `NDEBUG` for Paper too we have to detect, based on available options, whether we potentially have a Release build and this pull request does this. Thanks to #5383 we finally figures out how to do it the most agreeable way. We hardcode that for a build config named `Release` NDEBUG=1 will be set. If the user uses some custom release build config e.g. "MyRelease", NDEBUG=1 will not be set - in this situation we'll require the user to use `PRODUCTION=1`. Run all example apps (except TVOS cause I couldn't make it cooperate with me, and Web) on both iOS and Android in both Release and Debug and see if they work properly (e.g. add a throw in #ifndef fragment).
I've tested it and it seems to work 🎉 |
<!-- 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 > **warning** > DO NOT MERGE BEFORE #5195 GETS MERGED This change is needed since we have implemented one more config option for spring (clamp). The clamp PR: #5195 However documentation of spring config gets too cluttered. My suggestions: - Add minimum reanimated version supporting each config option - Split config table into three smaller tables (properties for duration-based spring, physical spring & common config properties) - Make it easier for the user to read only the interesting part of documentation (since it gets long). Maybe we can just add an example config for users, who don't care too much about details, or highlight the most important props. <!-- Explain the motivation for this PR. Include "Fixes #<number>" if applicable. --> ## Test plan Thats how it looks: ![image](https://github.com/software-mansion/react-native-reanimated/assets/56199675/030da0b4-1d0b-4c42-8d89-f3373ae38edc) <!-- Provide a minimal but complete code snippet that can be used to test out this change along with instructions how to run it and a description of the expected behavior. --> --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Aleksandra Cynk <aleksandracynk@Aleksandras-MacBook-Pro-3.local> Co-authored-by: Kacper Kapuściak <39658211+kacperkapusciak@users.noreply.github.com> Co-authored-by: joshlam <38872498+joshlam@users.noreply.github.com> Co-authored-by: Andrey <44743245+andreysam@users.noreply.github.com> Co-authored-by: Krzysztof Piaskowy <krzysztof.piaskowy@swmansion.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Summary
Add possibility to define limits that won't be crossed by bouncing spring.
In simplification we can assume that range of bounces is related just with damping ratio,
therefore we recalculate new damping ratio, that satisfies our expectation - the spring moves within its range.
Simplified alghoritm:
Examples
Two examples with different arbitrary chosen config.
Screen.Recording.2023-10-19.at.14.34.19.mov
Screen.Recording.2023-10-19.at.14.35.53.mov
Test plan
Resting example - code
Documentation