Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e359d86
Fix precision problems in gesture recognizers
hjohn Dec 3, 2022
eef709a
General clean ups
hjohn Dec 3, 2022
0477a1d
Revert all changes to RotateGestureRecognizer
hjohn Dec 15, 2022
ce9979f
Make RotateGestureRecognizer easier to test
hjohn Dec 15, 2022
db6b33c
Merge remote-tracking branch 'upstream/master' into feature/precision…
hjohn Dec 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GestureRecognizer.java
View file
Open in desktop
Expand Up @@ -25,5 +25,9 @@

package com.sun.javafx.tk.quantum;

import java.util.concurrent.TimeUnit;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100_000L ?

static final double NANOS_TO_SECONDS = 1.0 / TimeUnit.SECONDS.toNanos(1);
}
124 changes: 58 additions & 66 deletions modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/RotateGestureRecognizer.java
View file
Open in desktop
Expand Up @@ -25,76 +25,72 @@

package com.sun.javafx.tk.quantum;

import com.sun.glass.events.KeyEvent;
import com.sun.glass.events.TouchEvent;

import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import javafx.util.Duration;
import javafx.event.EventType;
import javafx.scene.input.RotateEvent;
import javafx.animation.Animation;
import java.util.concurrent.TimeUnit;

import com.sun.glass.events.KeyEvent;
import com.sun.glass.events.TouchEvent;

import javafx.animation.Interpolator;
import javafx.animation.KeyFrame;
import javafx.animation.KeyValue;
import javafx.animation.Timeline;
import javafx.beans.property.DoubleProperty;
import javafx.beans.property.SimpleDoubleProperty;
import javafx.scene.input.RotateEvent;
import javafx.util.Duration;

class RotateGestureRecognizer implements GestureRecognizer {
private ViewScene scene;
private static final double MAX_INITIAL_VELOCITY = 500;
private static final double ROTATION_INERTIA_MILLIS = 1500;
private static final long ROTATION_INERTIA_THRESHOLD_NANOS = TimeUnit.MILLISECONDS.toNanos(300);

// gesture will be activated if |rotation| > rotationThresholdDegrees
private static double rotationThresholdDegrees = 5;
private static boolean rotationInertiaEnabled = true;

// gesture will be activated if |rotation| > ROTATATION_THRESHOLD
private static double ROTATATION_THRESHOLD = 5; //in degrees
private static boolean ROTATION_INERTIA_ENABLED = true;
private static double MAX_INITIAL_VELOCITY = 500;
private static double ROTATION_INERTIA_MILLIS = 1500;
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ditto on line 64

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that sort of change seems out of scope.

}
s = System.getProperty("com.sun.javafx.gestures.rotate.inertia");
if (s != null) {
ROTATION_INERTIA_ENABLED = Boolean.valueOf(s);
rotationInertiaEnabled = Boolean.valueOf(s);
}
return null;
});
}

private RotateRecognitionState state = RotateRecognitionState.IDLE;
private Timeline inertiaTimeline = new Timeline();
private DoubleProperty inertiaRotationVelocity = new SimpleDoubleProperty();
private double initialInertiaRotationVelocity = 0;
private double rotationStartTime = 0;
private double lastTouchEventTime = 0;
private final Timeline inertiaTimeline = new Timeline();
private final DoubleProperty inertiaRotationVelocity = new SimpleDoubleProperty();
private final Map<Long, TouchPointTracker> trackers = new HashMap<>(); // from MultiTouchTracker

// from MultiTouchTracker
Map<Long, TouchPointTracker> trackers =
new HashMap<Long, TouchPointTracker>();
private ViewScene scene;
private RotateRecognitionState state = RotateRecognitionState.IDLE;
private double initialInertiaRotationVelocity;
private long rotationStartNanos;

int modifiers;
boolean direct;
private int modifiers;
private boolean direct;

