JDK-8298060: Fix precision bug in gesture recognizer classes #966

hjohn · 2022-12-03T22:17:55Z

This includes a fix for the precision problem we've found as part of the graphics warnings clean ups.

I've included two commits, one with just the minimal fix, and one with the clean ups. I can drop off the 2nd commit if it is deemed to be of no added value.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8298060: Fix precision bug in gesture recognizer classes

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/966/head:pull/966
$ git checkout pull/966

Update a local copy of the PR:
$ git checkout pull/966
$ git pull https://git.openjdk.org/jfx.git pull/966/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 966

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/966.diff

Webrev

Link to Webrev Comment

- Made constants written in capitals final - Made fields final that can be final - Made fields and methods private that can be private - Renamed constants that aren't constant to camel case - Fixed spelling errors - Re-ordered fields (statics at the top) - Removed redundant initial values - Removed unused fields and parameters - Optimized imports - Removed commented out code

bridgekeeper · 2022-12-03T22:18:35Z

👋 Welcome back jhendrikx! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2022-12-05T12:47:37Z

Webrevs

arapte · 2022-12-12T05:41:45Z

The commit#1 changes look good.
I would recommend to revert commit#2 for few reasons:

Cleanup changes are a lot more than actual fix change.
It will be difficult to revisit in future.
If required the cleanup changes could go in as a separate fix.

Could you please provide link to the discussion that led to this change?

hjohn · 2022-12-12T07:28:59Z

The commit#1 changes look good. I would recommend to revert commit#2 for few reasons:

Cleanup changes are a lot more than actual fix change.

It will be difficult to revisit in future.

If required the cleanup changes could go in as a separate fix.

Could you please provide link to the discussion that led to this change?

@arapte the discussion was part of fixing warnings in javafx.graphics. You can find it here (open first resolved comment): #960 (review)

