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

Add CatmullRomCurve and CatmullRomSpline #47547

Merged
merged 17 commits into from Jan 8, 2020

Conversation

gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Dec 20, 2019

Description

This adds CatmullRomCurve animation curve, and a CatmullRomSpline, which is what it uses to do interpolation.

This allows us to create animation curves which can smoothly interpolate the values given to the curve.

Here's an example program that uses these curves, and lets you edit them to see their properties. (this is not a permanent location for this program, and eventually the goal is for it to be available in the API docs).

Since I've introduced a 2D spline curve, I also created a Curve2D base class for such parametric curves.

Tests

  • Added tests for the curves
  • Added benchmark test for curves and for Cubic curve too.

Breaking Change

  • No, this is not a breaking change.

@fluttergithubbot fluttergithubbot added framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. labels Dec 20, 2019
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

I did not review the math behind this :)

packages/flutter/lib/src/animation/curves.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/animation/curves.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/animation/curves.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/animation/curves.dart Outdated Show resolved Hide resolved
packages/flutter/test/animation/curves_test.dart Outdated Show resolved Hide resolved
}

@override
String toString() => '$runtimeType';
Copy link
Member

Choose a reason for hiding this comment

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

runtimeType.toString is very slow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but I'm not sure what else I would want to print here, and overriding it in each class to a string literal is error prone. It's generally only ever used in debug mode.

Isn't it just as slow to not override toString and have it print "Instance of MyType"?

@gspencergoog gspencergoog force-pushed the checkbox_density branch 3 times, most recently from 8ff4b86 to 9d9b573 Compare January 7, 2020 18:23
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM, but maybe @HansMuller could also take a look since he seems to be more of a domain expert :)

packages/flutter/lib/src/animation/curves.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/animation/curves.dart Outdated Show resolved Hide resolved
@gspencergoog gspencergoog force-pushed the checkbox_density branch 2 times, most recently from ec35d06 to 298d78d Compare January 7, 2020 19:29
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM

packages/flutter/lib/src/animation/curves.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/animation/curves.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/animation/curves.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/animation/curves.dart Outdated Show resolved Hide resolved
Copy link
Contributor Author

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

Also, note that I added the example that we talked about with the flying "B".

packages/flutter/lib/src/animation/curves.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/animation/curves.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/animation/curves.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/animation/curves.dart Outdated Show resolved Hide resolved
@gspencergoog gspencergoog merged commit 8b13201 into flutter:master Jan 8, 2020
bkonyi pushed a commit that referenced this pull request Jan 9, 2020
This adds CatmullRomCurve animation curve, and a CatmullRomSpline, which is what it uses to do interpolation.

This allows us to create animation curves which can smoothly interpolate the values given to the curve.

Since I've introduced a 2D spline curve, I also created a Curve2D base class for such parametric curves.
@gspencergoog gspencergoog deleted the checkbox_density branch January 16, 2020 22:10
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants