8234712: Add pivot properties for scale and rotation in Node, ScaleTransition and RotateTransition

[nlisker]

CSR will be submitted at a later phase.

---

Progress

• [ ] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue
• [ ] Change must be properly reviewed

Issue

• JDK-8234712: Add pivot properties for scale and rotation in Node, ScaleTransition and RotateTransition

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/53/head:pull/53
$ git checkout pull/53

Update a local copy of the PR:
$ git checkout pull/53
$ git pull https://git.openjdk.java.net/jfx pull/53/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 53

View PR using the GUI difftool:
$ git pr show -t 53

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/53.diff

[bridgekeeper[bot]]

:wave: Welcome back nlisker! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

[nlisker]

Visual test program:

import javafx.animation.Animation;
import javafx.animation.Interpolator;
import javafx.animation.KeyFrame;
import javafx.animation.KeyValue;
import javafx.animation.RotateTransition;
import javafx.animation.ScaleTransition;
import javafx.animation.Timeline;
import javafx.animation.TranslateTransition;
import javafx.application.Application;
import javafx.beans.binding.Bindings;
import javafx.geometry.Point3D;
import javafx.scene.Group;
import javafx.scene.Scene;
import javafx.scene.paint.Color;
import javafx.scene.shape.Circle;
import javafx.scene.shape.Rectangle;
import javafx.stage.Stage;
import javafx.util.Duration;

public class PivotTest extends Application {

    @Override
    public void start(Stage stage) {
        var scaleRect = new Rectangle(100, 50);
        scaleRect.setFill(new Color(1, 0, 0, 0.5));

        var rotRect = new Rectangle(100, 50);
        rotRect.setFill(new Color(0, 0, 1, 0.5));
        rotRect.setTranslateX(150);
        rotRect.setTranslateY(50);

        var bothRect = new Rectangle(100, 50);
        bothRect.setFill(new Color(0, 1, 0, 0.5));
        bothRect.setTranslateX(75);
        bothRect.setTranslateY(170);

        var rotPivot = new Circle(3);
        rotPivot.setFill(Color.BLUE);
        rotPivot.translateXProperty().bind(Bindings.createDoubleBinding(() ->
                        rotRect.getRotationPivot().getX() + rotRect.getTranslateX(),
                        rotRect.rotationPivotProperty(), rotRect.translateXProperty()));
        rotPivot.translateYProperty().bind(Bindings.createDoubleBinding(() ->
                        rotRect.getRotationPivot().getY() + rotRect.getTranslateY(),
                        rotRect.rotationPivotProperty(), rotRect.translateYProperty()));

        var scalePivot = new Circle(3);
        scalePivot.setFill(Color.RED);
        scalePivot.translateXProperty().bind(Bindings.createDoubleBinding(() ->
                        scaleRect.getScalePivot().getX() + scaleRect.getTranslateX(),
                        scaleRect.scalePivotProperty(), scaleRect.translateXProperty()));
        scalePivot.translateYProperty().bind(Bindings.createDoubleBinding(() ->
                        scaleRect.getScalePivot().getY() + scaleRect.getTranslateY(),
                        scaleRect.scalePivotProperty(), scaleRect.translateYProperty()));

        Group root = new Group(scaleRect, rotRect, rotPivot, scalePivot, bothRect);

        var trans = new TranslateTransition(Duration.seconds(3), scaleRect);
        trans.setToY(100);
        trans.setAutoReverse(true);
        trans.setCycleCount(Animation.INDEFINITE);
        trans.play();

        var scaleTimeline = new Timeline(
                new KeyFrame(Duration.seconds(0),
                        new KeyValue(scaleRect.scalePivotProperty(), new Point3D(25, 25, 0), Interpolator.LINEAR),
                        new KeyValue(bothRect.scalePivotProperty(), new Point3D(25, 25, 0), Interpolator.LINEAR)),
                new KeyFrame(Duration.seconds(3.3),
                        new KeyValue(scaleRect.scalePivotProperty(), new Point3D(75, 40, 0), Interpolator.LINEAR),
                        new KeyValue(bothRect.scalePivotProperty(), new Point3D(75, 40, 0), Interpolator.LINEAR)));
        scaleTimeline.setAutoReverse(true);
        scaleTimeline.setCycleCount(Animation.INDEFINITE);

        var scale = new ScaleTransition(Duration.seconds(2), scaleRect);
        scale.setToY(0);
        scale.setToX(0);
        scale.setCycleCount(Animation.INDEFINITE);
        scale.setAutoReverse(true);
        scale.setInterpolator(Interpolator.LINEAR);

        var scale2 = new ScaleTransition(Duration.seconds(2), bothRect);
        scale2.setToY(0);
        scale2.setToX(0);
        scale2.setCycleCount(Animation.INDEFINITE);
        scale2.setAutoReverse(true);
        scale2.setInterpolator(Interpolator.LINEAR);

        var rotTimeline = new Timeline(
                new KeyFrame(Duration.seconds(0),
                        new KeyValue(rotRect.rotationPivotProperty(), new Point3D(25, 25, 0), Interpolator.LINEAR),
                        new KeyValue(bothRect.rotationPivotProperty(), new Point3D(25, 25, 0), Interpolator.LINEAR)),
                new KeyFrame(Duration.seconds(3.3),
                        new KeyValue(rotRect.rotationPivotProperty(), new Point3D(75, 40, 0), Interpolator.LINEAR),
                        new KeyValue(bothRect.rotationPivotProperty(), new Point3D(75, 40, 0), Interpolator.LINEAR)));
        rotTimeline.setAutoReverse(true);
        rotTimeline.setCycleCount(Animation.INDEFINITE);

        var rot = new RotateTransition(Duration.seconds(4), rotRect);
        rot.setToAngle(360);
        rot.setCycleCount(Animation.INDEFINITE);
        rot.setInterpolator(Interpolator.LINEAR);

        var rot2 = new RotateTransition(Duration.seconds(4), bothRect);
        rot2.setToAngle(360);
        rot2.setCycleCount(Animation.INDEFINITE);
        rot2.setInterpolator(Interpolator.LINEAR);

        stage.setTitle("rotate then scale");
        stage.setScene(new Scene(root));
        stage.show();

        scale.play();
        scaleTimeline.play();
        rot.play();
        rotTimeline.play();
        rot2.play();
        scale2.play();
    }

