From 1d9c966ba1ee0c40369c5093c0189776cfa8b2fd Mon Sep 17 00:00:00 2001 From: asvitkine Date: Sat, 15 Jun 2024 14:51:54 -0400 Subject: [PATCH] Make UnitImageFactory synchronized. (#12649) It's already used from multiple threads, per https://github.com/triplea-game/triplea/issues/12623, so making the methods that access internal collections synchronized to fix ConcurrentModificationException. This is using the lombok synchronized annotation which is better than the Java keyword since it keeps an internal lock object. --- .../triplea/image/UnitImageFactory.java | 65 +++++++++++-------- 1 file changed, 37 insertions(+), 28 deletions(-) diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/image/UnitImageFactory.java b/game-app/game-core/src/main/java/games/strategy/triplea/image/UnitImageFactory.java index f405f7d3ed..01a2ce6bca 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/image/UnitImageFactory.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/image/UnitImageFactory.java @@ -34,10 +34,11 @@ import javax.swing.ImageIcon; import lombok.Builder; import lombok.Getter; +import lombok.Synchronized; import lombok.Value; import lombok.extern.slf4j.Slf4j; -/** A factory with an image cache for creating unit images. */ +/** A factory with a thread-safe image cache for creating unit images. */ @Slf4j public class UnitImageFactory { public static final int DEFAULT_UNIT_ICON_SIZE = 48; @@ -83,6 +84,7 @@ public UnitImageFactory( this.mapData = mapData; } + @Synchronized public void clearCache() { images.clear(); icons.clear(); @@ -90,6 +92,7 @@ public void clearCache() { colorizedTempFiles.clear(); } + @Synchronized public void deleteTempFiles() { tempFiles.forEach(File::delete); tempFiles.clear(); @@ -222,6 +225,7 @@ public int getUnitCounterOffsetHeight() { return (int) (scaleFactor * unitCounterOffsetHeight); } + @Synchronized public boolean hasImage(final ImageKey imageKey) { return images.containsKey(imageKey) || getBaseImageUrl(imageKey).isPresent(); } @@ -230,32 +234,40 @@ public boolean hasImage(final ImageKey imageKey) { * Return the appropriate scaled unit image. If an image cannot be found, a placeholder 'no-image' * image is returned. */ + @Synchronized public Image getImage(final ImageKey imageKey) { return Optional.ofNullable(images.get(imageKey)) - .or( - () -> - getTransformedImage(imageKey) - .map( - baseImage -> { - // We want to scale units according to the given scale factor. - // We use smooth scaling since the images are cached to allow to take our - // time in doing the scaling. - // Image observer is null, since the image should have been guaranteed to - // be loaded. - final int baseWidth = baseImage.getWidth(null); - final int baseHeight = baseImage.getHeight(null); - final int width = Math.max(1, (int) (baseWidth * scaleFactor)); - final int height = Math.max(1, (int) (baseHeight * scaleFactor)); - final Image scaledImage = - baseImage.getScaledInstance(width, height, Image.SCALE_SMOOTH); - // Ensure the scaling is completed. - Util.ensureImageLoaded(scaledImage); - images.put(imageKey, scaledImage); - return scaledImage; - })) + .or(() -> getTransformedScaledImage(imageKey)) .orElseGet(() -> createNoImageImage(imageKey)); } + private Optional getTransformedScaledImage(final ImageKey imageKey) { + return getTransformedImage(imageKey) + .map( + baseImage -> { + // We want to scale units according to the given scale factor. + // We use smooth scaling since the images are cached to allow to take our + // time in doing the scaling. + // Image observer is null, since the image should have been guaranteed to + // be loaded. + final int baseWidth = baseImage.getWidth(null); + final int baseHeight = baseImage.getHeight(null); + final int width = Math.max(1, (int) (baseWidth * scaleFactor)); + final int height = Math.max(1, (int) (baseHeight * scaleFactor)); + final Image scaledImage = + baseImage.getScaledInstance(width, height, Image.SCALE_SMOOTH); + // Ensure the scaling is completed. + Util.ensureImageLoaded(scaledImage); + images.put(imageKey, scaledImage); + return scaledImage; + }); + } + + private Optional getTransformedImage(final ImageKey imageKey) { + return getBaseImageUrl(imageKey) + .map(imageLocation -> loadImageAndTransform(imageLocation, imageKey)); + } + private Image createNoImageImage(ImageKey imageKey) { BufferedImage image = resourceLoader.getImageOrThrow(FILE_NAME_BASE + "missing_unit_image.png"); Color playerColor = mapData.getPlayerColor(imageKey.getPlayer().getName()); @@ -270,7 +282,7 @@ private Image createNoImageImage(ImageKey imageKey) { return image; } - public Optional getBaseImageUrl(final ImageKey imageKey) { + private Optional getBaseImageUrl(final ImageKey imageKey) { final String baseImageName = imageKey.getBaseImageName(); final GamePlayer gamePlayer = imageKey.getPlayer(); // URL uses '/' not '\' @@ -280,6 +292,7 @@ public Optional getBaseImageUrl(final ImageKey imageKey) { return Optional.ofNullable(url); } + @Synchronized public Optional getPossiblyTransformedImageUrl(final ImageKey imageKey) { final Optional url = getBaseImageUrl(imageKey); if (url.isEmpty() || !shouldTransformImage(imageKey)) { @@ -309,11 +322,6 @@ public Optional getPossiblyTransformedImageUrl(final ImageKey imageKey) { })); } - private Optional getTransformedImage(final ImageKey imageKey) { - return getBaseImageUrl(imageKey) - .map(imageLocation -> loadImageAndTransform(imageLocation, imageKey)); - } - private Image loadImageAndTransform(URL imageLocation, ImageKey imageKey) { Image image = Toolkit.getDefaultToolkit().getImage(imageLocation); Util.ensureImageLoaded(image); @@ -376,6 +384,7 @@ public Image getHighlightImage(final ImageKey imageKey) { } /** Return an _unscaled_ icon image for a unit. */ + @Synchronized public ImageIcon getIcon(final ImageKey imageKey) { final String fullName = imageKey.getFullName(); return icons.computeIfAbsent(