//private int touchCount;
private int currentTouchCount = 0;
private int currentTouchCount;
private boolean touchPointsSetChanged;
private boolean touchPointsPressed;
int touchPointsInEvent;
long touchPointID1 = -1;
long touchPointID2 = -1;
double centerX, centerY;
double centerAbsX, centerAbsY;
private long touchPointID1 = -1;
private long touchPointID2 = -1;
private double centerX, centerY;
private double centerAbsX, centerAbsY;

double currentRotation;
double angleReference;
double totalRotation = 0;
double inertiaLastTime = 0;
private double currentRotation;
private double angleReference;
private double totalRotation;
private double inertiaLastTime;

RotateGestureRecognizer(final ViewScene scene) {
this.scene = scene;
Expand All @@ -115,27 +111,25 @@ public void notifyBeginTouchEvent(long time, int modifiers, boolean isDirect,
params(modifiers, isDirect);
touchPointsSetChanged = false;
touchPointsPressed = false;
touchPointsInEvent = 0;
}

@Override
public void notifyNextTouchEvent(long time, int type, long touchId,
int x, int y, int xAbs, int yAbs) {
touchPointsInEvent++;
switch(type) {
case TouchEvent.TOUCH_PRESSED:
touchPointsSetChanged = true;
touchPointsPressed = true;
touchPressed(touchId, time, x, y, xAbs, yAbs);
touchPressed(touchId, x, y, xAbs, yAbs);
break;
case TouchEvent.TOUCH_STILL:
break;
case TouchEvent.TOUCH_MOVED:
touchMoved(touchId, time, x, y, xAbs, yAbs);
touchMoved(touchId, x, y, xAbs, yAbs);
break;
case TouchEvent.TOUCH_RELEASED:
touchPointsSetChanged = true;
touchReleased(touchId, time, x, y, xAbs, yAbs);
touchReleased(touchId);
break;
default:
throw new RuntimeException("Error in Rotate gesture recognition: "
Expand Down Expand Up @@ -219,8 +213,7 @@ private void assignActiveTouchpoints() {
}

@Override
public void notifyEndTouchEvent(long time) {
lastTouchEventTime = time;
public void notifyEndTouchEvent(long nanos) {
if (currentTouchCount != trackers.size()) {
throw new RuntimeException("Error in Rotate gesture recognition: "
+ "touch count is wrong: " + currentTouchCount);
Expand All @@ -230,9 +223,10 @@ public void notifyEndTouchEvent(long time) {
if (state == RotateRecognitionState.ACTIVE) {
sendRotateFinishedEvent();
}
if (ROTATION_INERTIA_ENABLED && (state == RotateRecognitionState.PRE_INERTIA || state == RotateRecognitionState.ACTIVE)) {
double timeFromLastRotation = ((double)time - rotationStartTime) / 1000000;
if (timeFromLastRotation < 300) {
if (rotationInertiaEnabled && (state == RotateRecognitionState.PRE_INERTIA || state == RotateRecognitionState.ACTIVE)) {
long nanosSinceLastRotation = nanos - rotationStartNanos;

if (nanosSinceLastRotation < ROTATION_INERTIA_THRESHOLD_NANOS) {
state = RotateRecognitionState.INERTIA;
// activate inertia
inertiaLastTime = 0;
Expand All @@ -247,10 +241,7 @@ else if (initialInertiaRotationVelocity < -MAX_INITIAL_VELOCITY)
new KeyValue(inertiaRotationVelocity, initialInertiaRotationVelocity, Interpolator.LINEAR)),
new KeyFrame(
Duration.millis(ROTATION_INERTIA_MILLIS * Math.abs(initialInertiaRotationVelocity) / MAX_INITIAL_VELOCITY),
event -> {
//stop inertia
reset();
},
event -> reset(), // stop inertia
new KeyValue(inertiaRotationVelocity, 0, Interpolator.LINEAR))
);
inertiaTimeline.playFromStart();
Expand All @@ -268,7 +259,7 @@ else if (initialInertiaRotationVelocity < -MAX_INITIAL_VELOCITY)
if (currentTouchCount == 1) {
if (state == RotateRecognitionState.ACTIVE) {
sendRotateFinishedEvent();
if (ROTATION_INERTIA_ENABLED) {
if (rotationInertiaEnabled) {
//prepare for inertia
state = RotateRecognitionState.PRE_INERTIA;
} else {
Expand Down Expand Up @@ -297,7 +288,7 @@ else if (initialInertiaRotationVelocity < -MAX_INITIAL_VELOCITY)
} else {
currentRotation = getNormalizedDelta(angleReference, newAngle);
if (state == RotateRecognitionState.TRACKING) {
if (Math.abs(currentRotation) > ROTATATION_THRESHOLD) {
if (Math.abs(currentRotation) > rotationThresholdDegrees) {
state = RotateRecognitionState.ACTIVE;
sendRotateStartedEvent();
}
Expand All @@ -307,10 +298,11 @@ else if (initialInertiaRotationVelocity < -MAX_INITIAL_VELOCITY)
totalRotation += currentRotation;
sendRotateEvent(false);
angleReference = newAngle;
double timePassed = ((double)time - rotationStartTime) / 1000000000;
if (timePassed > 1e-4) {
initialInertiaRotationVelocity = currentRotation / timePassed;
rotationStartTime = time;
long nanosPassed = nanos - rotationStartNanos;

if (nanosPassed > INITIAL_VELOCITY_THRESHOLD_NANOS) {
initialInertiaRotationVelocity = currentRotation / nanosPassed * NANOS_TO_SECONDS;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

rotationStartNanos = nanos;
}
}
}
Expand Down Expand Up @@ -374,19 +366,19 @@ private void sendRotateFinishedEvent() {
}, scene.getAccessControlContext());
}

public void params(int modifiers, boolean direct) {
private void params(int modifiers, boolean direct) {
this.modifiers = modifiers;
this.direct = direct;
}

public void touchPressed(long id, long nanos, int x, int y, int xAbs, int yAbs) {
private void touchPressed(long id, int x, int y, int xAbs, int yAbs) {
currentTouchCount++;
TouchPointTracker tracker = new TouchPointTracker();
tracker.update(nanos, x, y, xAbs, yAbs);
tracker.update(x, y, xAbs, yAbs);
trackers.put(id, tracker);
}

public void touchReleased(long id, long nanos, int x, int y, int xAbs, int yAbs) {
private void touchReleased(long id) {
if (state != RotateRecognitionState.FAILURE) {
TouchPointTracker tracker = trackers.get(id);
if (tracker == null) {
Expand All @@ -400,7 +392,7 @@ public void touchReleased(long id, long nanos, int x, int y, int xAbs, int yAbs)
currentTouchCount--;
}

public void touchMoved(long id, long nanos, int x, int y, int xAbs, int yAbs) {
private void touchMoved(long id, int x, int y, int xAbs, int yAbs) {
if (state == RotateRecognitionState.FAILURE) {
return;
}
Expand All @@ -412,10 +404,10 @@ public void touchMoved(long id, long nanos, int x, int y, int xAbs, int yAbs) {
throw new RuntimeException("Error in rotate gesture "
+ "recognition: reported unknown touch point");
}
tracker.update(nanos, x, y, xAbs, yAbs);
tracker.update(x, y, xAbs, yAbs);
}

void reset() {
private void reset() {
state = RotateRecognitionState.IDLE;
touchPointID1 = -1;
touchPointID2 = -1;
Expand All @@ -427,7 +419,7 @@ private static class TouchPointTracker {
double x, y;
double absX, absY;

public void update(long nanos, double x, double y, double absX, double absY) {
public void update(double x, double y, double absX, double absY) {
this.x = x;
this.y = y;
this.absX = absX;
Expand Down