RFR: 8332895: Support interpolation for backgrounds and borders [v40]

[Kevin Rushforth]

On Wed, 25 Sep 2024 02:16:27 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> This PR completes the CSS Transitions story (see #870) by adding interpolation support for backgrounds and borders, making them targetable by transitions.
>> 
>> `Background` and `Border` objects are deeply immutable, but not interpolatable. Consider the following `Background`, which describes the background of a `Region`:
>> 
>> 
>> Background {
>>     fills = [
>>         BackgroundFill {
>>             fill = Color.RED
>>         }
>>     ]
>> }
>> 
>> 
>> Since backgrounds are deeply immutable, changing the region's background to another color requires the construction of a new `Background`, containing a new `BackgroundFill`, containing the new `Color`.
>> 
>> Animating the background color using a CSS transition therefore requires the entire Background object graph to be interpolatable in order to generate intermediate backgrounds.
>> 
>> More specifically, the following types will now implement `Interpolatable`.
>> 
>> - `Insets`
>> - `Background`
>> - `BackgroundFill`
>> - `BackgroundImage`
>> - `BackgroundPosition`
>> - `BackgroundSize`
>> - `Border`
>> - `BorderImage`
>> - `BorderStroke`
>> - `BorderWidths`
>> - `CornerRadii`
>> - `Stop`
>> - `Paint` and all of its subclasses
>> - `Margins` (internal type)
>> - `BorderImageSlices` (internal type)
>> 
>> ## Interpolation of composite objects
>> 
>> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of these classes is an aggregate of `double` values, which are combined using linear interpolation. However, many of the new interpolatable classes comprise of not only `double` values, but a whole range of other types. This requires us to more precisely define what we mean by "interpolation".
>> 
>> Mirroring the CSS specification, the `Interpolatable` interface defines several types of component interpolation:
>> 
>> | Interpolation type | Description |
>> |---|---|
>> | default | Component types that implement `Interpolatable` are interpolated by calling the `interpolate(Object, double)}` method. |
>> | linear | Two components are combined by linear interpolation such that `t = 0` produces the start value, and `t = 1` produces the end value. This interpolation type is usually applicable for numeric components. |
>> | discrete | If two components cannot be meaningfully combined, the intermediate component value is equal to the start value for `t < 0.5` and equal to the end value for `t >= 0.5`. |
>> | pairwise | Two lists are combined by pairwise interpolation. If the start list has fewer elements than the target list, the...
>
> Michael Strauß has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 63 commits:
> 
>  - fix merge conflicts
>  - Merge branch 'master' into feature/interpolatable
>    
>    # Conflicts:
>    #	modules/javafx.graphics/src/test/java/test/javafx/geometry/InsetsTest.java
>    #	modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundFillTest.java
>    #	modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundImageTest.java
>    #	modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundPositionTest.java
>    #	modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundSizeTest.java
>    #	modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundTest.java
>    #	modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BorderStrokeTest.java
>    #	modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BorderTest.java
>    #	modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BorderWidthsTest.java
>    #	modules/javafx.graphics/src/test/java/test/javafx/scene/paint/ColorTest.java
>    #	modules/javafx.graphics/src/test/java/test/javafx/scene/paint/ImagePatternTest.java
>    #	modules/javafx.graphics/src/test/java/test/javafx/scene/paint/LinearGradientTest.java
>    #	modules/javafx.graphics/src/test/java/test/javafx/scene/paint/RadialGradientTest.java
>    #	modules/javafx.graphics/src/test/java/test/javafx/scene/paint/StopListTest.java
>    #	modules/javafx.graphics/src/test/java/test/javafx/scene/paint/StopTest.java
>  - javadoc change
>  - javadoc change
>  - javadoc change
>  - small doc changes, copyright header
>  - fixed a bug when null values require discrete transitions
>  - javadoc changes
>  - added tests
>  - javadoc changes
>  - ... and 53 more: https://git.openjdk.org/jfx/compare/29004352...3027caa5

This looks really good and is almost ready to go in. Since the implementation has been heavily reviewed already, I'll limit my review to the API and do some light testing.

I left a few comments on the API and docs inline. In addition, should `ImagePattern::getImage` specify an interpolation type of "discrete"? The docs for that method weren't updated.

As soon as these issues are addressed, I'll re-review and also Review the CSR.

modules/javafx.graphics/src/main/java/javafx/animation/Interpolatable.java line 43:

> 41:  *         <tr><td style="vertical-align: top"><a id="linear" style="white-space: nowrap">linear</a></td>
> 42:  *             <td>Two components are combined by linear interpolation such that {@code t = 0} produces
> 43:  *                 the start value, and {@code t = 1} produces the end value. This interpolation type

Minor: I might add something like: `and {@code 0 < t < 1} produces {@code (1 - t) * start + t * end}`, to clearly define what linear means. This would be OK to add in a follow-up.

modules/javafx.graphics/src/main/java/javafx/scene/layout/BackgroundFill.java line 53:

> 51:      *
> 52:      * @return the Paint to use for filling the background of the {@link Region}
> 53:      * @interpolationType see {@link BackgroundFill}

There isn't any mention of how interpolation is done in the class docs of this class. Did you mean to add something to the class docs, or should it link to `Paint` instead?

-------------

PR Review: https://git.openjdk.org/jfx/pull/1522#pullrequestreview-2334313772
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1779022695
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1778964247