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

Tue Sep 3 03:28:31 UTC 2024

On Sun, 1 Sep 2024 12:31:06 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 incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 48 additional commits since the last revision:
> 
>  - Merge branch 'master' into feature/interpolatable
>  - remove StyleConverter.WithReconstructionSupport
>  - fix line separators
>  - StyleableStringProperty should be transitionable
>  - non-interpolatable values should always transition discretely
>  - only call get() when necessary
>  - add more documentation
>  - replace reconstruction annotation with interface
>  - interpolate integers in real number space
>  - replace StyleConverter.SupportsDeconstruction interface with annotation
>  - ... and 38 more: https://git.openjdk.org/jfx/compare/ca9f51c5...2337ca98

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

> 55:  *             </td>
> 56:  *         </tr>
> 57:  *         <tr><td style="white-space: nowrap; vertical-align: top">(see prose)</td>

This could just be a comment below the table?  Not sure what "(see prose)" means here.

modules/javafx.graphics/src/main/java/javafx/css/StyleableLongProperty.java line 137:

> 135:         public void onUpdate(double progress) {
> 136:             // Longs are interpolated in real number space and rounded to the nearest long.
> 137:             set(progress < 1 ? Math.round(startValue + (endValue - startValue) * progress) : endValue);

I don't think this will produce correct start values for all longs, and it may produce values outside [start,end] range I think.

Here is a bad start value for example:

    double progress = 0;
    long startValue = Long.MAX_VALUE - 1;
    long endValue = 0;

    System.out.println(progress < 1 ? Math.round(startValue + (endValue - startValue) * progress) : endValue);
    System.out.println(startValue);

The calculated value will be `Long.MAX_VALUE` which is outside the [startValue, endValue] range.

Furthermore, when the longs are large (>49 bits), then interpolation will be terrible:

    public static void main(String[] args) {
      long startValue = 1000000000000000000L;
      long endValue =   1000000000000000100L;

      for (double progress : new double[] {0.0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 1.0}) {
        System.out.println(progress < 1 ? Math.round(startValue + (endValue - startValue) * progress) : endValue);
      }
    }

Prints:

1000000000000000000
1000000000000000000
1000000000000000000
1000000000000000000
1000000000000000000
1000000000000000000
1000000000000000000
1000000000000000128
1000000000000000128
1000000000000000128
1000000000000000128

I think if you make sure that the subtraction and addition are done as `long` (instead of as `double`) it will be far more accurate:

       long diff = endValue - startValue;
       long result = startValue + Math.round(progress * diff);

You still will need to ensure the result is clamped I think, and I you'll need to make an exception for `1.0` as it is the result of a double calculation + rounding which can be off.  The start value should always be correct I think, even if `progress` is some tiny value like `0.0000000000000000001`.

    public static void main(String[] args) {
        long startValue = 1000000000000000333L;
        long endValue = 2000000000000000100L;

        for (double progress : new double[] {0.0000000000000000001, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 1.0}) {
            long diff = endValue - startValue;
            long interpolatedValue = startValue + Math.round(progress * diff);
            System.out.println(interpolatedValue);
        }
    }

Prints:

1000000000000000333
1100000000000000317
1200000000000000301
1300000000000000269
1400000000000000269
1500000000000000205
1600000000000000205
1700000000000000077
1800000000000000205
1900000000000000077
2000000000000000077

So the final `1.0` is not calculated correctly, and so should be handled as a special case.

modules/javafx.graphics/src/main/java/javafx/css/TransitionEvent.java line 112:

> 110:         super(Objects.requireNonNull(eventType, "eventType cannot be null"));
> 111:         this.property = Objects.requireNonNull(property, "property cannot be null");
> 112:         this.propertyName = Objects.requireNonNull(propertyName, "propertyName cannot be null");

Can this be different from `property.getCssMetadata().getProperty()`?  What does it mean if it is?

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1741355798
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1741361742
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1741376154