Skip to content

Commit

Permalink
Make UnitImageFactory synchronized. (#12649)
Browse files Browse the repository at this point in the history
It's already used from multiple threads, per #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.
  • Loading branch information
asvitkine committed Jun 15, 2024
1 parent 1a5fdce commit 1d9c966
Showing 1 changed file with 37 additions and 28 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -83,13 +84,15 @@ public UnitImageFactory(
this.mapData = mapData;
}

@Synchronized
public void clearCache() {
images.clear();
icons.clear();
deleteTempFiles();
colorizedTempFiles.clear();
}

@Synchronized
public void deleteTempFiles() {
tempFiles.forEach(File::delete);
tempFiles.clear();
Expand Down Expand Up @@ -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();
}
Expand All @@ -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<Image> 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<Image> 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());
Expand All @@ -270,7 +282,7 @@ private Image createNoImageImage(ImageKey imageKey) {
return image;
}

public Optional<URL> getBaseImageUrl(final ImageKey imageKey) {
private Optional<URL> getBaseImageUrl(final ImageKey imageKey) {
final String baseImageName = imageKey.getBaseImageName();
final GamePlayer gamePlayer = imageKey.getPlayer();
// URL uses '/' not '\'
Expand All @@ -280,6 +292,7 @@ public Optional<URL> getBaseImageUrl(final ImageKey imageKey) {
return Optional.ofNullable(url);
}

@Synchronized
public Optional<URL> getPossiblyTransformedImageUrl(final ImageKey imageKey) {
final Optional<URL> url = getBaseImageUrl(imageKey);
if (url.isEmpty() || !shouldTransformImage(imageKey)) {
Expand Down Expand Up @@ -309,11 +322,6 @@ public Optional<URL> getPossiblyTransformedImageUrl(final ImageKey imageKey) {
}));
}

private Optional<Image> 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);
Expand Down Expand Up @@ -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(
Expand Down

0 comments on commit 1d9c966

Please sign in to comment.