    public static void main(String[] args) {
        launch(args);
    }
}

[nlisker]

@kevinrushforth I think that this fix can be separated from the rest of the animation issues, so I would like it reviewed earlier. The draft shows an implementation using the Point3D approach, but after doing it I'm starting to lean more towards doubles. See the JBS issue for the relevant points.

[kevinrushforth]

This will need discussion on the openjfx-dev mailing list. Here are the questions that need to be resolved:

1. The state of whether to use the computed center pivot or the value of the pivot attribute is implicit with no way for an application to know which it is, and no way to set it back to using the computed center (i.e., the state is sticky once you set it). Perhaps if you defined a null value as meaning "computed center" then an app could at least reset it to the "computed center" state, although there would still be no way for the app to know that it was in that state.

2. Do we need separate properties for scale pivot and center pivot? A single pivot property would be easier to define, but wouldn't allow you to set it from a RotationTransition and a ScaleTransition if you wanted to apply both to the same Node. With two separate properties, as you have defined it, it is more flexible, but you need to worry about what coordinate space the rotation pivot is defined in. The current transform chain looks like this:

T(layout+translate) * T(pivot) * T(rot) * T(scale) * T(-pivot)
    * transform[0] * transform [1] ...

Perhaps if the rotation pivot were defined in unscaled space, it might work. The transform chain would then look like this:

T(layout+translate) * T(pivotRot/scale) * T(rot) * T(-pivotRot/scale) * T(pivotScale) * T(scale) * T(-pivotScale)
    * transform[0] * transform [1] ...

In any case, you need to think about the implications of having one of scale or rotate being set explicitly and the other being the computed center.

3. Should the pivot be specified as a Point3D or 3 separate doubles? I can see pros / cons of either approach. Separate doubles are more consistent with the the pivot values in the Rotate and Scale Transform objects, and are easier to using in binding. On the other hand, there would be no out-of-band null value to use (see issue 1 above), so you would need a boolean property for each of scale pivot and rotation pivot to indicate whether the computed value or the value of the pivot properties should be used. I don't think that the fact that the rotation axis is defined as a Point3D should have any bearing on whether the pivot should be so defined. I'd probably lean towards separate doubles.

[mstr2]

Here's another proposal:

1. Expose pivotX (default: 0.5), pivotY (default: 0.5) and pivotZ (default: 0.5) as three separate doubles.

