You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sis.apache.org by de...@apache.org on 2023/02/13 11:04:21 UTC
[sis] branch geoapi-4.0 updated: Fix a bug in the positionning of the image when some gestures are applied while an image is computed in background thread. This bug became more visible after the support of Cloud Optimized GeoTIFF through HTTP because of network transfer delays.
This is an automated email from the ASF dual-hosted git repository.
desruisseaux pushed a commit to branch geoapi-4.0
in repository https://gitbox.apache.org/repos/asf/sis.git
The following commit(s) were added to refs/heads/geoapi-4.0 by this push:
new c281eeb145 Fix a bug in the positionning of the image when some gestures are applied while an image is computed in background thread. This bug became more visible after the support of Cloud Optimized GeoTIFF through HTTP because of network transfer delays.
c281eeb145 is described below
commit c281eeb145097cf4ec44a7eafb5beb6a03844875
Author: Martin Desruisseaux <ma...@geomatys.com>
AuthorDate: Mon Feb 13 12:02:59 2023 +0100
Fix a bug in the positionning of the image when some gestures are applied while an image is computed in background thread.
This bug became more visible after the support of Cloud Optimized GeoTIFF through HTTP because of network transfer delays.
---
.../apache/sis/gui/coverage/CoverageCanvas.java | 2 +-
.../java/org/apache/sis/gui/map/MapCanvas.java | 55 ++++++++++++++++++----
.../java/org/apache/sis/gui/map/MapCanvasAWT.java | 9 ++--
.../java/org/apache/sis/gui/map/RenderingTask.java | 42 +++++++++++++++++
.../org/apache/sis/coverage/grid/GridExtent.java | 2 +-
.../sis/internal/map/coverage/RenderingData.java | 31 ++++++++++--
6 files changed, 119 insertions(+), 22 deletions(-)
diff --git a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/CoverageCanvas.java b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/CoverageCanvas.java
index 7d4a3757e5..07ecc32463 100644
--- a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/CoverageCanvas.java
+++ b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/CoverageCanvas.java
@@ -728,7 +728,7 @@ public class CoverageCanvas extends MapCanvasAWT {
* point all remaining points are executed:
*
* <ol>
- * <li>Read a new coverage if zoom as changed more than some threshold value.</li>
+ * <li>Read a new coverage if zoom has changed more than some threshold value.</li>
* <li>Compute statistics on sample values (if needed).</li>
* <li>Stretch the color ramp (if requested).</li>
* <li>Resample the image and convert to integer values.</li>
diff --git a/application/sis-javafx/src/main/java/org/apache/sis/gui/map/MapCanvas.java b/application/sis-javafx/src/main/java/org/apache/sis/gui/map/MapCanvas.java
index 5f5b946dd1..78c37612f6 100644
--- a/application/sis-javafx/src/main/java/org/apache/sis/gui/map/MapCanvas.java
+++ b/application/sis-javafx/src/main/java/org/apache/sis/gui/map/MapCanvas.java
@@ -63,6 +63,7 @@ import org.apache.sis.referencing.operation.matrix.MatrixSIS;
import org.apache.sis.referencing.operation.transform.MathTransforms;
import org.apache.sis.referencing.operation.transform.LinearTransform;
import org.apache.sis.referencing.cs.CoordinateSystems;
+import org.apache.sis.referencing.IdentifiedObjects;
import org.apache.sis.geometry.DirectPosition2D;
import org.apache.sis.geometry.Envelope2D;
import org.apache.sis.geometry.AbstractEnvelope;
@@ -87,7 +88,6 @@ import org.apache.sis.internal.referencing.j2d.AffineTransform2D;
import org.apache.sis.portrayal.PlanarCanvas;
import org.apache.sis.portrayal.RenderException;
import org.apache.sis.portrayal.TransformChangeEvent;
-import org.apache.sis.referencing.IdentifiedObjects;
import static org.apache.sis.internal.gui.LogHandler.LOGGER;
import static org.apache.sis.internal.util.StandardDateFormat.NANOS_PER_MILLISECOND;
@@ -1051,7 +1051,7 @@ public abstract class MapCanvas extends PlanarCanvas {
* </ol>
*
* @author Martin Desruisseaux (Geomatys)
- * @version 1.1
+ * @version 1.4
* @since 1.1
*/
protected abstract static class Renderer {
@@ -1272,7 +1272,7 @@ public abstract class MapCanvas extends PlanarCanvas {
renderedContentStamp = contentChangeCount;
final Renderer context = createRenderer();
if (context != null && context.initialize(floatingPane)) {
- final Task<?> worker = createWorker(context);
+ final RenderingTask<?> worker = createWorker(context);
assert renderingInProgress == null : renderingInProgress;
BackgroundThreads.execute(worker);
renderingInProgress = worker; // Set after we know that the task has been scheduled.
@@ -1291,15 +1291,15 @@ public abstract class MapCanvas extends PlanarCanvas {
/**
* Creates the background task which will invoke {@link Renderer#render()} in a background thread.
- * The tasks must invoke {@link #renderingCompleted(Task)} in JavaFX thread after completion,
+ * The tasks must invoke {@link #renderingCompleted(RenderingTask)} in JavaFX thread after completion,
* either successful or not.
*
* <p><b>Note:</b> it is important that no other worker is in progress at the time this method is invoked
* ({@code assert renderingInProgress == null}), otherwise conflicts may happen when workers will update
* the {@code MapCanvas} fields after they completed their task.</p>
*/
- Task<?> createWorker(final Renderer renderer) {
- return new Task<Void>() {
+ RenderingTask<?> createWorker(final Renderer renderer) {
+ return new RenderingTask<Void>() {
/** Invoked in background thread. */
@Override protected Void call() throws Exception {
renderer.render();
@@ -1327,8 +1327,28 @@ public abstract class MapCanvas extends PlanarCanvas {
* which is now integrated in the map. That transform will be removed from {@link #floatingPane} transforms.
* The {@link #transform} result is identity if no zoom, rotation or pan gesture has been applied since last
* rendering.
+ *
+ * <h4>Use case</h4>
+ * <p>Suppose that the {@link RenderingTask} has been started in response to some user gestures.
+ * For example, the user has zoomed on the map. The renderer has been initialized with a snapshot
+ * of this {@code MapCanvas} state at the time when the {@link Renderer} has been constructed.
+ * From this state, the renderer infers which data region to load at which resolution.</p>
+ *
+ * <p>Suppose that the rendering takes a long time, and during that time the user continues to do
+ * zoom and pan gestures. {@code MapCanvas} records those gestures in an {@link Affine} transform
+ * and will apply those changes on the image created by the <em>previous</em> rendering.
+ * For example if the user did a pan gesture of 100 pixels while the {@link Renderer} was working,
+ * then after the renderer finished to produce an image, that new image will also be translated by
+ * 100 pixels and a new call to {@link #repaint()} will happen later.</p>
+ *
+ * <p>Suppose that a zoom-in gesture caused the {@link Rendeder} to paint an image having a resolution
+ * twice finer than the resolution used in previous rendering. If the user does a translation of 100 pixels
+ * while this rendering is in progress, that "100 pixels" measurement is in units of the old rendering.
+ * It will need to be converted to 200 pixels after the rendering completed.</p>
+ *
+ * @param task the background task which has been completed, successfully or not.
*/
- final void renderingCompleted(final Task<?> task) {
+ final void renderingCompleted(final RenderingTask<?> task) {
assert Platform.isFxApplicationThread();
assert renderingInProgress == task : "Expected " + renderingInProgress + " but was " + task;
// Keep cursor unchanged if contents changed, because caller will invoke `repaint()` again.
@@ -1336,15 +1356,30 @@ public abstract class MapCanvas extends PlanarCanvas {
restoreCursorAfterPaint();
}
renderingInProgress = null;
+ /*
+ * Display coordinates stored in this `MapCanvas` need to be converted to the
+ * new display coordinates, as expected by the new "objective to display" CRS.
+ */
final Point2D p = changeInProgress.transform(xPanStart, yPanStart);
xPanStart = p.getX();
yPanStart = p.getY();
try {
changeInProgress.invert();
- transform.prepend(changeInProgress);
+ transform.append(changeInProgress);
+ /*
+ * Note: intuitively one may expect `prepend(…)` instead of `append(…)` above.
+ * The use of `prepend(…)` would give a `transform` result which would be as if
+ * the transform was the identity transform at the time that rendering started,
+ * and all operations on it are gesture events that occurred while the renderer
+ * was working in background. But actually this is not quite correct.
+ * See the zoom-in discussion in "use case" section in method javadoc.
+ */
} catch (NonInvertibleTransformException e) {
unexpectedException("repaint", e);
}
+ /*
+ * At this point the rendering is completed. If some error occurred, report them.
+ */
isRendering.set(false);
final Throwable ex = task.getException();
if (ex != null) {
@@ -1372,11 +1407,11 @@ public abstract class MapCanvas extends PlanarCanvas {
* This is especially useful when the first gesture event is a tiny change because the user just started
* panning or zooming.
*
- * <div class="note"><b>Design note:</b>
+ * <h4>Design note:</h4>
* using a thread for waiting seems a waste of resources, but a thread (likely this one) is going to be used
* for real after the waiting time is elapsed. That thread usually exists anyway in {@link BackgroundThreads}
* as an idle thread, and it is unlikely that other parts of this JavaFX application need that thread in same
- * time (if it happens, other threads will be created).</div>
+ * time (if it happens, other threads will be created).
*
* @see #requestRepaint()
*/
diff --git a/application/sis-javafx/src/main/java/org/apache/sis/gui/map/MapCanvasAWT.java b/application/sis-javafx/src/main/java/org/apache/sis/gui/map/MapCanvasAWT.java
index ee90198bd7..da79c8359e 100644
--- a/application/sis-javafx/src/main/java/org/apache/sis/gui/map/MapCanvasAWT.java
+++ b/application/sis-javafx/src/main/java/org/apache/sis/gui/map/MapCanvasAWT.java
@@ -40,7 +40,6 @@ import javafx.scene.image.PixelFormat;
import javafx.scene.image.WritableImage;
import javafx.beans.property.ObjectProperty;
import javafx.beans.property.SimpleObjectProperty;
-import javafx.concurrent.Task;
import javafx.util.Callback;
import org.apache.sis.internal.coverage.j2d.ColorModelFactory;
import org.apache.sis.internal.system.Configuration;
@@ -54,7 +53,7 @@ import org.apache.sis.internal.system.Configuration;
* controls by the user.
*
* @author Martin Desruisseaux (Geomatys)
- * @version 1.3
+ * @version 1.4
* @since 1.1
*/
public abstract class MapCanvasAWT extends MapCanvas {
@@ -354,7 +353,7 @@ public abstract class MapCanvasAWT extends MapCanvas {
* @see #requestRepaint()
*/
@Override
- final Task<?> createWorker(final MapCanvas.Renderer mc) {
+ final RenderingTask<?> createWorker(final MapCanvas.Renderer mc) {
assert Platform.isFxApplicationThread();
final Renderer context = (Renderer) mc;
if (!context.isValid(imageMargin.get(), buffer)) {
@@ -370,7 +369,7 @@ public abstract class MapCanvasAWT extends MapCanvas {
* previous resources that we can recycle, either because they have never been created yet or because
* they are not suitable anymore (for example because the image size changed).
*/
- private final class Creator extends Task<WritableImage> {
+ private final class Creator extends RenderingTask<WritableImage> {
/**
* The user-provided object which will perform the actual rendering.
* Its {@link Renderer#paint(Graphics2D)} method will be invoked in background thread.
@@ -467,7 +466,7 @@ public abstract class MapCanvasAWT extends MapCanvas {
* The Java2D volatile image will be rendered in background thread, then its content will be
* transferred to JavaFX image (through {@link BufferedImage} shared array) in JavaFX thread.
*/
- private final class Updater extends Task<VolatileImage> implements Callback<PixelBuffer<IntBuffer>, Rectangle2D> {
+ private final class Updater extends RenderingTask<VolatileImage> implements Callback<PixelBuffer<IntBuffer>, Rectangle2D> {
/**
* The user-provided object which will perform the actual rendering.
* Its {@link Renderer#paint(Graphics2D)} method will be invoked in background thread.
diff --git a/application/sis-javafx/src/main/java/org/apache/sis/gui/map/RenderingTask.java b/application/sis-javafx/src/main/java/org/apache/sis/gui/map/RenderingTask.java
new file mode 100644
index 0000000000..140a642c54
--- /dev/null
+++ b/application/sis-javafx/src/main/java/org/apache/sis/gui/map/RenderingTask.java
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.sis.gui.map;
+
+import javafx.concurrent.Task;
+
+
+/**
+ * Base class of tasks executed in background thread for doing rendering.
+ * This is currently used only for type safety.
+ *
+ * @author Martin Desruisseaux (Geomatys)
+ * @version 1.4
+ *
+ * @param <V> type of value computed by the task.
+ *
+ * @see MapCanvas.Renderer
+ * @see MapCanvas#renderingCompleted(RenderingTask)
+ *
+ * @since 1.4
+ */
+abstract class RenderingTask<V> extends Task<V> {
+ /**
+ * Creates a new rendering task.
+ */
+ RenderingTask() {
+ }
+}
diff --git a/core/sis-feature/src/main/java/org/apache/sis/coverage/grid/GridExtent.java b/core/sis-feature/src/main/java/org/apache/sis/coverage/grid/GridExtent.java
index 35c0f58f8c..a023268b90 100644
--- a/core/sis-feature/src/main/java/org/apache/sis/coverage/grid/GridExtent.java
+++ b/core/sis-feature/src/main/java/org/apache/sis/coverage/grid/GridExtent.java
@@ -830,7 +830,7 @@ public class GridExtent implements GridEnvelope, LenientComparable, Serializable
* The default implementation returns the median (or center) coordinates of this grid extent,
* but subclasses can override this method if another point is considered more representative.
*
- * <p>The {@code anchpr} argument tells {@linkplain GridGeometry#getGridToCRS(PixelInCell) which transform}
+ * <p>The {@code anchor} argument tells {@linkplain GridGeometry#getGridToCRS(PixelInCell) which transform}
* the caller intend to use for converting the grid coordinates to "real world" coordinates.
* With the default implementation, the coordinate values returned with {@code CELL_CORNER}
* are 0.5 cell units higher than the coordinate values returned with {@code CELL_CENTER}.
diff --git a/core/sis-portrayal/src/main/java/org/apache/sis/internal/map/coverage/RenderingData.java b/core/sis-portrayal/src/main/java/org/apache/sis/internal/map/coverage/RenderingData.java
index f23993485a..b02ccdb4c8 100644
--- a/core/sis-portrayal/src/main/java/org/apache/sis/internal/map/coverage/RenderingData.java
+++ b/core/sis-portrayal/src/main/java/org/apache/sis/internal/map/coverage/RenderingData.java
@@ -136,6 +136,7 @@ public class RenderingData implements Cloneable {
/**
* The pyramid level of {@linkplain #data} loaded by the {@linkplain #coverageLoader}.
+ * Value 0 is finest resolution.
*/
private int currentPyramidLevel;
@@ -393,7 +394,7 @@ public class RenderingData implements Cloneable {
domain = r.getImageGeometry(BIDIMENSIONAL);
xyDims = r.getXYDimensions();
}
- setImageSpace(domain, ranges, xyDims);
+ setImageSpace(domain, ranges, xyDims); // Implies `dataGeometry = domain`.
currentSlice = sliceExtent;
data = image;
/*
@@ -420,12 +421,32 @@ public class RenderingData implements Cloneable {
toOld = toNew.inverse();
}
}
- final MathTransform forward = concatenate(PixelInCell.CELL_CORNER, dataGeometry, old, toOld);
- final MathTransform inverse = concatenate(PixelInCell.CELL_CENTER, old, dataGeometry, toNew);
- cornerToObjective = MathTransforms.concatenate(forward, cornerToObjective);
- objectiveToCenter = MathTransforms.concatenate(objectiveToCenter, inverse);
+ /*
+ * `inverse` is the transform from new grid coordinates to old grid coordinates.
+ * `forward` is the converse, with the addition of half-pixel translation terms.
+ */
+ final MathTransform inverse = concatenate(PixelInCell.CELL_CORNER, dataGeometry, old, toOld);
+ final MathTransform forward = concatenate(PixelInCell.CELL_CENTER, old, dataGeometry, toNew);
+ cornerToObjective = MathTransforms.concatenate(inverse, cornerToObjective);
+ objectiveToCenter = MathTransforms.concatenate(objectiveToCenter, forward);
}
return true;
+ /*
+ * Note: the `forward` transform above is of particular interest and may be returned in a future version.
+ * It is the transform from new pixel coordinates to old pixel coordinates of the data before resampling
+ * (i.e. ignoring changes caused by user's zoom or pan gestures on the map). Typical values are:
+ *
+ * • An identity transform, meaning that the data changed but the new data uses the same pixel coordinates
+ * than the previous data. For example the user may have selected a new slice in a three-dimensional cube.
+ * • An affine transform represented by a diagonal matrix, i.e. with only scale factors and no translation.
+ * It happens when there is a change of resolution between the previous data and the new one, for example
+ * because a zoom change caused a change of pyramid level in `MultiResolutionCoverageLoader`.
+ * In such case the scale factors are typically 0.5 (after zoom-in) or 2 (after zoom out).
+ *
+ * That transform has already been applied to `RenderingData` internal state,
+ * but maybe some caller will need to apply that change to its own data.
+ * We wait to see if such need happens.
+ */
}
/**