On Sun, 27 Jul 2025 18:48:18 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> JavaFX unnecessarily restricts interpolation in the following ways: >> 1. `Interpolatable` implementations often clamp intermediate values to the >> interpolation factor range [0,1]. >> 2. `SplineInterpolator` doesn't accept Y coordinates outside of [0,1] for >> its control points. While this was probably done so that the computed >> interpolation factor doesn't exceed [0,1], the restriction goes far beyond >> that. For example, the following function is not representable, even though >> its values are all within the [0,1] range:<br> >> <img >> src="https://github.com/user-attachments/assets/368b6142-052d-4ead-8a59-cbddf4a19660"; >> width="400"/><br> >> The following function is also not representable, but would be very >> useful for [bouncy animations](https://easings.net/#easeOutBack):<br> >> <img >> src="https://github.com/user-attachments/assets/af02b044-ae4c-4250-b181-72178ad9f3f3"; >> width="400"/> >> >> Fortunately, there is no technical reason why JavaFX can't support the full >> range of animations that can be represented with a cubic Beziér >> interpolation function. >> >> This PR includes the following changes: >> 1. The specification of `Interpolatable` is changed to require >> implementations to accept interpolation factors outside of [0,1]. >> 2. All implementations of `Interpolatable` now correctly return intermediate >> values outside of [0,1]. >> 3. `SplineInterpolator` now accepts control points with any Y coordinate. >> >> Here's how the result looks like for the previously unrepresentable >> interpolation function `cubic-bezier(0.34, 2.2, 0.64, 1)`:<br> >> <img >> src="https://github.com/user-attachments/assets/72c10d0d-71b4-4bb5-b58c-ae377279b0fd"; >> width="500"/> > > 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 three additional > commits since the last revision: > > - Merge branch 'master' into feature/relaxed-interpolation > - javadoc > - Allow interpolation outside of range [0,1] looks good; a few minor comments. modules/javafx.graphics/src/main/java/com/sun/javafx/scene/layout/region/Margins.java line 79: > 77: Objects.requireNonNull(endValue, "endValue cannot be null"); > 78: > 79: if (t == 0 || equals(endValue)) { the class javadoc says * If proportional is true, then the values represent fractions or percentages * and are in the range 0..1, although this is not enforced. should this be changed/clarified? modules/javafx.graphics/src/main/java/com/sun/javafx/scene/layout/region/Margins.java line 92: > 90: > 91: return new Margins( > 92: Math.max(0, InterpolationUtils.interpolate(top, endValue.top, > t)), what is the reason for clamping this at 0, but not 1.0 ? also, why clamp here but not in `Insets` ? modules/javafx.graphics/src/main/java/javafx/scene/paint/RadialGradient.java line 251: > 249: * @param cycleMethod cycle method applied to the gradient > 250: * @param stops the gradient's color specification > 251: * @throws IllegalArgumentException if {@code focusDistance} or > {@code radius} is negative can we use the `@since` tag applicable to `@throws` only? (also below) modules/javafx.graphics/src/main/java/javafx/scene/paint/RadialGradient.java line 613: > 611: if (distance < 0) { > 612: throw new IllegalArgumentException( > 613: "Invalid gradient specification: focus-distance > cannot be negative"); why wrap here? also below modules/javafx.graphics/src/test/java/test/javafx/scene/paint/RadialGradientTest.java line 291: > 289: try { > 290: RadialGradient.valueOf("radial-gradient(radius -100, red 0%, > blue 30%, black 100%)"); > 291: fail("IllegalArgument should have been thrown."); nice: these tests do more than a simple `assertThrows` would do. modules/javafx.graphics/src/test/java/test/javafx/scene/paint/RadialGradientTest.java line 302: > 300: } catch (IllegalArgumentException iae) { > 301: assertTrue(iae.getMessage().contains("focus-distance cannot > be negative")); > 302: } `RadialGradient` adds `checkInvariants` in three places, but only two tests are added. Should we add one more? ------------- Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1822#pullrequestreview-3169216858 PR Review Comment: https://git.openjdk.org/jfx/pull/1822#discussion_r2310509351 PR Review Comment: https://git.openjdk.org/jfx/pull/1822#discussion_r2310519598 PR Review Comment: https://git.openjdk.org/jfx/pull/1822#discussion_r2310582888 PR Review Comment: https://git.openjdk.org/jfx/pull/1822#discussion_r2310586598 PR Review Comment: https://git.openjdk.org/jfx/pull/1822#discussion_r2310595057 PR Review Comment: https://git.openjdk.org/jfx/pull/1822#discussion_r2310611660
