Better synchronization

README.md

@@ -1,3 +1,9 @@
-AsyncPackScan makes pack scanning in the resource pack organizer screen asynchronous.
+In vanilla minecraft, the list of resource packs is refreshed by scanning all possible sources synchronously.
 
-This has unintended consequences because no proper support for concurrency is in place, but massively speeds up the screen with some mod combinations.
+Generally, this is not a problem, but in the resource pack organizer screen, this scan is performed whenever it is resized and every twenty ticks.
+
+If another mod (such as respackopts) hooks into the pack scan and increases its duration even slightly, this leads to major lag spikes and makes the screen near-unusable when using more than a few packs.
+
+This mod fixes that issue by moving this computation to another thread and scheduling a scan task on those events instead, drastically improving perceived performance.
+
+Since the vanilla code is designed for synchronous execution, this can cause issues like crashes in edge cases, but worked well enough in my testing.

build.gradle.kts

@@ -4,6 +4,10 @@ plugins {
     id("jfmod") version "1.3-SNAPSHOT"
 }
 
+loom {
+    accessWidenerPath.set(file("src/client/resources/async-pack-scan.accesswidener"))
+}
+
 dependencies {
     modImplementation("io.gitlab.jfronny.libjf:libjf-base:${prop("libjf_version")}")
 

src/client/java/io/gitlab/jfronny/aps/client/impl/ResourcePackOrganizerLockState.java

@@ -0,0 +1,50 @@
+package io.gitlab.jfronny.aps.client.impl;
+
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class ResourcePackOrganizerLockState {
+    private final Lock resourcesLock = new ReentrantLock();
+
+    public SafeCloseable lockResources() {
+        resourcesLock.lock();
+        return resourcesLock::unlock;
+    }
+
+    private boolean isRunning = false;
+    private boolean isScheduled = false;
+    private final Lock scanStateLock = new ReentrantLock();
+
+    public ScanFinishedResponse emitScanFinished() {
+        scanStateLock.lock();
+        try {
+            if (isScheduled) {
+                isScheduled = false;
+                return new ScanFinishedResponse(true);
+            } else {
+                isRunning = false;
+                return new ScanFinishedResponse(false);
+            }
+        } finally {
+            scanStateLock.unlock();
+        }
+    }
+
+    public RequestScanResponse requestScan() {
+        scanStateLock.lock();
+        try {
+            if (isRunning) {
+                isScheduled = true;
+                return new RequestScanResponse(false);
+            } else {
+                isRunning = true;
+                return new RequestScanResponse(true);
+            }
+        } finally {
+            scanStateLock.unlock();
+        }
+    }
+
+    public record ScanFinishedResponse(boolean shouldContinue) {}
+    public record RequestScanResponse(boolean shouldStart) {}
+}

src/client/java/io/gitlab/jfronny/aps/client/impl/SafeCloseable.java

@@ -0,0 +1,6 @@
+package io.gitlab.jfronny.aps.client.impl;
+
+public interface SafeCloseable extends AutoCloseable {
+    @Override
+    void close();
+}

src/client/java/io/gitlab/jfronny/aps/client/mixin/ResourcePackOrganizerMixin.java

@@ -1,27 +1,27 @@
 package io.gitlab.jfronny.aps.client.mixin;
 
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Lists;
+import io.gitlab.jfronny.aps.client.impl.ResourcePackOrganizerLockState;
 import io.gitlab.jfronny.libjf.LibJf;
 import net.minecraft.client.gui.screen.pack.ResourcePackOrganizer;
 import net.minecraft.resource.ResourcePackManager;
 import net.minecraft.resource.ResourcePackProfile;
 import org.spongepowered.asm.mixin.*;
-import org.spongepowered.asm.mixin.injection.At;
-import org.spongepowered.asm.mixin.injection.Inject;
-import org.spongepowered.asm.mixin.injection.callback.CallbackInfo;
 
 import java.util.List;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
+import java.util.stream.Stream;
 
 @Mixin(ResourcePackOrganizer.class)
-public class ResourcePackOrganizerMixin {
-    @Shadow
-    @Final
-    private ResourcePackManager resourcePackManager;
-    @Shadow @Final private List<ResourcePackProfile> enabledPacks;
-    @Shadow @Final private List<ResourcePackProfile> disabledPacks;
+public abstract class ResourcePackOrganizerMixin {
+    @Shadow @Final private ResourcePackManager resourcePackManager;
+    @Shadow @Final List<ResourcePackProfile> enabledPacks;
+    @Shadow @Final List<ResourcePackProfile> disabledPacks;
 
     @Unique private Future<Void> aps$packScan = null;
+    @Unique private final ResourcePackOrganizerLockState aps$lock = new ResourcePackOrganizerLockState();
 
     /**
      * @author JFronny
@@ -29,25 +29,77 @@ public class ResourcePackOrganizerMixin {
      */
     @Overwrite
     public void refresh() {
-        if (aps$packScan != null) aps$packScan.cancel(true);
-        Future<Void>[] task = new Future[1];
-        aps$packScan = task[0] = resourcePackManager.scanPacksAsync(() -> {
+        if (aps$lock.requestScan().shouldStart()) aps$startScan();
+    }
+
+    private void aps$startScan() {
+        Future<Void>[] tasks = new Future[1];
+        aps$packScan = tasks[0] = resourcePackManager.scanPacksAsync(() -> aps$afterScan(tasks[0]));
+    }
+
+    private void aps$afterScan(Future<Void> task) {
+        try (var lock = aps$lock.lockResources()) {
+            if (task.isCancelled()) return;
             enabledPacks.retainAll(resourcePackManager.getProfiles());
             disabledPacks.clear();
             disabledPacks.addAll(resourcePackManager.getProfiles());
             disabledPacks.removeAll(enabledPacks);
-            if (aps$packScan == task[0]) aps$packScan = null;
-        });
+            if (aps$packScan == task) aps$packScan = null;
+        }
+        if (aps$lock.emitScanFinished().shouldContinue()) aps$startScan();
     }
 
-    @Inject(method = "apply()V", at = @At("HEAD"))
-    void onApply(CallbackInfo ci) {
-        if (aps$packScan != null) {
-            try {
+    /**
+     * @author JFronny
+     * @reason Inject lock and resynchronize from pack scan
+     */
+    @Overwrite
+    public void refreshEnabledProfiles() {
+        try {
+            if (aps$packScan != null) {
                 aps$packScan.get();
-            } catch (InterruptedException | ExecutionException e) {
-                LibJf.LOGGER.error("Pack scan was interrupted", e);
             }
+            try (var lock = aps$lock.lockResources()) {
+                // Vanilla statement, copied into here
+                this.resourcePackManager.setEnabledProfiles(
+                        Lists.reverse(this.enabledPacks)
+                                .stream()
+                                .map(ResourcePackProfile::getName)
+                                .collect(ImmutableList.toImmutableList())
+                );
+            }
+        } catch (InterruptedException | ExecutionException e) {
+            LibJf.LOGGER.error("Pack scan was interrupted", e);
+        }
+    }
+
+    /**
+     * @author JFronny
+     * @reason Inject lock
+     */
+    @Overwrite
+    public Stream<ResourcePackOrganizer.Pack> getDisabledPacks() {
+        try (var lock = aps$lock.lockResources()) {
+            return this.disabledPacks
+                    .stream()
+                    .<ResourcePackOrganizer.Pack>map(pack -> ((ResourcePackOrganizer) (Object) this).new DisabledPack(pack))
+                    .toList()
+                    .stream();
+        }
+    }
+
+    /**
+     * @author JFronny
+     * @reason Inject lock
+     */
+    @Overwrite
+    public Stream<ResourcePackOrganizer.Pack> getEnabledPacks() {
+        try (var lock = aps$lock.lockResources()) {
+            return this.enabledPacks
+                    .stream()
+                    .<ResourcePackOrganizer.Pack>map(pack -> ((ResourcePackOrganizer) (Object) this).new EnabledPack(pack))
+                    .toList()
+                    .stream();
         }
     }
 }

src/client/resources/async-pack-scan.accesswidener

@@ -0,0 +1,5 @@
+accessWidener	v2	named
+
+accessible class net/minecraft/client/gui/screen/pack/ResourcePackOrganizer$DisabledPack
+
+accessible class net/minecraft/client/gui/screen/pack/ResourcePackOrganizer$EnabledPack

src/main/java/io/gitlab/jfronny/aps/impl/AsyncPackScan.java

@@ -0,0 +1,7 @@
+package io.gitlab.jfronny.aps.impl;
+
+import io.gitlab.jfronny.commons.log.Logger;
+
+public class AsyncPackScan {
+    public static final Logger LOG = Logger.forName("async-pack-scan");
+}

src/main/resources/fabric.mod.json

@@ -21,6 +21,7 @@
       "environment": "client"
     }
   ],
+  "accessWidener": "async-pack-scan.accesswidener",
   "depends": {
     "fabricloader": ">=0.12.0",
     "minecraft": "*",