From e0d70ab382d6b113729d945549eab27801cd128f Mon Sep 17 00:00:00 2001 From: Max Weber Date: Tue, 12 Jun 2018 20:49:29 -0600 Subject: [PATCH] runelite-client: Make OverlayManager thread safe There was something happening where rebuildOverlayLayers would race itself almost 100% of the time, preventing overlays from being removed. --- .../client/ui/overlay/OverlayManager.java | 112 +++++++++--------- .../client/ui/overlay/OverlayRenderer.java | 51 ++++---- .../client/ui/overlay/OverlayManagerTest.java | 6 +- 3 files changed, 89 insertions(+), 80 deletions(-) diff --git a/runelite-client/src/main/java/net/runelite/client/ui/overlay/OverlayManager.java b/runelite-client/src/main/java/net/runelite/client/ui/overlay/OverlayManager.java index 8ba84c013b..a0a75cd104 100644 --- a/runelite-client/src/main/java/net/runelite/client/ui/overlay/OverlayManager.java +++ b/runelite-client/src/main/java/net/runelite/client/ui/overlay/OverlayManager.java @@ -27,13 +27,18 @@ package net.runelite.client.ui.overlay; import com.google.common.annotations.VisibleForTesting; import java.awt.Dimension; import java.awt.Point; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.CopyOnWriteArrayList; +import java.util.Set; +import java.util.TreeSet; import java.util.function.Predicate; import javax.inject.Inject; import javax.inject.Singleton; +import lombok.AccessLevel; import lombok.Getter; import net.runelite.client.config.ConfigGroup; import net.runelite.client.config.ConfigManager; @@ -50,11 +55,34 @@ public class OverlayManager private static final String OVERLAY_CONFIG_PREFERRED_SIZE = "_preferredSize"; private static final String RUNELITE_CONFIG_GROUP_NAME = RuneLiteConfig.class.getAnnotation(ConfigGroup.class).keyName(); - @Getter - private final List overlays = new CopyOnWriteArrayList<>(); + @VisibleForTesting + static final Comparator OVERLAY_COMPARATOR = (a, b) -> + { + if (a.getPosition() != b.getPosition()) + { + // This is so non-dynamic overlays render after dynamic + // overlays, which are generally in the scene + return a.getPosition().compareTo(b.getPosition()); + } - @Getter - private final Map> overlayLayers = new ConcurrentHashMap<>(); + // For dynamic overlays, higher priority means to + // draw *later* so it is on top. + // For non-dynamic overlays, higher priority means + // draw *first* so that they are closer to their + // defined position. + return a.getPosition() == OverlayPosition.DYNAMIC + ? a.getPriority().compareTo(b.getPriority()) + : b.getPriority().compareTo(a.getPriority()); + }; + + /** + * Sorted set of overlays + * All access to this must be guarded by a lock on this OverlayManager + */ + @Getter(AccessLevel.PACKAGE) + private final Set overlays = new TreeSet<>(OVERLAY_COMPARATOR); + + private final Map> overlayLayers = new HashMap<>(); private final ConfigManager configManager; @@ -64,13 +92,24 @@ public class OverlayManager this.configManager = configManager; } + /** + * Gets all of the overlays on a layer + * + * @param layer the layer + * @return An immutable list of all of the overlays on that layer + */ + synchronized List getLayer(OverlayLayer layer) + { + return overlayLayers.get(layer); + } + /** * Add overlay. * * @param overlay the overlay * @return true if overlay was added */ - public boolean add(final Overlay overlay) + public synchronized boolean add(final Overlay overlay) { final boolean add = overlays.add(overlay); @@ -82,7 +121,6 @@ public class OverlayManager overlay.setPreferredSize(size); final OverlayPosition position = loadOverlayPosition(overlay); overlay.setPreferredPosition(position); - sortOverlays(overlays); rebuildOverlayLayers(); } @@ -95,13 +133,12 @@ public class OverlayManager * @param overlay the overlay * @return true if overlay was removed */ - public boolean remove(final Overlay overlay) + public synchronized boolean remove(final Overlay overlay) { final boolean remove = overlays.remove(overlay); if (remove) { - sortOverlays(overlays); rebuildOverlayLayers(); } @@ -114,10 +151,9 @@ public class OverlayManager * @param filter the filter * @return true if any overlay was removed */ - public boolean removeIf(Predicate filter) + public synchronized boolean removeIf(Predicate filter) { final boolean removeIf = overlays.removeIf(filter); - sortOverlays(overlays); rebuildOverlayLayers(); return removeIf; } @@ -125,10 +161,9 @@ public class OverlayManager /** * Clear all overlays */ - public void clear() + public synchronized void clear() { overlays.clear(); - sortOverlays(overlays); rebuildOverlayLayers(); } @@ -137,12 +172,11 @@ public class OverlayManager * * @param overlay overlay to save */ - public void saveOverlay(final Overlay overlay) + public synchronized void saveOverlay(final Overlay overlay) { saveOverlayPosition(overlay); saveOverlaySize(overlay); saveOverlayLocation(overlay); - sortOverlays(overlays); rebuildOverlayLayers(); } @@ -151,7 +185,7 @@ public class OverlayManager * * @param overlay overlay to reset */ - public void resetOverlay(final Overlay overlay) + public synchronized void resetOverlay(final Overlay overlay) { final String locationKey = overlay.getName() + OVERLAY_CONFIG_PREFERRED_LOCATION; final String positionKey = overlay.getName() + OVERLAY_CONFIG_PREFERRED_POSITION; @@ -159,13 +193,15 @@ public class OverlayManager configManager.unsetConfiguration(RUNELITE_CONFIG_GROUP_NAME, locationKey); configManager.unsetConfiguration(RUNELITE_CONFIG_GROUP_NAME, positionKey); configManager.unsetConfiguration(RUNELITE_CONFIG_GROUP_NAME, sizeKey); - sortOverlays(overlays); rebuildOverlayLayers(); } - private void rebuildOverlayLayers() + private synchronized void rebuildOverlayLayers() { - overlayLayers.clear(); + for (OverlayLayer l : OverlayLayer.values()) + { + overlayLayers.put(l, new ArrayList<>()); + } for (final Overlay overlay : overlays) { @@ -181,17 +217,10 @@ public class OverlayManager } } - overlayLayers.compute(layer, (key, value) -> - { - if (value == null) - { - value = new CopyOnWriteArrayList<>(); - } - - value.add(overlay); - return value; - }); + overlayLayers.get(layer).add(overlay); } + + overlayLayers.forEach((layer, value) -> overlayLayers.put(layer, Collections.unmodifiableList(value))); } private void saveOverlayLocation(final Overlay overlay) @@ -265,27 +294,4 @@ public class OverlayManager final String locationKey = overlay.getName() + OVERLAY_CONFIG_PREFERRED_POSITION; return configManager.getConfiguration(RUNELITE_CONFIG_GROUP_NAME, locationKey, OverlayPosition.class); } - - @VisibleForTesting - static void sortOverlays(List overlays) - { - overlays.sort((a, b) -> - { - if (a.getPosition() != b.getPosition()) - { - // This is so non-dynamic overlays render after dynamic - // overlays, which are generally in the scene - return a.getPosition().compareTo(b.getPosition()); - } - - // For dynamic overlays, higher priority means to - // draw *later* so it is on top. - // For non-dynamic overlays, higher priority means - // draw *first* so that they are closer to their - // defined position. - return a.getPosition() == OverlayPosition.DYNAMIC - ? a.getPriority().compareTo(b.getPriority()) - : b.getPriority().compareTo(a.getPriority()); - }); - } } \ No newline at end of file diff --git a/runelite-client/src/main/java/net/runelite/client/ui/overlay/OverlayRenderer.java b/runelite-client/src/main/java/net/runelite/client/ui/overlay/OverlayRenderer.java index 7b1568a9d3..6463b61119 100644 --- a/runelite-client/src/main/java/net/runelite/client/ui/overlay/OverlayRenderer.java +++ b/runelite-client/src/main/java/net/runelite/client/ui/overlay/OverlayRenderer.java @@ -123,7 +123,7 @@ public class OverlayRenderer extends MouseListener implements KeyListener public void render(Graphics2D graphics, final OverlayLayer layer) { final Client client = clientProvider.get(); - final List overlays = overlayManager.getOverlayLayers().get(layer); + final List overlays = overlayManager.getLayer(layer); if (client == null || overlays == null @@ -245,36 +245,39 @@ public class OverlayRenderer extends MouseListener implements KeyListener final Point mousePoint = mouseEvent.getPoint(); mousePosition.setLocation(mousePoint); - for (Overlay overlay : overlayManager.getOverlays()) + synchronized (overlayManager) { - if (overlay.getBounds().contains(mousePoint)) + for (Overlay overlay : overlayManager.getOverlays()) { - if (SwingUtilities.isRightMouseButton(mouseEvent)) + if (overlay.getBounds().contains(mousePoint)) { - // detached overlays have no place to reset back to - if (overlay.getPosition() != OverlayPosition.DETACHED) + if (SwingUtilities.isRightMouseButton(mouseEvent)) { - overlay.setPreferredPosition(null); - overlay.setPreferredSize(null); - overlay.setPreferredLocation(null); - overlayManager.resetOverlay(overlay); + // detached overlays have no place to reset back to + if (overlay.getPosition() != OverlayPosition.DETACHED) + { + overlay.setPreferredPosition(null); + overlay.setPreferredSize(null); + overlay.setPreferredLocation(null); + overlayManager.resetOverlay(overlay); + } } - } - else - { - final Point offset = new Point(mousePoint.x, mousePoint.y); - offset.translate(-overlay.getBounds().x, -overlay.getBounds().y); - overlayOffset.setLocation(offset); + else + { + final Point offset = new Point(mousePoint.x, mousePoint.y); + offset.translate(-overlay.getBounds().x, -overlay.getBounds().y); + overlayOffset.setLocation(offset); - mousePoint.translate(-offset.x, -offset.y); - movedOverlay = overlay; - movedOverlay.setPreferredPosition(null); - movedOverlay.setPreferredLocation(mousePoint); - overlayManager.saveOverlay(movedOverlay); - } + mousePoint.translate(-offset.x, -offset.y); + movedOverlay = overlay; + movedOverlay.setPreferredPosition(null); + movedOverlay.setPreferredLocation(mousePoint); + overlayManager.saveOverlay(movedOverlay); + } - mouseEvent.consume(); - break; + mouseEvent.consume(); + break; + } } } diff --git a/runelite-client/src/test/java/net/runelite/client/ui/overlay/OverlayManagerTest.java b/runelite-client/src/test/java/net/runelite/client/ui/overlay/OverlayManagerTest.java index 808d023640..7e1c0c5eda 100644 --- a/runelite-client/src/test/java/net/runelite/client/ui/overlay/OverlayManagerTest.java +++ b/runelite-client/src/test/java/net/runelite/client/ui/overlay/OverlayManagerTest.java @@ -55,7 +55,7 @@ public class OverlayManagerTest Overlay tlh = new TestOverlay(OverlayPosition.TOP_LEFT, OverlayPriority.HIGH); Overlay tll = new TestOverlay(OverlayPosition.TOP_LEFT, OverlayPriority.LOW); List overlays = Arrays.asList(tlh, tll); - OverlayManager.sortOverlays(overlays); + overlays.sort(OverlayManager.OVERLAY_COMPARATOR); assertEquals(tlh, overlays.get(0)); assertEquals(tll, overlays.get(1)); } @@ -67,7 +67,7 @@ public class OverlayManagerTest Overlay tlh = new TestOverlay(OverlayPosition.TOP_LEFT, OverlayPriority.HIGH); Overlay dyn = new TestOverlay(OverlayPosition.DYNAMIC, OverlayPriority.HIGH); List overlays = Arrays.asList(tlh, dyn); - OverlayManager.sortOverlays(overlays); + overlays.sort(OverlayManager.OVERLAY_COMPARATOR); assertEquals(dyn, overlays.get(0)); assertEquals(tlh, overlays.get(1)); } @@ -80,7 +80,7 @@ public class OverlayManagerTest Overlay dyn = new TestOverlay(OverlayPosition.DYNAMIC, OverlayPriority.HIGH); Overlay tlh = new TestOverlay(OverlayPosition.TOP_LEFT, OverlayPriority.HIGH); List overlays = Arrays.asList(t, dyn, tlh); - OverlayManager.sortOverlays(overlays); + overlays.sort(OverlayManager.OVERLAY_COMPARATOR); assertEquals(dyn, overlays.get(0)); assertEquals(tlh, overlays.get(1)); assertEquals(t, overlays.get(2));