From 01c8f20a65aef69a7a67d5bd96e2165df87c617b Mon Sep 17 00:00:00 2001 From: Kitlith Date: Tue, 21 Jul 2020 18:16:17 -0700 Subject: [PATCH 1/2] 99% of the way to the alternative implementation I've been talking about Only issue is: once again, the lambda is static, and that *doesn't work for this* because we need the lootManager to fire the event. I do have a couple ideas revolving around doing something hacky with JsonElement, and doing something hacky with ImmutableMap.Builder, but this is my current status. --- .../net/patchworkmc/impl/loot/LootHooks.java | 140 ++++++------------ .../mixin/loot/MixinLootManager.java | 45 +++--- 2 files changed, 75 insertions(+), 110 deletions(-) diff --git a/patchwork-loot/src/main/java/net/patchworkmc/impl/loot/LootHooks.java b/patchwork-loot/src/main/java/net/patchworkmc/impl/loot/LootHooks.java index 93492205..f268b83e 100644 --- a/patchwork-loot/src/main/java/net/patchworkmc/impl/loot/LootHooks.java +++ b/patchwork-loot/src/main/java/net/patchworkmc/impl/loot/LootHooks.java @@ -19,18 +19,12 @@ package net.patchworkmc.impl.loot; -import java.util.Deque; -import java.util.HashSet; - -import javax.annotation.Nullable; - -import com.google.common.collect.Queues; -import com.google.common.collect.Sets; import com.google.gson.Gson; +import com.google.gson.JsonArray; import com.google.gson.JsonElement; import com.google.gson.JsonObject; import com.google.gson.JsonParseException; -import org.spongepowered.asm.mixin.Unique; +import org.apache.commons.lang3.mutable.MutableInt; import net.minecraft.loot.LootManager; import net.minecraft.loot.LootTable; @@ -39,111 +33,75 @@ import net.patchworkmc.impl.event.loot.LootEvents; -// NOTE: this class is more or less a direct copy of parts of Forge's ForgeHooks. public class LootHooks { - @Unique - private static ThreadLocal> lootContext = new ThreadLocal>(); - - public static LootTable loadLootTable(Gson gson, Identifier name, JsonElement data, boolean custom, LootManager lootTableManager) { - Deque que = lootContext.get(); - - if (que == null) { - que = Queues.newArrayDeque(); - lootContext.set(que); - } - - LootTable ret = null; - - try { - que.push(new LootTableContext(name, custom)); - ret = gson.fromJson(data, LootTable.class); - que.pop(); - } catch (JsonParseException e) { - que.pop(); - throw e; - } - - if (!custom) { - ret = LootEvents.loadLootTable(name, ret, lootTableManager); - } - - // if (ret != null) { - // ret.freeze(); - // } - - return ret; - } - - private static LootTableContext getLootTableContext() { - LootTableContext ctx = lootContext.get().peek(); - - if (ctx == null) { - throw new JsonParseException("Invalid call stack, could not grab json context!"); // Should I throw this? Do we care about custom deserializers outside the manager? + public static void prepareLootTable(Identifier id, JsonObject lootTableObj, boolean custom, LootManager lootTableManager) { + // passing 'custom' value into lambda mixin via json after json parsing + lootTableObj.addProperty("custom", custom); + + // passing pool names into loot pool deserialization mixin since we have all the information we need now + if (lootTableObj.has("pools")) { + MutableInt poolCount = new MutableInt(0); + JsonArray pools = lootTableObj.getAsJsonArray("pools"); + + for (JsonElement elem: pools) { + JsonObject pool = JsonHelper.asObject(elem, "loot pool"); + String name = getPoolName(pool, id, custom, poolCount); + pool.addProperty("name", name); + } } - return ctx; + // we don't call GSON here because we're letting vanilla do it in a bit. + // ForgeHooks implementation wraps this, performing the deserialization } - public static String readPoolName(JsonObject json) { - LootTableContext ctx = LootHooks.getLootTableContext(); - ctx.resetPoolCtx(); + // More or less a copy of ForgeHooks.readPoolName. Actual implementation of that will just pull the name + // from the json, since prepareLootTable is taking the name from this and putting it in the json. + private static String getPoolName(JsonObject lootPoolObj, Identifier id, boolean custom, MutableInt poolCount) { + boolean vanilla = "minecraft".equals(id.getNamespace()); - if (json.has("name")) { - return JsonHelper.getString(json, "name"); + if (lootPoolObj.has("name")) { + return JsonHelper.getString(lootPoolObj, "name"); } - if (ctx.custom) { - return "custom#" + json.hashCode(); //We don't care about custom ones modders shouldn't be editing them! + if (custom) { + return "custom#" + lootPoolObj.hashCode(); //We don't care about custom ones modders shouldn't be editing them! } - ctx.poolCount++; + poolCount.increment(); - if (!ctx.vanilla) { + if (!vanilla) { throw new JsonParseException("Loot Table \"" + ctx.name.toString() + "\" Missing `name` entry for pool #" + (ctx.poolCount - 1)); } - return ctx.poolCount == 1 ? "main" : "pool" + (ctx.poolCount - 1); + return poolCount.intValue() == 1 ? "main" : "pool" + (poolCount.intValue() - 1); } - private static class LootTableContext { - public final Identifier name; - public final boolean custom; - private final boolean vanilla; - public int poolCount = 0; - public int entryCount = 0; - private HashSet entryNames = Sets.newHashSet(); - - private LootTableContext(Identifier name, boolean custom) { - this.name = name; - this.custom = custom; - this.vanilla = "minecraft".equals(this.name.getNamespace()); - } + // TODO: should we move this implementation to ForgeHooks? since this is only going to be called by ForgeHooks. + public static LootTable loadLootTable(Gson gson, Identifier name, JsonElement data, boolean custom, LootManager lootTableManager) { + JsonObject lootTableObj = JsonHelper.asObject(data, "loot table"); - private void resetPoolCtx() { - this.entryCount = 0; - this.entryNames.clear(); - } + // This accomplishes the stashing of data that Forge does, without needing a threadlocal static + prepareLootTable(name, lootTableObj, custom, lootTableManager); - public String validateEntryName(@Nullable String name) { - if (name != null && !this.entryNames.contains(name)) { - this.entryNames.add(name); - return name; - } + // TODO: Can we do something like call the lambda method directly? + // As is, we're gonna duplicate the stuff we add via mixin. - if (!this.vanilla) { - throw new JsonParseException("Loot Table \"" + this.name.toString() + "\" Duplicate entry name \"" + name + "\" for pool #" + (this.poolCount - 1) + " entry #" + (this.entryCount - 1)); - } + LootTable ret = gson.fromJson(data, LootTable.class); - int x = 0; + if (!custom) { + ret = LootEvents.loadLootTable(name, ret, lootTableManager); + } - while (this.entryNames.contains(name + "#" + x)) { - x++; - } + // if (ret != null) { + // ret.freeze(); + // } - name = name + "#" + x; - this.entryNames.add(name); + return ret; + } - return name; - } + // Replacement for ForgeHooks.readPoolName: since we prepared the json object, the name is already present. + public static String readPoolName(JsonObject lootPoolObj) { + // TODO: throw a better exception? + return JsonHelper.getString(lootPoolObj, "name"); } } diff --git a/patchwork-loot/src/main/java/net/patchworkmc/mixin/loot/MixinLootManager.java b/patchwork-loot/src/main/java/net/patchworkmc/mixin/loot/MixinLootManager.java index 4ae6cb97..e2a7e395 100644 --- a/patchwork-loot/src/main/java/net/patchworkmc/mixin/loot/MixinLootManager.java +++ b/patchwork-loot/src/main/java/net/patchworkmc/mixin/loot/MixinLootManager.java @@ -19,8 +19,8 @@ package net.patchworkmc.mixin.loot; +import java.io.IOException; import java.util.Map; -import java.util.function.BiConsumer; import com.google.common.collect.ImmutableMap; import com.google.gson.Gson; @@ -28,21 +28,21 @@ import org.apache.logging.log4j.Logger; import org.spongepowered.asm.mixin.Final; import org.spongepowered.asm.mixin.Mixin; -import org.spongepowered.asm.mixin.Overwrite; import org.spongepowered.asm.mixin.Shadow; import org.spongepowered.asm.mixin.injection.At; import org.spongepowered.asm.mixin.injection.Inject; -import org.spongepowered.asm.mixin.injection.Redirect; +import org.spongepowered.asm.mixin.injection.ModifyVariable; import org.spongepowered.asm.mixin.injection.callback.CallbackInfo; -import org.spongepowered.asm.mixin.injection.callback.LocalCapture; import net.minecraft.loot.LootManager; import net.minecraft.loot.LootTable; import net.minecraft.resource.Resource; import net.minecraft.resource.ResourceManager; import net.minecraft.util.Identifier; +import net.minecraft.util.JsonHelper; import net.minecraft.util.profiler.Profiler; +import net.patchworkmc.impl.event.loot.LootEvents; import net.patchworkmc.impl.loot.LootHooks; @Mixin(LootManager.class) @@ -55,27 +55,34 @@ public abstract class MixinLootManager extends MixinJsonDataLoader { @Final private static Logger LOGGER; - @Redirect(method = "apply", at = @At(value = "INVOKE", target = "java/util/Map.forEach (Ljava/util/function/BiConsumer;)V", ordinal = 0)) - private void cancel_forEach(Map map, BiConsumer consumer) { - // ignore this call, we're gonna reintroduce it but with capturing locals - } - - @Inject(method = "apply", at = @At(value = "INVOKE", target = "java/util/Map.forEach (Ljava/util/function/BiConsumer;)V", ordinal = 0), locals = LocalCapture.CAPTURE_FAILHARD) - private void reintroduce_forEach(Map map, ResourceManager resourceManager, Profiler profiler, CallbackInfo info, ImmutableMap.Builder builder) { - map.forEach((id, jsonObject) -> { + // NOTE: this could also be a Redirect of forEach that just wraps the existing lambda, instead of an additional forEach + @Inject(method = "apply", at = @At(value = "INVOKE", target = "java/util/Map.forEach (Ljava/util/function/BiConsumer;)V", ordinal = 0)) + private void prepareJson(Map map, ResourceManager resourceManager, Profiler profiler, CallbackInfo info) { + map.forEach((id, lootTableObj) -> { try { - LootManager lootManager = (LootManager) (Object) this; Resource res = resourceManager.getResource(this.getPreparedPath(id)); - LootTable lootTable = LootHooks.loadLootTable(GSON, id, jsonObject, res == null || !res.getResourcePackName().equals("Default"), lootManager); - builder.put(id, lootTable); - } catch (Exception ex) { + boolean custom = res == null || !res.getResourcePackName().equals("Default"); + LootManager lootManager = (LootManager) (Object) this; + LootHooks.prepareLootTable(id, lootTableObj, custom, lootManager); + } catch (IOException ex) { LOGGER.error("Couldn't parse loot table {}", id, ex); } }); } - @Overwrite - private static void method_20711(ImmutableMap.Builder builder, Identifier id, JsonObject obj) { - // We are effectively overwriting this lambda with our own, so let's make that explicit by actually overwriting it. + @ModifyVariable(method = "method_20711", at = @At(value = "INVOKE_ASSIGN", target = "com/google/gson/Gson.fromJson (Lcom/google/gson/JsonElement;Ljava/lang/Class;)Ljava/lang/Object;")) + private static LootTable modify_lootTable(LootTable table, ImmutableMap.Builder builder, Identifier id, JsonObject obj) { + // TODO: this is the only reason this implementation doesn't work. again. ugh. + // is there a hacky way we can pass an arbitrary java object as a JsonElement? + LootManager lootManager = (LootManager) (Object) this; + + if (!JsonHelper.getBoolean(obj, "custom", false)) { + table = LootEvents.loadLootTable(id, table, lootManager); + } + + // if (ret != null) { + // ret.freeze(); + // } + return table; } } From a9d9dff6f004dce556e1baedfedfb6e6554493f9 Mon Sep 17 00:00:00 2001 From: Kitlith Date: Tue, 21 Jul 2020 19:07:33 -0700 Subject: [PATCH 2/2] It's totally feasible now. :) --- .../net/patchworkmc/impl/loot/LootHooks.java | 5 +-- .../impl/loot/WrappedImmutableMapBuilder.java | 30 ++++++++++++++++ .../mixin/loot/MixinLootManager.java | 36 +++++++++++-------- 3 files changed, 52 insertions(+), 19 deletions(-) create mode 100644 patchwork-loot/src/main/java/net/patchworkmc/impl/loot/WrappedImmutableMapBuilder.java diff --git a/patchwork-loot/src/main/java/net/patchworkmc/impl/loot/LootHooks.java b/patchwork-loot/src/main/java/net/patchworkmc/impl/loot/LootHooks.java index f268b83e..7a7cf39f 100644 --- a/patchwork-loot/src/main/java/net/patchworkmc/impl/loot/LootHooks.java +++ b/patchwork-loot/src/main/java/net/patchworkmc/impl/loot/LootHooks.java @@ -35,10 +35,7 @@ public class LootHooks { public static void prepareLootTable(Identifier id, JsonObject lootTableObj, boolean custom, LootManager lootTableManager) { - // passing 'custom' value into lambda mixin via json after json parsing - lootTableObj.addProperty("custom", custom); - - // passing pool names into loot pool deserialization mixin since we have all the information we need now + // passing pool names into loot pool deserialization mixin since we have all the information we need right now, but not later if (lootTableObj.has("pools")) { MutableInt poolCount = new MutableInt(0); JsonArray pools = lootTableObj.getAsJsonArray("pools"); diff --git a/patchwork-loot/src/main/java/net/patchworkmc/impl/loot/WrappedImmutableMapBuilder.java b/patchwork-loot/src/main/java/net/patchworkmc/impl/loot/WrappedImmutableMapBuilder.java new file mode 100644 index 00000000..7e01fff5 --- /dev/null +++ b/patchwork-loot/src/main/java/net/patchworkmc/impl/loot/WrappedImmutableMapBuilder.java @@ -0,0 +1,30 @@ +package net.patchworkmc.impl.loot; + +import java.util.function.BiFunction; + +import javax.annotation.ParametersAreNonnullByDefault; + +import com.google.common.collect.ImmutableMap; + +import net.patchworkmc.mixin.loot.MixinLootManager; + +/** + * A builder for creating immutable map instances that applies some sort of transformation + * before putting items into the map. See {@link ImmutableMap.Builder} for normal usage. + * + *

