Skip to content

chore: Make CSS interpolators stateless #6993

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

Merged
merged 16 commits into from
Feb 18, 2025

Conversation

MatiPl01
Copy link
Member

@MatiPl01 MatiPl01 commented Feb 6, 2025

Summary

This PR removes, probably unnecessary, premature optimizations which added code complexity to interpolators implementation. Interpolators kept state of the current keyframe and last interpolated value. They also were tightly coupled with progress providers which must have been passed in the constructor.

Even though the previous implementation worked fine, it was impossible to optimize creation of the animation (we had to create interpolators for all views separately, even if they used the same animation). This also resulted in higher memory consumption.

Instead of moving left or right on the keyframes sequence, I now use a binary search to find the current keyframe. It may have worse time complexity (O(log(n)) instead of O(1)) but, in fact, may be even faster because of removed logic responsible for storing a previous value, iterating over keyframes, etc. This change shouldn't affect performance as long as somebody doesn't use thousands or keyframes in animations (which is rather impossible).

How it was tested?

I just went through examples in the example app (on both, iOS and Android) to make sure that no regression was introduced in this PR.

@MatiPl01 MatiPl01 self-assigned this Feb 6, 2025
@MatiPl01 MatiPl01 changed the base branch from main to parse-transition-shorthand February 6, 2025 17:18
@MatiPl01 MatiPl01 force-pushed the @matipl01/make-css-interpolators-stateless branch from 2429ada to d49b6e1 Compare February 10, 2025 16:06
@MatiPl01 MatiPl01 changed the title @matipl01/make css interpolators stateless chore: Make CSS interpolators stateless Feb 10, 2025
@MatiPl01 MatiPl01 marked this pull request as ready for review February 10, 2025 16:19
@MatiPl01 MatiPl01 requested a review from piaskowyk February 10, 2025 16:42
Base automatically changed from parse-transition-shorthand to main February 11, 2025 11:23
@MatiPl01 MatiPl01 force-pushed the @matipl01/make-css-interpolators-stateless branch 3 times, most recently from de7d7eb to 672f435 Compare February 15, 2025 21:43
Copy link
Member

@piaskowyk piaskowyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 I've just left a few small comments, but nothing really important. I'm glad that these changes have significantly improved the readability of the codebase.

@MatiPl01 MatiPl01 force-pushed the @matipl01/make-css-interpolators-stateless branch from 5a3f873 to c5c593f Compare February 18, 2025 16:51
@MatiPl01 MatiPl01 added this pull request to the merge queue Feb 18, 2025
Merged via the queue into main with commit e5a0cdf Feb 18, 2025
19 checks passed
@MatiPl01 MatiPl01 deleted the @matipl01/make-css-interpolators-stateless branch February 18, 2025 18:11
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.

2 participants