2. Add relativePivot (default: true), which controls whether the pivot coordinates are absolute coordinates, or relative to the layout bounds. Having the option to toggle between those two modes saves a lot of work for app developers, and it also removes the need for a "computed special value".

3. Having separate pivots for rotate and scale transforms is not supported.

[nlisker]

There was a continuation of the discussion here in the mailing list https://mail.openjdk.java.net/pipermail/openjfx-dev/2019-December/024314.html.

[openjdk[bot]]

@nlisker this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8234712_Add_pivot_properties_for_scale_and_rotation_in_Node,_ScaleTransition_and_RotateTransition
git fetch https://git.openjdk.java.net/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[nlisker]

Providing a test app

import javafx.animation.Animation;
import javafx.animation.Interpolator;
import javafx.animation.RotateTransition;
import javafx.animation.ScaleTransition;
import javafx.application.Application;
import javafx.beans.binding.Bindings;
import javafx.beans.binding.DoubleBinding;
import javafx.beans.binding.NumberBinding;
import javafx.beans.binding.When;
import javafx.beans.property.DoubleProperty;
import javafx.scene.Scene;
import javafx.scene.control.CheckBox;
import javafx.scene.control.Label;
import javafx.scene.control.Separator;
import javafx.scene.control.Slider;
import javafx.scene.control.TextField;
import javafx.scene.layout.BorderPane;
import javafx.scene.layout.HBox;
import javafx.scene.layout.Pane;
import javafx.scene.layout.VBox;
import javafx.scene.paint.Color;
import javafx.scene.shape.Circle;
import javafx.scene.shape.Rectangle;
import javafx.stage.Stage;
import javafx.util.Duration;
import javafx.util.converter.NumberStringConverter;

public class PivotApp extends Application {

    private final Rectangle rect = new Rectangle(100, 50);
    private final RotateTransition rotate = new RotateTransition(Duration.seconds(2), rect);
    private final ScaleTransition scale = new ScaleTransition(Duration.seconds(2), rect);

    @Override
    public void start(Stage stage) {
        rect.setFill(Color.SADDLEBROWN);
        rect.setTranslateX(150);
        rect.setTranslateY(150);

        scale.setToX(2);
        scale.setToY(2);
        scale.setCycleCount(Animation.INDEFINITE);
        scale.setAutoReverse(true);
        scale.setInterpolator(Interpolator.LINEAR);

        rotate.setToAngle(360);
        rotate.setCycleCount(Animation.INDEFINITE);
        rotate.setInterpolator(Interpolator.LINEAR);

        var rotPivot = createRotPivotIndicator();
        var scalePivot = createScalePivotIndicator();

        var controls = new VBox(createRotationControls(), new Separator(), createScaleControls());
        var shapes = new Pane(rect, scalePivot, rotPivot);
        var root = new BorderPane(shapes);
        root.setLeft(controls);
        stage.setScene(new Scene(root));
        stage.setWidth(700);
        stage.setHeight(500);
        stage.show();
    }

    private Circle createRotPivotIndicator() {
        var rotPivot = new Circle(3);
        rotPivot.setFill(Color.BLUE);
    
        DoubleProperty pivotX = rect.rotationPivotXProperty();
        DoubleBinding normalizedPivotX =  Bindings.createDoubleBinding(() -> clamp(0, pivotX.get(), 1) * rect.getBoundsInLocal().getWidth(),
                pivotX, rect.boundsInLocalProperty());
        NumberBinding relativePivotX = new When(rect.normalizedRotationPivotProperty()).then(normalizedPivotX).otherwise(pivotX);
        DoubleBinding absoultePivotX = rect.translateXProperty().add(relativePivotX);
        rotPivot.translateXProperty().bind(absoultePivotX);
    
        DoubleProperty pivotY = rect.rotationPivotYProperty();
        DoubleBinding normalizedPivotY =  Bindings.createDoubleBinding(() -> clamp(0, pivotY.get(), 1) * rect.getBoundsInLocal().getHeight(),
                pivotY, rect.boundsInLocalProperty());
        NumberBinding relativePivotY = new When(rect.normalizedRotationPivotProperty()).then(normalizedPivotY).otherwise(pivotY);
        DoubleBinding absoultePivotY = rect.translateYProperty().add(relativePivotY);
        rotPivot.translateYProperty().bind(absoultePivotY);

        return rotPivot;
    }