Yes, this is a hack, see usage in {@link MixinLootManager}

+ */ +@ParametersAreNonnullByDefault +public class WrappedImmutableMapBuilder extends ImmutableMap.Builder { + private final BiFunction wrap; + + public WrappedImmutableMapBuilder(BiFunction wrap) { + super(); + this.wrap = wrap; + } + + @Override + public ImmutableMap.Builder put(K key, V value) { + return super.put(key, wrap.apply(key, value)); + } +} diff --git a/patchwork-loot/src/main/java/net/patchworkmc/mixin/loot/MixinLootManager.java b/patchwork-loot/src/main/java/net/patchworkmc/mixin/loot/MixinLootManager.java index e2a7e395..3845d02b 100644 --- a/patchwork-loot/src/main/java/net/patchworkmc/mixin/loot/MixinLootManager.java +++ b/patchwork-loot/src/main/java/net/patchworkmc/mixin/loot/MixinLootManager.java @@ -29,9 +29,10 @@ import org.spongepowered.asm.mixin.Final; import org.spongepowered.asm.mixin.Mixin; import org.spongepowered.asm.mixin.Shadow; +import org.spongepowered.asm.mixin.Unique; import org.spongepowered.asm.mixin.injection.At; import org.spongepowered.asm.mixin.injection.Inject; -import org.spongepowered.asm.mixin.injection.ModifyVariable; +import org.spongepowered.asm.mixin.injection.Redirect; import org.spongepowered.asm.mixin.injection.callback.CallbackInfo; import net.minecraft.loot.LootManager; @@ -39,11 +40,11 @@ import net.minecraft.resource.Resource; import net.minecraft.resource.ResourceManager; import net.minecraft.util.Identifier; -import net.minecraft.util.JsonHelper; import net.minecraft.util.profiler.Profiler; import net.patchworkmc.impl.event.loot.LootEvents; import net.patchworkmc.impl.loot.LootHooks; +import net.patchworkmc.impl.loot.WrappedImmutableMapBuilder; @Mixin(LootManager.class) public abstract class MixinLootManager extends MixinJsonDataLoader { @@ -60,8 +61,7 @@ public abstract class MixinLootManager extends MixinJsonDataLoader { private void prepareJson(Map map, ResourceManager resourceManager, Profiler profiler, CallbackInfo info) { map.forEach((id, lootTableObj) -> { try { - Resource res = resourceManager.getResource(this.getPreparedPath(id)); - boolean custom = res == null || !res.getResourcePackName().equals("Default"); + boolean custom = isLootTableCustom(resourceManager, id); LootManager lootManager = (LootManager) (Object) this; LootHooks.prepareLootTable(id, lootTableObj, custom, lootManager); } catch (IOException ex) { @@ -70,19 +70,25 @@ private void prepareJson(Map map, ResourceManager resour }); } - @ModifyVariable(method = "method_20711", at = @At(value = "INVOKE_ASSIGN", target = "com/google/gson/Gson.fromJson (Lcom/google/gson/JsonElement;Ljava/lang/Class;)Ljava/lang/Object;")) - private static LootTable modify_lootTable(LootTable table, ImmutableMap.Builder builder, Identifier id, JsonObject obj) { - // TODO: this is the only reason this implementation doesn't work. again. ugh. - // is there a hacky way we can pass an arbitrary java object as a JsonElement? + @Redirect(method = "apply", at = @At(value = "INVOKE", target = "com/google/common/collect/ImmutableMap.builder ()Lcom/google/common/collect/ImmutableMap$Builder;")) + private ImmutableMap.Builder wrapBuilder(Map map, ResourceManager resourceManager, Profiler profiler) { LootManager lootManager = (LootManager) (Object) this; + return new WrappedImmutableMapBuilder((id, table) -> { + // TODO: handle exception? + if (!isLootTableCustom(resourceManager, id)) { + table = LootEvents.loadLootTable(id, table, lootManager); + } - if (!JsonHelper.getBoolean(obj, "custom", false)) { - table = LootEvents.loadLootTable(id, table, lootManager); - } + // if (table != null) { + // table.freeze(); + // } + return table; + }); + } - // if (ret != null) { - // ret.freeze(); - // } - return table; + @Unique + private boolean isLootTableCustom(ResourceManager resourceManager, Identifier id) throws IOException { + Resource res = resourceManager.getResource(this.getPreparedPath(id)); + return res == null || !res.getResourcePackName().equals("Default"); } }