On Fri, 2 Aug 2024 01:39:11 GMT, Michael Strauß <mstra...@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 incrementally with two additional 
> commits since the last revision:
> 
>  - fix since tag
>  - adjust table styling

Thank you for this contribution!

This is the second batch of code review comments.  Will probably have more 
after testing.

modules/javafx.graphics/src/main/java/javafx/scene/paint/Stop.java line 162:

> 160:         // re-allocating when we run out of space. In general, we expect 
> the size of stop lists
> 161:         // to be quite small (a single-digit number of stops at most).
> 162:         Stop[] stops = new Stop[firstList.size() + secondList.size()];

are there unit tests to cover most of the scenarios, at least using short 
sequences?

modules/javafx.graphics/src/test/java/test/javafx/scene/paint/StopListTest.java 
line 264:

> 262:                 new Stop(1, color2)),
> 263:             res);
> 264:         }

Do you think any other combinations should be added, for completeness sake?

For example, let's consider two lists with 2 points each.  We might want to 
test scenarios:


--
     --

--|
  |--

---
 ---

----|
 ---|

------
  --

  |-----
  |--

  |---|
  |---|

    |--
  --|

     ---
--


and also have the two lists swapped maybe?

what do you think?

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

PR Review: https://git.openjdk.org/jfx/pull/1522#pullrequestreview-2215836542
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702023646
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702046900

Reply via email to