    private Circle createScalePivotIndicator() {
        var scalePivot = new Circle(3);
        scalePivot.setFill(Color.RED);

        DoubleProperty pivotX = rect.scalePivotXProperty();
        DoubleBinding normalizedPivotX =  Bindings.createDoubleBinding(() -> clamp(0, pivotX.get(), 1) * rect.getBoundsInLocal().getWidth(),
                pivotX, rect.boundsInLocalProperty());
        NumberBinding relativePivotX = new When(rect.normalizedScalePivotProperty()).then(normalizedPivotX).otherwise(pivotX);
        DoubleBinding absoultePivotX = rect.translateXProperty().add(relativePivotX);
        scalePivot.translateXProperty().bind(absoultePivotX);

        DoubleProperty pivotY = rect.scalePivotYProperty();
        DoubleBinding normalizedPivotY =  Bindings.createDoubleBinding(() -> clamp(0, pivotY.get(), 1) * rect.getBoundsInLocal().getHeight(),
                pivotY, rect.boundsInLocalProperty());
        NumberBinding relativePivotY = new When(rect.normalizedScalePivotProperty()).then(normalizedPivotY).otherwise(pivotY);
        DoubleBinding absoultePivotY = rect.translateYProperty().add(relativePivotY);
        scalePivot.translateYProperty().bind(absoultePivotY);

        return scalePivot;
    }

    private static double clamp(double min, double value, double max) {
        if (value < min) return min;
        if (value > max) return max;
        return value;
    }

    private Pane createRotationControls() {
        var pivotX = createSliderControl("x pivot", rotate.pivotXProperty(), -10, 10, 0.5);
        var pivotY = createSliderControl("y pivot", rotate.pivotYProperty(), -10, 10, 0.5);
        var pivotZ = createSliderControl("z pivot", rotate.pivotZProperty(), -10, 10, 0.5);

        var normalized = new CheckBox("Normalized");
        normalized.selectedProperty().bindBidirectional(rect.normalizedRotationPivotProperty());

        var on = new CheckBox("On/Off");
        on.selectedProperty().addListener((obs, ov, nv) -> {
            if (nv == true) {
                rotate.play();
            } else {
                rotate.stop();
                rect.setRotate(0);
            }
        });
        return new VBox(5, new Label("Rotation"), on, pivotX, pivotY, pivotZ, normalized);
    }

    private Pane createScaleControls() {
        var pivotX = createSliderControl("x pivot", scale.pivotXProperty(), -10, 10, 0.5);
        var pivotY = createSliderControl("y pivot", scale.pivotYProperty(), -10, 10, 0.5);
        var pivotZ = createSliderControl("z pivot", scale.pivotZProperty(), -10, 10, 0.5);

        var normalized = new CheckBox("Normalized");
        normalized.selectedProperty().bindBidirectional(rect.normalizedScalePivotProperty());

        var on = new CheckBox("On/Off");
        on.selectedProperty().addListener((obs, ov, nv) -> {
            if (nv == true) {
                scale.play();
            } else {
                scale.stop();
                rect.setScaleX(1);
                rect.setScaleY(1);
            }
        });
        return new VBox(5, new Label("Scale "), on, pivotX, pivotY, pivotZ, normalized);
    }

    private HBox createSliderControl(String name, DoubleProperty property, double min, double max, double start) {
        var slider = createSlider(min, max, start);
        property.bind(slider.valueProperty());
        return createSliderControl(name, slider);
    }

    private Slider createSlider(double min, double max, double start) {
        var slider = new Slider(min, max, start);
        slider.setShowTickMarks(true);
        slider.setShowTickLabels(true);
        return slider;
    }

    private HBox createSliderControl(String name, Slider slider) {
        var tf = createTextField(slider);
        return new HBox(5, new Label(name), slider, tf);
    }

    private TextField createTextField(Slider slider) {
        var tf = new TextField();
        tf.textProperty().bindBidirectional(slider.valueProperty(), new NumberStringConverter());
        tf.setMaxWidth(50);
        return tf;
    }
}

class PivotAppLauncher {

    public static void main(String[] args) {
        Application.launch(PivotApp.class, args);
    }
}

[hjohn]

I'd be in favor of doing this with a Point3D -- the amount of garbage created should be taken in the light of the framerate of the application. At 60 fps, that might be 60 Point3Ds per second or about 1.5 kB/sec, which I think can be fairly safely disregarded.

