RFR: 8332895: Support interpolation for backgrounds and borders [v3]
John Hendrikx
jhendrikx at openjdk.org
Fri Aug 2 22:36:57 UTC 2024
On Fri, 2 Aug 2024 01:39:11 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 incrementally with two additional commits since the last revision:
>
> - fix since tag
> - adjust table styling
I only managed to get partially through this, it's a lot of changes. Left some comments so far.
modules/javafx.base/src/main/java/com/sun/javafx/UnmodifiableArrayList.java line 78:
> 76:
> 77: int index = 0;
> 78: int numNonNullValues = 0;
It seems to me, just one of these variables is sufficient. They both start at `0` and both are incremented at the same time.
modules/javafx.base/src/main/java/com/sun/javafx/UnmodifiableArrayList.java line 81:
> 79:
> 80: @SuppressWarnings("unchecked")
> 81: T[] newValues = (T[])new Object[list.size()];
With many `null`s this array may be too big. If that's intended, then ignore this comment.
modules/javafx.base/src/main/java/com/sun/javafx/UnmodifiableArrayList.java line 123:
> 121: if (elements[i] != null) {
> 122: newValues[j++] = elements[i];
> 123: ++numNonNullValues;
Both `j` and `numNonNullValues` are always the same. Could eliminate one.
modules/javafx.graphics/src/main/java/com/sun/javafx/scene/paint/PaintUtils.java line 102:
> 100: source.getEndX(), source.getEndY(),
> 101: source.isProportional(),
> 102: source.getCycleMethod(),
Should these be copied from the source? When the gradient is solid, they could be set to constants.
modules/javafx.graphics/src/main/java/com/sun/javafx/util/Utils.java line 183:
> 181: * <li>If the intermediate list is shallow-equal to the first list passed into the method (i.e. its
> 182: * elements are references to the same objects), the existing list is returned.
> 183: * <li>If a new list is returned, it is unmodifiable.
Well, it's documented, but I'd rather see this return an unmodifiable list in all cases (and not one of the input lists if it was determined to be modifiable). Perhaps ensure this is compatible with `List.copyOf` so that copies are only made when needed.
modules/javafx.graphics/src/main/java/com/sun/javafx/util/Utils.java line 197:
> 195: if (secondList.isEmpty()) {
> 196: return firstList.isEmpty() ? firstList : secondList;
> 197: }
This logic looks odd. If the second list is empty, we return the first list **if** it is empty (and so empty is returned), otherwise the second list (which is also empty). So, this is equivalent?
Suggestion:
if (secondList.isEmpty()) {
return List.of(); // suggested here, as the two input lists are not guaranteed to be immutable
}
modules/javafx.graphics/src/main/java/com/sun/javafx/util/Utils.java line 230:
> 228: * shallow-equal to the list that is passed into this method, i.e. its elements are references
> 229: * to the same objects. The existing list is returned in this case.
> 230: */
Was there a performance problem here?
Allocating 8 locals should be equivalent to an array of 8 elements that never escapes (ie. JIT optimizes this). Then either JIT is smart enough to conditionally allocate a heap object and copy the array there, or you can help it by cloning that local array and letting only the clone escape.
modules/javafx.graphics/src/main/java/com/sun/javafx/util/Utils.java line 311:
> 309: }
> 310: }
> 311: }
You could write this without nesting using `switch`:
switch(listSize) {
case 8: newArray[7] = item7; // fallthrough
case 7: newArray[6] = item6;
case 6: newArray[5] = item5;
// etc
}
modules/javafx.graphics/src/main/java/com/sun/javafx/util/Utils.java line 330:
> 328:
> 329: if (secondArraySeries.length == 0) {
> 330: return firstArraySeries.length == 0 ? firstArraySeries : secondArraySeries;
Same comment as on the other function. I find it suspect. Is there code relying on a **specific** array being returned? If so then that probably should be documented. Otherwise just immediately return `secondArraySeries`?
modules/javafx.graphics/src/main/java/javafx/css/ComponentTransitionable.java line 35:
> 33: * Identifies a class that supports component-wise CSS transitions.
> 34: * <p>
> 35: * Component-wise transitions offer more flexibility than {@link Interpolatable} transitions.
Why is the name not `ComponentInterpolatable`?
modules/javafx.graphics/src/main/java/javafx/css/StyleConverter.java line 268:
> 266: * @since 24
> 267: */
> 268: public Map<CssMetaData<? extends Styleable, ?>, Object> convertBack(T value) {
I know the name `convert` wasn't all that great (it probably should have been `construct`, `parse` or `deserialize`), but I think `convertBack` is not that great either. Have you considered `toMetaData`, `revert`, `serialize` or `deconstruct`?
modules/javafx.graphics/src/main/java/javafx/geometry/Insets.java line 117:
> 115: *
> 116: * @throws NullPointerException {@inheritDoc}
> 117: * @since 23
Should this be `24`?
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9020:
> 9018: if (timer != null) {
> 9019: if (result == null) {
> 9020: result = new HashMap<>(5);
I think that when you specify map sizes like this, it might be worth a comment at the intent. Did you intend to create a map that can hold 6 items before resizing? (5 becomes 8, which is sufficient for 6 mappings with a 75% load factor). If you want to express the intent more clearly, use `HashMap.newHashMap(5)`.
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9106:
> 9104:
> 9105: // Make a copy of the list, because completing the timers causes them to be removed
> 9106: // from the list, which would result in a ConcurrentModificationException.
Comment is out of date
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9107:
> 9105: // Make a copy of the list, because completing the timers causes them to be removed
> 9106: // from the list, which would result in a ConcurrentModificationException.
> 9107: for (TransitionTimer timer : Map.copyOf(transitionTimers).values()) {
If you only need a copy of the values, copying the whole map is a bit overkill. How about `List.copyOf(transitionTimers.values())` ? Lists are also much faster than maps for this purpose.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1522#pullrequestreview-2215813399
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701977895
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701979810
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701980936
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701997212
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702322235
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702318609
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702330147
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702331405
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702333480
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702335141
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702351819
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702368300
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702373249
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702373633
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702374898
More information about the openjfx-dev
mailing list