From b795d486b347286dc7784f3920e1b3e4165c1c63 Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Tue, 25 Apr 2023 10:51:48 -0700 Subject: [PATCH 1/4] Remove repaint throttling in DefaultTool `ZoneRenderer` debounces repaints, so it isn't necessary to throttle updates just to avoid excessive repaints. By not throttling updates, the next `ZoneRenderer` paint will tend to be more up-to-date than it would be with the throttling. --- .../rptools/maptool/client/tool/DefaultTool.java | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/main/java/net/rptools/maptool/client/tool/DefaultTool.java b/src/main/java/net/rptools/maptool/client/tool/DefaultTool.java index f1dc145dcf..b5c5bdab03 100644 --- a/src/main/java/net/rptools/maptool/client/tool/DefaultTool.java +++ b/src/main/java/net/rptools/maptool/client/tool/DefaultTool.java @@ -56,9 +56,7 @@ public abstract class DefaultTool extends Tool protected int mouseY; // This is to manage overflowing of map move events (keep things snappy) - private long lastMoveRedraw; private int mapDX, mapDY; - private static final int REDRAW_DELAY = 25; // millis // TBD private boolean isTouchScreen = false; @@ -294,16 +292,9 @@ public void mouseDragged(MouseEvent e) { setDragStart(mX, mY); - long now = System.currentTimeMillis(); - if (now - lastMoveRedraw > REDRAW_DELAY) { - // TODO: does it matter to capture the last map move in the series ? - // TODO: This should probably be genericized and put into ZoneRenderer to prevent over - // zealous repainting - renderer.moveViewBy(mapDX, mapDY); - mapDX = 0; - mapDY = 0; - lastMoveRedraw = now; - } + renderer.moveViewBy(mapDX, mapDY); + mapDX = 0; + mapDY = 0; } } From 021cdc30dd57ba9b0c31d5fe6ddd66407b60d3a7 Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Tue, 25 Apr 2023 11:09:14 -0700 Subject: [PATCH 2/4] Do not clear images prior to returning them from BufferedImagePool It can be the callers responsibility instead to clear the image if necessary. As it happens, current usage already fills the buffers completely and does not require a prior clearing: - `renderLumensOverlay` and `renderLightOverlay` fill with a solid colour first. - `renderFog` and `paintComponent` render opaquely to the entire buffer. --- .../rptools/maptool/client/ui/zone/BufferedImagePool.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/main/java/net/rptools/maptool/client/ui/zone/BufferedImagePool.java b/src/main/java/net/rptools/maptool/client/ui/zone/BufferedImagePool.java index 60dcd17f82..15b3e5ff7e 100644 --- a/src/main/java/net/rptools/maptool/client/ui/zone/BufferedImagePool.java +++ b/src/main/java/net/rptools/maptool/client/ui/zone/BufferedImagePool.java @@ -14,7 +14,6 @@ */ package net.rptools.maptool.client.ui.zone; -import java.awt.AlphaComposite; import java.awt.GraphicsConfiguration; import java.awt.GraphicsEnvironment; import java.awt.Transparency; @@ -118,11 +117,6 @@ public Handle acquire() { final var instance = available.removeLast(); checkedOut.add(instance); - final var g = instance.createGraphics(); - g.setComposite(AlphaComposite.Clear); - g.fillRect(0, 0, instance.getWidth(), instance.getHeight()); - g.dispose(); - return new Handle(instance); } From 0d0a9305c01adb10789ec74b21a371a869924b59 Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Tue, 25 Apr 2023 11:05:12 -0700 Subject: [PATCH 3/4] Remove unused NotificationOverlay --- .../client/ui/zone/NotificationOverlay.java | 90 ------------------- 1 file changed, 90 deletions(-) delete mode 100644 src/main/java/net/rptools/maptool/client/ui/zone/NotificationOverlay.java diff --git a/src/main/java/net/rptools/maptool/client/ui/zone/NotificationOverlay.java b/src/main/java/net/rptools/maptool/client/ui/zone/NotificationOverlay.java deleted file mode 100644 index 9940b819f1..0000000000 --- a/src/main/java/net/rptools/maptool/client/ui/zone/NotificationOverlay.java +++ /dev/null @@ -1,90 +0,0 @@ -/* - * This software Copyright by the RPTools.net development team, and - * licensed under the Affero GPL Version 3 or, at your option, any later - * version. - * - * MapTool Source Code is distributed in the hope that it will be - * useful, but WITHOUT ANY WARRANTY; without even the implied warranty - * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. - * - * You should have received a copy of the GNU Affero General Public - * License * along with this source Code. If not, please visit - * and specifically the Affero license - * text at . - */ -package net.rptools.maptool.client.ui.zone; - -import java.awt.Graphics2D; -import java.util.List; -import java.util.concurrent.CopyOnWriteArrayList; -import javax.swing.SwingUtilities; -import net.rptools.maptool.client.MapTool; -import net.rptools.maptool.client.swing.Animatable; -import net.rptools.maptool.client.swing.AnimationManager; -import net.rptools.maptool.util.GraphicsUtil; - -public class NotificationOverlay implements ZoneOverlay, Animatable { - - private List eventList = new CopyOnWriteArrayList(); - - // TODO: make this configurable - private static final int MESSAGE_DELAY = 2500; - - public NotificationOverlay() { - AnimationManager.addAnimatable(this); - } - - public void paintOverlay(ZoneRenderer renderer, Graphics2D g) { - - int y = 15; - for (EventDetail detail : eventList) { - - GraphicsUtil.drawBoxedString(g, detail.message, 10, y, SwingUtilities.LEFT); - - y += 20; - } - } - - public void addEvent(String message) { - if (message == null) { - return; - } - - eventList.add(new EventDetail(message)); - } - - private static class EventDetail { - - public long timestamp; - public String message; - - public EventDetail(String message) { - this.message = message; - timestamp = System.currentTimeMillis(); - } - } - - //// - // ANIMATABLE - public void animate() { - - boolean requiresRepaint = false; - while (eventList.size() > 0) { - - EventDetail detail = eventList.get(0); - if (System.currentTimeMillis() - detail.timestamp > MESSAGE_DELAY) { - - eventList.remove(0); - requiresRepaint = true; - } else { - break; - } - } - if (requiresRepaint) { - ZoneRenderer renderer = MapTool.getFrame().getCurrentZoneRenderer(); - if (renderer != null) { - renderer.repaint(); - } - } - } -} From 9a2c1765389bb0cafa8f06571b90a328d6cff36b Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Wed, 26 Apr 2023 00:17:06 -0700 Subject: [PATCH 4/4] Rewrite DebounceExecutor to minimize time spent between tasks The previous implementation would always apply a full delay to any dispatch request, regardless of how long it had been since the last scheduling of a task. This meant that execution A could complete, then execution B could be requested _just before_ the end of the delay time and still be delayed the full time, resulting a double delay. This is noticeable when events are coming in rapid succession (e.g., mouse dragging) as what should be a steady stream of task runs is instead choppy and inconsistent. The new implementation only delays tasks that have come in close succession to a previous task. If a long time has passed, no delay is applied. The new implementation also avoids locks in favour of a lighter weight compare-exchange operation. --- .../maptool/client/DebounceExecutor.java | 61 ++++++++----------- 1 file changed, 24 insertions(+), 37 deletions(-) diff --git a/src/main/java/net/rptools/maptool/client/DebounceExecutor.java b/src/main/java/net/rptools/maptool/client/DebounceExecutor.java index 8576838fa4..e432f23438 100644 --- a/src/main/java/net/rptools/maptool/client/DebounceExecutor.java +++ b/src/main/java/net/rptools/maptool/client/DebounceExecutor.java @@ -14,12 +14,12 @@ */ package net.rptools.maptool.client; -import java.util.Date; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.ScheduledFuture; import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; +import javax.annotation.Nonnull; /** * Manages throttling rapid successive attempts to execute a task. @@ -42,9 +42,6 @@ public class DebounceExecutor { .setNameFormat("debounce-executor-%d") .build(); - /** The time, in milliseconds, during which to throttle subsequent requests to run the task. */ - private final long delay; - /** * A {@link ScheduledExecutorService} that will be used to run the debounced task when the delay * elapses. @@ -52,26 +49,14 @@ public class DebounceExecutor { private final ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor(threadFactory); - /** A {@link ScheduledFuture} that represents the debounced task. */ - private ScheduledFuture future; - - /** - * The synchronization lock used during the critical section for determining how to dispose of any - * single request. - */ - private final Object syncLock = new Object(); + /** The time, in milliseconds, during which to throttle subsequent requests to run the task. */ + private final long delay; /** A {@link Runnable} representing the task to be managed. */ private final Runnable task; - /** - * A {@link long} indicating the time, in milliseconds, when the task last entered a pending - * state. - */ - private long taskPendingSince = -1; - - /** A reference to the logging service. */ - // private static final Logger log = LogManager.getLogger(DebounceExecutor.class); + /** A {@link long} indicating the time, in milliseconds, when the task was last scheduled for. */ + private final AtomicLong taskScheduledTime = new AtomicLong(-1); /** * Initializes a new instance of the {@link DebounceExecutor} class. @@ -85,32 +70,34 @@ public class DebounceExecutor { * executing the task and throttle subsequent requests. * @param task The task to be executed after the delay elapses. */ - public DebounceExecutor(long delay, Runnable task) { + public DebounceExecutor(long delay, @Nonnull Runnable task) { this.delay = delay; this.task = task; } /** Dispatches a task to be executed by this {@link DebounceExecutor} instance. */ public void dispatch() { - if (this.task == null) { - // log.info("Exited debouncer because of a null task."); - return; - } if (this.delay < 1) { this.task.run(); return; } - synchronized (syncLock) { - long now = (new Date()).getTime(); - if (this.taskPendingSince == -1 || now - this.taskPendingSince >= this.delay) { - this.taskPendingSince = now; - this.future = this.executor.schedule(this.task, this.delay, TimeUnit.MILLISECONDS); - } /* else { - log.info( - String.format( - "Task execution was debounced. (now: %d; taskPendingSince: %d; delay: %d; now - taskPendingSince: %d)", - now, this.taskPendingSince, this.delay, now - this.taskPendingSince)); - } */ + + /* + * There are three time windows we need to account for. + * 1. The scheduled time has not yet passed, so we consider the execution redundant. + * 2. The scheduled time has passed, but not by much. So we can run the task with a small delay. + * 3. The scheduled time has long passed, so we can run the task right away. + */ + + final var taskScheduledTime = this.taskScheduledTime.get(); + final var now = System.currentTimeMillis(); + if (now >= taskScheduledTime) { + // This request is not redundant, so we need to schedule it. + final var nextTargetTime = Math.max(now, taskScheduledTime + this.delay); + // If this check fails, that means someone beat us to the punch and our task is now redundant. + if (this.taskScheduledTime.compareAndSet(taskScheduledTime, nextTargetTime)) { + this.executor.schedule(this.task, nextTargetTime - now, TimeUnit.MILLISECONDS); + } } } }