andy-goryachev-oracle · 2022-12-12T16:48:54Z

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GestureRecognizer.java

 interface GestureRecognizer extends GlassTouchEventListener {
+    static final long INITIAL_VELOCITY_THRESHOLD_NANOS = 100L * 1000;


andy-goryachev-oracle · 2022-12-12T16:51:01Z

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/RotateGestureRecognizer.java

    static {
        @SuppressWarnings("removal")
        var dummy = AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
            String s = System.getProperty("com.sun.javafx.gestures.rotate.threshold");
            if (s != null) {
-                ROTATATION_THRESHOLD = Double.valueOf(s);
+                rotationThresholdDegrees = Double.valueOf(s);


should we catch a NumberFormatException and re-throw it with a meaningful text message? Or the current behavior is ok?

ditto on line 64

I suppose that would be out of scope for this PR. I agree though that it would be good to mention that the double that couldn't be parsed is the one we got from System.getProperty("com.sun.javafx.gestures.rotate.threshold") as otherwise the error would mean nothing to anyone.

Yes, that sort of change seems out of scope.

andy-goryachev-oracle · 2022-12-12T16:55:01Z

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/ScrollGestureRecognizer.java

            }
            s = System.getProperty("com.sun.javafx.gestures.scroll.inertia");
            if (s != null) {
-                SCROLL_INERTIA_ENABLED = Boolean.valueOf(s);
+                scrollInertiaEnabled = Boolean.valueOf(s);


same comment about re-throwing an exception (x2)

andy-goryachev-oracle · 2022-12-12T16:57:12Z

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/ScrollGestureRecognizer.java

                        //capture radius (pytaguras) or init to variables x,y ???
                        double scrollMagnitude = Math.sqrt(deltaX * deltaX + deltaY * deltaY);
                        factorX = deltaX / scrollMagnitude;
                        factorY = deltaY / scrollMagnitude;
-                        initialInertiaScrollVelocity = scrollMagnitude / timePassed;
+                        initialInertiaScrollVelocity = scrollMagnitude / nanosPassed * NANOS_TO_SECONDS;


is this correct? shouldn't it be scrollMagnitude / (nanosPassed * NANOS_TO_SECONDS)?

I think it's correct, unless you have reason to believe why it wouldn't be. scrollMagnitude is a double, and the division and multiplication that follow it will be all done as doubles.

your change is equivalent to

(scrollMagnitude * NANOS_TO_SECONDS) / nanosPassed

is that correct?

That would be equivalent yes.

Perhaps it is because timePassed was a double before measured in seconds while I'm now doing the calculation with nano seconds?

Okay I see what you're getting at, your first suggestion would not be correct, it's supposed to be what you suggest in the 2nd one. So what I want is:

(scrollMagnitude / nanosPassed) * NANOS_TO_SECONDS

or:

(scrollMagnitude * NANOS_TO_SECONDS) / nanosPassed

...but that's the same as what I wrote.

I just needed you to confirm that this is indeed correct.

Perhaps a default method double nanosToSeconds(long) in GestureRecognizer would be clearer?

But is it correct? The previous code computed a deltaTime in seconds as ((currentNanos - startNanos) / 1e9), and then divided the scrollMagnitude by that. I think that the equivalent to the old code is:

scrollMagnitude / (nanosPassed * NANOS_TO_SECONDS)

It turns out it is not correct, the parenthesis are indeed required, not sure how I could have missed it. I'm going to see if I can add a test to this as they're missing to ensure it does the same before/after (aside from precision).

could we create a new method, like 'nanosToSeconds'?

andy-goryachev-oracle · 2022-12-12T16:57:59Z

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/RotateGestureRecognizer.java

+                        long nanosPassed = nanos - rotationStartNanos;
+
+                        if (nanosPassed > INITIAL_VELOCITY_THRESHOLD_NANOS) {
+                            initialInertiaRotationVelocity = currentRotation / nanosPassed * NANOS_TO_SECONDS;


is this correct? shouldn't it be currentRotation / (nanosPassed * NANOS_TO_SECONDS) ?

andy-goryachev-oracle · 2022-12-12T16:58:55Z

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/ZoomGestureRecognizer.java

            }
            s = System.getProperty("com.sun.javafx.gestures.zoom.inertia");
            if (s != null) {
-                ZOOM_INERTIA_ENABLED = Boolean.valueOf(s);
+                zoomInertiaEnabled = Boolean.valueOf(s);


... and here

andy-goryachev-oracle · 2022-12-12T16:59:41Z

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/ZoomGestureRecognizer.java

+                        long nanosPassed = nanos - zoomStartNanos;
+
+                        if (nanosPassed > INITIAL_VELOCITY_THRESHOLD_NANOS) {
+                            initialInertiaZoomVelocity = (totalZoomFactor - prevTotalZoomFactor) / nanosPassed * NANOS_TO_SECONDS;


(totalZoomFactor - prevTotalZoomFactor) / (nanosPassed * NANOS_TO_SECONDS); ?

The order is irrelevant, it could as well be ((totalZoomFactor - prevTotalZoomFactor) / nanosPassed) * NANOS_TO_SECONDS -- I'll let the compiler decide what is more optimal.

See other conversation, your suggestion would not be correct as it changes the order of the operations.

kevinrushforth

inline

kevinrushforth · 2022-12-12T22:21:48Z

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/RotateGestureRecognizer.java

    static {
        @SuppressWarnings("removal")
        var dummy = AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
            String s = System.getProperty("com.sun.javafx.gestures.rotate.threshold");
            if (s != null) {
-                ROTATATION_THRESHOLD = Double.valueOf(s);
+                rotationThresholdDegrees = Double.valueOf(s);


Yes, that sort of change seems out of scope.

kevinrushforth · 2022-12-12T23:02:06Z

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/ScrollGestureRecognizer.java

                        //capture radius (pytaguras) or init to variables x,y ???
                        double scrollMagnitude = Math.sqrt(deltaX * deltaX + deltaY * deltaY);
                        factorX = deltaX / scrollMagnitude;
                        factorY = deltaY / scrollMagnitude;
-                        initialInertiaScrollVelocity = scrollMagnitude / timePassed;
+                        initialInertiaScrollVelocity = scrollMagnitude / nanosPassed * NANOS_TO_SECONDS;


But is it correct? The previous code computed a deltaTime in seconds as ((currentNanos - startNanos) / 1e9), and then divided the scrollMagnitude by that. I think that the equivalent to the old code is:

scrollMagnitude / (nanosPassed * NANOS_TO_SECONDS)

openjdk · 2022-12-15T12:15:47Z

@hjohn 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 feature/precision-problem-in-gesture-recognizers
git fetch https://git.openjdk.org/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

…-problem-in-gesture-recognizers

hjohn · 2022-12-15T12:23:54Z

@kevinrushforth

Please advise if I should continue on this road:

I've reverted RotateGestureRecognizer to add a test case first.

To make RotateGestureRecognizer testable I had to make it public and abstract away the need for ViewScene as constructor parameter, see the Make RotateGestureRecognizer easier to test commit. I also added a test case (90% coverage).

If you are okay with these changes (I needed to change GlassViewEventHandler and add a small interface), I can do the same for the Zoom & ScrollGestureRecognizers.

The test I added reveals the precision problem when you uncomment the line: passTime(Long.MAX_VALUE / 1000 / 1000 - 10000000); -- the accuracy problem of converting longs to doubles then breaks the test. The newer version of the code works (after I fixed the calculation by adding parenthesis).

kevinrushforth · 2022-12-16T23:57:51Z

To make RotateGestureRecognizer testable I had to make it public and abstract away the need for ViewScene as constructor parameter, see the Make RotateGestureRecognizer easier to test commit. I also added a test case (90% coverage).

This seems quite an intrusive change just to support low level testing of this. I can take a closer look next week, but I'm not really sold on the idea.

bridgekeeper · 2023-03-31T23:05:20Z

@hjohn This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 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!

hjohn · 2023-04-01T09:05:25Z

I think this change still has merit. I also think the change to make the code more testable is a good thing. A lot of code in JavaFX is hard to test, and when things are hard to test, the testing is often done poorly or not at all -- these classes did not have any tests.

bridgekeeper · 2023-04-29T09:18:49Z

@hjohn This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 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 · 2023-05-27T15:17:54Z

@hjohn This pull request has been inactive for more than 8 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.

hjohn added 2 commits December 3, 2022 22:48

Fix precision problems in gesture recognizers

e359d86

hjohn marked this pull request as ready for review December 5, 2022 12:42

openjdk bot added the rfr Ready for review label Dec 5, 2022

kevinrushforth requested a review from arapte December 5, 2022 21:27

andy-goryachev-oracle suggested changes Dec 12, 2022

View reviewed changes

kevinrushforth reviewed Dec 12, 2022

View reviewed changes

hjohn added 2 commits December 15, 2022 11:51

Revert all changes to RotateGestureRecognizer

0477a1d

Make RotateGestureRecognizer easier to test

ce9979f

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Dec 15, 2022

Merge remote-tracking branch 'upstream/master' into feature/precision…

db6b33c

…-problem-in-gesture-recognizers

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Dec 15, 2022

kevinrushforth self-requested a review December 19, 2022 17:34

bridgekeeper bot closed this May 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8298060: Fix precision bug in gesture recognizer classes #966

JDK-8298060: Fix precision bug in gesture recognizer classes #966

hjohn commented Dec 3, 2022 •

edited by openjdk bot

bridgekeeper bot commented Dec 3, 2022

mlbridge bot commented Dec 5, 2022 •

edited

arapte commented Dec 12, 2022

hjohn commented Dec 12, 2022

andy-goryachev-oracle Dec 12, 2022

andy-goryachev-oracle Dec 12, 2022

hjohn Dec 12, 2022

kevinrushforth Dec 12, 2022

andy-goryachev-oracle Dec 12, 2022

andy-goryachev-oracle Dec 12, 2022

hjohn Dec 12, 2022

andy-goryachev-oracle Dec 12, 2022

hjohn Dec 12, 2022

hjohn Dec 12, 2022

andy-goryachev-oracle Dec 12, 2022 •

edited

kevinrushforth Dec 12, 2022

hjohn Dec 13, 2022

andy-goryachev-oracle Dec 13, 2022

andy-goryachev-oracle Dec 12, 2022

andy-goryachev-oracle Dec 12, 2022

andy-goryachev-oracle Dec 12, 2022

hjohn Dec 12, 2022

hjohn Dec 12, 2022

kevinrushforth left a comment

kevinrushforth Dec 12, 2022

kevinrushforth Dec 12, 2022

openjdk bot commented Dec 15, 2022

hjohn commented Dec 15, 2022

kevinrushforth commented Dec 16, 2022

bridgekeeper bot commented Mar 31, 2023

hjohn commented Apr 1, 2023

bridgekeeper bot commented Apr 29, 2023

bridgekeeper bot commented May 27, 2023

		interface GestureRecognizer extends GlassTouchEventListener {
		static final long INITIAL_VELOCITY_THRESHOLD_NANOS = 100L * 1000;

JDK-8298060: Fix precision bug in gesture recognizer classes #966

JDK-8298060: Fix precision bug in gesture recognizer classes #966

Conversation

hjohn commented Dec 3, 2022 • edited by openjdk bot

Progress

Issue

Reviewing

Webrev

bridgekeeper bot commented Dec 3, 2022

mlbridge bot commented Dec 5, 2022 • edited

Webrevs

arapte commented Dec 12, 2022

hjohn commented Dec 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-goryachev-oracle Dec 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Dec 15, 2022

hjohn commented Dec 15, 2022

kevinrushforth commented Dec 16, 2022

bridgekeeper bot commented Mar 31, 2023

hjohn commented Apr 1, 2023

bridgekeeper bot commented Apr 29, 2023

bridgekeeper bot commented May 27, 2023

hjohn commented Dec 3, 2022 •

edited by openjdk bot

mlbridge bot commented Dec 5, 2022 •

edited

andy-goryachev-oracle Dec 12, 2022 •

edited