However, what if this could instead be completely unnecessary? What I'm wondering is, is this really a failing of Node that such properties are not provided or a failing of the transition system not to provide ways of controlling the pivot(s)?

I think it can be argued that having the basic 3 properties on Node (translation, scaling and rotation) should allow you to obtain any "look" for the Node that you want. I can rotate it by 90 degrees, while translating it to make it look like it rotated around its top left corner. This requires some math to achieve, but nothing too spectacular I think, especially if this was integrated into standard transitions.

If transitions provided ways of controlling the pivots, but without support in Node, then combining transitions (with parallel transition) wouldn't work properly any more. The transitions currently assume they're the only users of the properties involved. A rotate transition that simulates an off-center pivot by using the translation properties could not be combined with a translate transition or a scale transition that also uses a pivot.

Still, this seems more a transition problem than a Node problem. Currently you can't combine two Translate transitions either, even though it might be much easier to express two radically different translates by composing them instead of rolling them into one more complex calculation (especially if one transition triggered a bit later and the first one didn't finish yet). A smarter way of composing multiple transitions so they complement each other instead of interfering with each other could be an alternative?

(Thinking out of the box: if ParallelTransition would be responsible for the final changes to the translation/scale and rotation properties, then it could fire each transition in turn, accumulate the changes made, reset the changes, and after the final transition set the combined result on Node -- there would a bit more to it than that, order of transitions would matter, but that doesn't seem backward incompatible as transitions didn't share any properties yet before -- there are other issues, like unwanted change events but I think that could be solved as well if transitions didn't access Node directly, instead using a separate structure that they manipulate that can be copied to Node's properties later).

[nlisker]

I will look into the option of integrating this behavior into transitions only. While they don't have access to the way Node does it via its local transforms matrix, maybe there is some creative solution in this direction.

On Sun, Nov 6, 2022 at 5:10 PM John Hendrikx @.***> wrote:

I'd be in favor of doing this with a Point3D -- the amount of garbage created should be taken in the light of the framerate of the application. At 60 fps, that might be 60 Point3Ds per second or about 1.5 kB/sec, which I think can be fairly safely disregarded.

However, what if this could instead be completely unnecessary? What I'm wondering is, is this really a failing of Node that such properties are not provided or a failing of the transition system not to provide ways of controlling the pivot(s)?

I think it can be argued that having the basic 3 properties on Node (translation, scaling and rotation) should allow you to obtain any "look" for the Node that you want. I can rotate it by 90 degrees, while translating it to make it look like it rotated around its top left corner. This requires some math to achieve, but nothing too spectacular I think, especially if this was integrated into standard transitions.

If transitions provided ways of controlling the pivots, but without support in Node, then combining transitions (with parallel transition) wouldn't work properly any more. The transitions currently assume they're the only users of the properties involved. A rotate transition that simulates an off-center pivot by using the translation properties could not be combined with a translate transition or a scale transition that also uses a pivot.

Still, this seems more a transition problem than a Node problem. Currently you can't combine two Translate transitions either, even though it might be much easier to express two radically different translates by composing them instead of rolling them into one more complex calculation (especially if one transition triggered a bit later and the first one didn't finish yet). A smarter way of composing multiple transitions so they complement each other instead of interfering with each other could be an alternative?

(Thinking out of the box: if ParallelTransition would be responsible for the final changes to the translation/scale and rotation properties, then it could fire each transition in turn, accumulate the changes made, reset the changes, and after the final transition set the combined result on Node -- there would a bit more to it than that, order of transitions would matter, but that doesn't seem backward incompatible as transitions didn't share any properties yet before -- there are other issues, like unwanted change events but I think that could be solved as well if transitions didn't access Node directly, instead using a separate structure that they manipulate that can be copied to Node's properties later).

— Reply to this email directly, view it on GitHub https://github.com/openjdk/jfx/pull/53#issuecomment-1304822989, or unsubscribe https://github.com/notifications/unsubscribe-auth/AI5QOMZCZKPJF55Q4ULULTLWG7C5XANCNFSM4JR3TYYQ . You are receiving this because you were mentioned.Message ID: @.***>

[bridgekeeper[bot]]

@nlisker This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[nlisker]

Still alive, bridgekeeperbot

[bridgekeeper[bot]]

@nlisker This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[nlisker]

Still alive, bridgekeeperbot

[bridgekeeper[bot]]

@nlisker This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper[bot]]

@nlisker This pull request has been inactive for more than 16 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.