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.

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.Set;
-import java.util.concurrent.CopyOnWriteArrayList;
+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
+	@VisibleForTesting
-	private final List<Overlay> overlays = new CopyOnWriteArrayList<>();
+	static final Comparator<Overlay> 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
+		// For dynamic overlays, higher priority means to
-	private final Map<OverlayLayer, List<Overlay>> overlayLayers = new ConcurrentHashMap<>();
+		// 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<Overlay> overlays = new TreeSet<>(OVERLAY_COMPARATOR);
+
+	private final Map<OverlayLayer, List<Overlay>> 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<Overlay> 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<Overlay> filter)
+	public synchronized boolean removeIf(Predicate<Overlay> 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) ->
+			overlayLayers.get(layer).add(overlay);
-			{
-				if (value == null)
-				{
-					value = new CopyOnWriteArrayList<>();
-				}
-
-				value.add(overlay);
-				return value;
-			});
 		}
+
+		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<Overlay> 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());
-		});
-	}
 }

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<Overlay> overlays = overlayManager.getOverlayLayers().get(layer);
+		final List<Overlay> 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 (SwingUtilities.isRightMouseButton(mouseEvent))
-					if (overlay.getPosition() != OverlayPosition.DETACHED)
 					{
-						overlay.setPreferredPosition(null);
+						// detached overlays have no place to reset back to
-						overlay.setPreferredSize(null);
+						if (overlay.getPosition() != OverlayPosition.DETACHED)
-						overlay.setPreferredLocation(null);
+						{
-						overlayManager.resetOverlay(overlay);
+							overlay.setPreferredPosition(null);
+							overlay.setPreferredSize(null);
+							overlay.setPreferredLocation(null);
+							overlayManager.resetOverlay(overlay);
+						}
 					}
-				}
+					else
-				else
+					{
-				{
+						final Point offset = new Point(mousePoint.x, mousePoint.y);
-					final Point offset = new Point(mousePoint.x, mousePoint.y);
+						offset.translate(-overlay.getBounds().x, -overlay.getBounds().y);
-					offset.translate(-overlay.getBounds().x, -overlay.getBounds().y);
+						overlayOffset.setLocation(offset);
-					overlayOffset.setLocation(offset);
 
-					mousePoint.translate(-offset.x, -offset.y);
+						mousePoint.translate(-offset.x, -offset.y);
-					movedOverlay = overlay;
+						movedOverlay = overlay;
-					movedOverlay.setPreferredPosition(null);
+						movedOverlay.setPreferredPosition(null);
-					movedOverlay.setPreferredLocation(mousePoint);
+						movedOverlay.setPreferredLocation(mousePoint);
-					overlayManager.saveOverlay(movedOverlay);
+						overlayManager.saveOverlay(movedOverlay);
-				}
+					}
 
-				mouseEvent.consume();
+					mouseEvent.consume();
-				break;
+					break;
+				}
 			}
 		}
 

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<Overlay> 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<Overlay> 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<Overlay> 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));