-
Notifications
You must be signed in to change notification settings - Fork 546
8372203: Piecewise linear easing function #1977
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
base: master
Are you sure you want to change the base?
Conversation
|
👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@mstr2 has indicated that a compatibility and specification (CSR) request is needed for this pull request. @mstr2 please create a CSR request for issue JDK-8372203 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
Webrevs
|
andy-goryachev-oracle
left a comment
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.
Could you provide an example (in the JBS maybe) of a CSS that illustrates the usage? Or maybe add an example to the javadoc?
|
|
||
| // Ensure that the input progress value of each control point is greater than or equal to the | ||
| // input progress values of all preceding control points (monotonically non-decreasing). | ||
| double largestX = controlPoints[0]; |
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.
Question: what happens when a sequence is specified which is somehow invalid? Will it throw an exception, write to stderr, or silently ignore?
(I can't think of an invalid sequence, maybe 0, 0 0% 0%, 0 -10%, NaN ?)
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.
The sequence will always be successfully canonicalized, so there's never a log output or exception. In your example, the canonicalized easing function will be 0 for t=0, and NaN for t>0. If you use this function in an animation, you may get weird values and undefined things may happen. This is not unique to easing functions: we almost never handle NaNs in JavaFX. For example, you can construct a Color with NaN components (which is what would happen if you used your easing function in a color transition).
| } | ||
|
|
||
| // Linearly interpolate (or extrapolate) along the segment (ax, ay) -> (bx, by). | ||
| if (ax == bx) { |
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.
FP alert: is it possible for ax != bx yet the result of division on L203 to produce an infinity?
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.
jshell> 1 / 9e-310
$25 ==> Infinity
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 added code to ensure that when an infinity is unavoidable, we at least get an infinity consistent with the line (and not, say, NaN). While this makes this interpolator locally consistent, we may still end up passing infinities into the animation system, so it's not a catch-all solution (that would be a completely different thing that we're not going to solve in interpolators).
| private static double[] getControlPoints(LinearInterpolator li) { | ||
| try { | ||
| Field f = LinearInterpolator.class.getDeclaredField("controlPoints"); | ||
| f.setAccessible(true); |
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.
should an Accessor be used instead?
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'm not a big fan of that because of its impact on the source code of the class (you know those comments: "package-private only for the sake of testing").
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 would recommend to use the Accessor pattern then.
Reflection is brittle, the IDEs won't track the dependency, and I prefer clear and obvious dependencies. Besides, this is the pattern used throughout the JavaFX code.
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 realized it's only for testing. It's probably ok, for testing. Maybe add a comment to the field saying that the reflection is used to access this field by tests?
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 think I'll leave it as is, because it's covered by tests. It won't go unnoticed when someone tries to change the name of the field, and adjusting the unit tests is trivial.
I've added three examples of linear easing functions to cssref.html, do you think there's something missing? |
|
Maybe we should retire the weird naming scheme used in this class (uppercasing the name of methods), or at least not proliferate it any further. So instead of |
|
Could you provide a complete selector for the example in this PR description please, so I can copy and paste it? Similarly, do you think it's worth adding the same (complete selector example) to the class javadoc? |
+1 for
|
Here it is, for a region with the .rect {
-fx-min-width: 150;
-fx-min-height: 50;
-fx-background-color: purple;
transition-property: -fx-min-width, -fx-background-color;
transition-duration: 2s;
transition-timing-function: linear(0, 0.063, 0.25, 0.563, 1 36.4%,
0.812, 0.75, 0.813, 1 72.7%,
0.953, 0.938, 0.953, 1 90.9%,
0.984, 1 100% 100%);
}
.rect:hover {
-fx-min-width: 400;
-fx-background-color: green;
}I've also added this example to tests/manual/graphics/CssTransitionsTest.
No, mainly because the |
Thank you very much, this makes the life of the reviewers so much easier! |
andy-goryachev-oracle
left a comment
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.
Looks good, left a couple of suggestions.
Used .button selector with transition in the monkey tester, with hilarious results.
| return ay; | ||
| } | ||
|
|
||
| // Linearly interpolate (or extrapolate) along the segment (ax, ay) -> (bx, by). |
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.
Alternative proposal: instead of introducing infinities, we probably should just arrive at the end-of-segment value when the ax ~= bx, something like this:
// Linearly interpolate (or extrapolate) along the segment (ax, ay) -> (bx, by).
if (isNear(ax, bx)) {
// Degenerate segment; just treat as a step.
return ay;
}
where isNear is Math.abs(ax - bx) < SMALL_CONSTANT
(can be inline), unless the value of SMALL_CONSTANT depends on the interval.
what do you think?
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 found that the algorithm in the specification is quite a bit simpler, and it passes all tests, so I've changed the implementation to match this algorithm. It's also exactly what is implemented by Chromium.
| * @return a new piecewise-linear interpolator | ||
| * @since 26 | ||
| */ | ||
| public static Interpolator LINEAR(Point2D... controlPoints) { |
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.
as you pointed out earlier, it's probably better to name this method ofLinear() since it is not a final constant.
andy-goryachev-oracle
left a comment
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.
Looks good!
Turns out the fears about infinities are basically unfounded - it appears the CssParser does not like scientific notation, and I could not get it to trip with a very small decimal step.
Example:
transition-timing-function: linear(0, 1.4E-45, 0.063, 0.25, 0.563, 1 36.4%,
0.812, 0.75, 0.813, 1 72.7%,
0.953, 0.938, 0.953, 1 90.9%,
0.984, 1 100% 100%);
bails out with
Unexpected token '1.4E' at [7,40]
Nov 25, 2025 7:47:46 AM javafx.css.CssParser reportException
WARNING: Please report java.lang.NullPointerException at:
javafx.graphics/javafx.css.CssParser.term(CssParser.java:5127)
javafx.graphics/javafx.css.CssParser.expr(CssParser.java:5021)
|
|
||
| /** | ||
| * Creates a piecewise-linear interpolator with the specified control points. | ||
| * Returns a piecewise-linear interpolator with the specified control points. |
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.
Question: do we need to specify that a new instance is returned each time (as opposed to some sort of caching as with Integer.valueOf)?
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 changed the wording here to give us more flexibility in terms of how we implement this in the future. While we are returning a new instance each time, we don't need to: all our currently implemented interpolators are stateless and safe to share, even across threads.
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.
is this something worth mentioning in the javadoc, at least for the linear interpolator?
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 think usually only the stronger guarantee is mentioned explicitly (that it is a new instance).
kevinrushforth
left a comment
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.
The API and docs look good. I left a couple questions on the ofLinear method.
| * distributed evenly between its neighboring control points. If the input progress value of | ||
| * the first or last control point is unspecified, it is set to 0 or 1, respectively. | ||
| * | ||
| * @param controlPoints the control points |
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.
Do the control points need to be sorted by their X values (excluding NaN)? The treatment of NaN values regarding of "neighboring control points" suggests that it should be in order for it to be sensible, but perhaps the algorithm doesn't require it?
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.
The answer to this question depends on what you mean by "need to be sorted". It's certainly possible to specify control points in any order, but the algorithm won't sort them. Instead, it modifies their input progress values (X coordinate) such that it never decreases. Here's the relevant part of the specification:
If any control point has an input progress value that is less than the input progress value of any preceding control point, set its input progress value to the largest input progress value of any preceding control point.
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.
Do you think it is worth adding similar language here about what the behavior is if an input progress value is less than any preceding input progress values?
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.
That's a good idea, I've added a paragraph that explains the behavior.
| * | ||
| * @param controlPoints the control points | ||
| * @throws NullPointerException if {@code controlPoints} is {@code null} | ||
| * @throws IllegalArgumentException if {@code controlPoints} is empty |
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.
Should it throw IAE if length < 2?
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.
The CSS syntax is specified as linear( [ <number> && <percentage>{0,2} ]# ), where # means "one or more". So a single point is allowed.
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 noticed that the text in cssref.html says "two or more", not "one or more". I'm going to change that.
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.
Thanks. That's why I asked whether < 2 should be IAE. Fixing the cssref is good.
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'm going to correct myself once more. It seems like this has changed between spec revisions, and per the final spec, even though the syntax uses the "one or more" modifier, the parsing algorithm adds the following restriction:
If there are less than two items in stopList, then return failure.
So I'm going to change that again, and add an appropriate IAE if length < 2.
| if (controlPoints == null) { | ||
| throw new NullPointerException("controlPoints cannot be null"); | ||
| } |
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.
You can use Objects.requireNonNull.
| /** | ||
| * Returns a piecewise-linear interpolator with the specified control points. | ||
| * <p> | ||
| * Each control point associates an input progress value (X) with an output progress value (Y). |
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 think an explanation of what these progress values are will help, especially with names such as "input" and "output" associated with them instead of the x, y coordinate value names one could expect.
Implementation of the linear easing function, which is now widely supported by all browsers, but still missing in JavaFX.
It allows developers to approximate arbitrary easing functions with linear segments:
/reviewers 2
/csr
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1977/head:pull/1977$ git checkout pull/1977Update a local copy of the PR:
$ git checkout pull/1977$ git pull https://git.openjdk.org/jfx.git pull/1977/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1977View PR using the GUI difftool:
$ git pr show -t 1977Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1977.diff
Using Webrev
Link to Webrev Comment