From 59e3b83d7496e05de1a32db97b68ecb7215bae05 Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Mon, 28 Apr 2025 10:59:49 -0400 Subject: [PATCH 1/2] Default to showing the whole model registry to mods This should fix many silent incompatibilities that existed with the original opt-in approach. To my knowledge, there are no remaining popular mods on 1.20 that forcefully load all models in this event (by iterating over entrySet and calling getValue unconditionally) so doing this should be safe. Additional logging is also added to provide quick insight into what mod(s) have the slowest handling of this event. --- .../dynresources/ModelBakeEventHelper.java | 197 +++++++++++------- .../ForgeHooksClientMixin.java | 20 +- 2 files changed, 135 insertions(+), 82 deletions(-) diff --git a/forge/src/main/java/org/embeddedt/modernfix/forge/dynresources/ModelBakeEventHelper.java b/forge/src/main/java/org/embeddedt/modernfix/forge/dynresources/ModelBakeEventHelper.java index c1a36d27..9a15f31d 100644 --- a/forge/src/main/java/org/embeddedt/modernfix/forge/dynresources/ModelBakeEventHelper.java +++ b/forge/src/main/java/org/embeddedt/modernfix/forge/dynresources/ModelBakeEventHelper.java @@ -1,8 +1,7 @@ package org.embeddedt.modernfix.forge.dynresources; import com.google.common.collect.ForwardingMap; -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterators; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Sets; import com.google.common.graph.GraphBuilder; import com.google.common.graph.MutableGraph; @@ -11,12 +10,12 @@ import net.minecraft.client.renderer.block.BlockModelShaper; import net.minecraft.client.resources.model.BakedModel; import net.minecraft.client.resources.model.ModelBakery; import net.minecraft.client.resources.model.ModelResourceLocation; +import net.minecraft.core.registries.BuiltInRegistries; import net.minecraft.resources.ResourceLocation; import net.minecraft.world.level.block.state.BlockState; import net.minecraftforge.fml.ModContainer; import net.minecraftforge.fml.ModList; import net.minecraftforge.forgespi.language.IModInfo; -import net.minecraftforge.registries.ForgeRegistries; import org.embeddedt.modernfix.ModernFix; import org.embeddedt.modernfix.util.ForwardingInclDefaultsMap; import org.jetbrains.annotations.Nullable; @@ -24,6 +23,7 @@ import org.jetbrains.annotations.Nullable; import java.util.AbstractSet; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.HashSet; import java.util.Iterator; import java.util.List; @@ -38,18 +38,22 @@ import java.util.function.BiFunction; * of the model registry that emulates vanilla keySet behavior. */ public class ModelBakeEventHelper { - // TODO: make into config option - private static final Set INCOMPATIBLE_MODS = ImmutableSet.of( - "industrialforegoing", - "mekanism", - "vampirism", - "elevatorid", - "cfm", - "refinedstorage", - "embers", - "buildcraftsilicon", - "buildcrafttransport", - "buildcraftfactory"); + private enum UniverseVisibility { + /** + * Mod cannot see any view of the universe of model locations. + */ + NONE, + /** + * Mod can see its own model locations and those of dependencies/dependents. + */ + SELF_AND_DEPS, + /** + * Mod can see every model location. + */ + EVERYTHING + } + private static final Map MOD_VISIBILITY_CONFIGURATION = ImmutableMap.builder() + .build(); private final Map modelRegistry; private final Set topLevelModelLocations; private final MutableGraph dependencyGraph; @@ -57,31 +61,36 @@ public class ModelBakeEventHelper { this.modelRegistry = modelRegistry; this.topLevelModelLocations = new ObjectLinkedOpenHashSet<>(); // Skip going through ModelLocationCache because most of the accesses will be misses - ForgeRegistries.BLOCKS.getEntries().forEach(entry -> { + BuiltInRegistries.BLOCK.entrySet().forEach(entry -> { var location = entry.getKey().location(); for(BlockState state : entry.getValue().getStateDefinition().getPossibleStates()) { topLevelModelLocations.add(BlockModelShaper.stateToModelLocation(location, state)); } }); - ForgeRegistries.ITEMS.getKeys().forEach(key -> topLevelModelLocations.add(new ModelResourceLocation(key, "inventory"))); + BuiltInRegistries.ITEM.keySet().forEach(key -> topLevelModelLocations.add(new ModelResourceLocation(key, "inventory"))); this.topLevelModelLocations.addAll(modelRegistry.keySet()); - this.dependencyGraph = GraphBuilder.undirected().build(); + this.dependencyGraph = buildDependencyGraph(); + } + + private static MutableGraph buildDependencyGraph() { + MutableGraph dependencyGraph = GraphBuilder.undirected().build(); ModList.get().forEachModContainer((id, mc) -> { - this.dependencyGraph.addNode(id); + dependencyGraph.addNode(id); for(IModInfo.ModVersion version : mc.getModInfo().getDependencies()) { - this.dependencyGraph.addNode(version.getModId()); + dependencyGraph.addNode(version.getModId()); } }); - for(String id : this.dependencyGraph.nodes()) { + for(String id : dependencyGraph.nodes()) { Optional mContainer = ModList.get().getModContainerById(id); if(mContainer.isPresent()) { for(IModInfo.ModVersion version : mContainer.get().getModInfo().getDependencies()) { // avoid self-loops if(!Objects.equals(id, version.getModId())) - this.dependencyGraph.putEdge(id, version.getModId()); + dependencyGraph.putEdge(id, version.getModId()); } } } + return dependencyGraph; } private static final Set WARNED_MOD_IDS = new HashSet<>(); @@ -132,73 +141,94 @@ public class ModelBakeEventHelper { } public Map wrapRegistry(String modId) { + var config = MOD_VISIBILITY_CONFIGURATION.getOrDefault(modId, UniverseVisibility.EVERYTHING); + if (config == UniverseVisibility.NONE) { + return createWarningRegistry(modId); + } final Set modIdsToInclude = new HashSet<>(); modIdsToInclude.add(modId); try { modIdsToInclude.addAll(this.dependencyGraph.adjacentNodes(modId)); } catch(IllegalArgumentException ignored) { /* sanity check */ } modIdsToInclude.remove("minecraft"); - if(modIdsToInclude.stream().noneMatch(INCOMPATIBLE_MODS::contains)) - return createWarningRegistry(modId); - Set ourModelLocations = Sets.filter(this.topLevelModelLocations, loc -> modIdsToInclude.contains(loc.getNamespace())); + Set ourModelLocations; + if (config == UniverseVisibility.SELF_AND_DEPS) { + ourModelLocations = Sets.filter(this.topLevelModelLocations, loc -> modIdsToInclude.contains(loc.getNamespace())); + } else { + ourModelLocations = this.topLevelModelLocations; + } BakedModel missingModel = modelRegistry.get(ModelBakery.MISSING_MODEL_LOCATION); - return new ForwardingMap() { - @Override - protected Map delegate() { - return modelRegistry; - } + return new EmulatedModelRegistry(modId, modIdsToInclude, missingModel, ourModelLocations); + } - @Override - public BakedModel get(@Nullable Object key) { - BakedModel model = super.get(key); - if(model == null && key != null && modIdsToInclude.contains(((ResourceLocation)key).getNamespace())) { - ModernFix.LOGGER.warn("Model {} is missing, but was requested in model bake event. Returning missing model", key); - return missingModel; + public class EmulatedModelRegistry extends ForwardingMap { + private final Set modIdsToInclude; + private final BakedModel missingModel; + private final Set ourModelLocations; + private final String modId; + + private EmulatedModelRegistry(String modId, Set modIdsToInclude, BakedModel missingModel, Set ourModelLocations) { + this.modId = modId; + this.modIdsToInclude = modIdsToInclude; + this.missingModel = missingModel; + this.ourModelLocations = ourModelLocations; + } + + @Override + protected Map delegate() { + return modelRegistry; + } + + @Override + public BakedModel get(@Nullable Object key) { + BakedModel model = super.get(key); + if(model == null && key != null && modIdsToInclude.contains(((ResourceLocation)key).getNamespace())) { + ModernFix.LOGGER.warn("Model {} is missing, but was requested in model bake event. Returning missing model", key); + return missingModel; + } + return model; + } + + @Override + public Set keySet() { + return Collections.unmodifiableSet(ourModelLocations); + } + + @Override + public boolean containsKey(@Nullable Object key) { + return ourModelLocations.contains(key) || super.containsKey(key); + } + + @Override + public Set> entrySet() { + return new DynamicModelEntrySet(this, ourModelLocations); + } + + @Override + public void replaceAll(BiFunction function) { + ModernFix.LOGGER.warn("Mod '{}' is calling replaceAll on the model registry. Some hacks will be used to keep this fast, but they may not be 100% compatible.", modId); + List locations = new ArrayList<>(ourModelLocations); + for(ResourceLocation location : locations) { + /* + * Fetching every model is insanely slow. So we call the function with a null object first, since it + * probably isn't expecting that. If we get an exception thrown, or it returns nonnull, then we know + * it actually cares about the given model. + */ + boolean needsReplacement; + try { + needsReplacement = function.apply(location, null) != null; + } catch(Throwable e) { + needsReplacement = true; } - return model; - } - - @Override - public Set keySet() { - return ourModelLocations; - } - - @Override - public boolean containsKey(@Nullable Object key) { - return ourModelLocations.contains(key) || super.containsKey(key); - } - - @Override - public Set> entrySet() { - return new DynamicModelEntrySet(this, ourModelLocations); - } - - @Override - public void replaceAll(BiFunction function) { - ModernFix.LOGGER.warn("Mod '{}' is calling replaceAll on the model registry. Some hacks will be used to keep this fast, but they may not be 100% compatible.", modId); - List locations = new ArrayList<>(keySet()); - for(ResourceLocation location : locations) { - /* - * Fetching every model is insanely slow. So we call the function with a null object first, since it - * probably isn't expecting that. If we get an exception thrown, or it returns nonnull, then we know - * it actually cares about the given model. - */ - boolean needsReplacement; - try { - needsReplacement = function.apply(location, null) != null; - } catch(Throwable e) { - needsReplacement = true; - } - if(needsReplacement) { - BakedModel existing = get(location); - BakedModel replacement = function.apply(location, existing); - if(replacement != existing) { - put(location, replacement); - } + if(needsReplacement) { + BakedModel existing = get(location); + BakedModel replacement = function.apply(location, existing); + if(replacement != existing) { + put(location, replacement); } } } - }; + } } private static class DynamicModelEntrySet extends AbstractSet> { @@ -212,7 +242,18 @@ public class ModelBakeEventHelper { @Override public Iterator> iterator() { - return Iterators.transform(Iterators.unmodifiableIterator(this.modelLocations.iterator()), DynamicModelEntry::new); + var iter = this.modelLocations.iterator(); + return new Iterator<>() { + @Override + public boolean hasNext() { + return iter.hasNext(); + } + + @Override + public Map.Entry next() { + return new DynamicModelEntry(iter.next()); + } + }; } @Override diff --git a/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/perf/dynamic_resources/ForgeHooksClientMixin.java b/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/perf/dynamic_resources/ForgeHooksClientMixin.java index 076ce3c6..198420e7 100644 --- a/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/perf/dynamic_resources/ForgeHooksClientMixin.java +++ b/forge/src/main/java/org/embeddedt/modernfix/forge/mixin/perf/dynamic_resources/ForgeHooksClientMixin.java @@ -1,6 +1,7 @@ package org.embeddedt.modernfix.forge.mixin.perf.dynamic_resources; import com.google.common.base.Stopwatch; +import it.unimi.dsi.fastutil.objects.Object2ObjectOpenHashMap; import net.minecraft.client.resources.model.BakedModel; import net.minecraft.resources.ResourceLocation; import net.minecraftforge.client.ForgeHooksClient; @@ -17,6 +18,8 @@ import org.spongepowered.asm.mixin.injection.At; import org.spongepowered.asm.mixin.injection.Redirect; import java.lang.reflect.Method; +import java.time.Duration; +import java.util.Comparator; import java.util.Map; import java.util.concurrent.TimeUnit; @@ -32,19 +35,28 @@ public class ForgeHooksClientMixin { ModelEvent.ModifyBakingResult bakeEvent = ((ModelEvent.ModifyBakingResult)event); ModelBakeEventHelper helper = new ModelBakeEventHelper(bakeEvent.getModels()); Method acceptEv = ObfuscationReflectionHelper.findMethod(ModContainer.class, "acceptEvent", Event.class); + Stopwatch globalTimer = Stopwatch.createStarted(); + Map times = new Object2ObjectOpenHashMap<>(); ModList.get().forEachModContainer((id, mc) -> { Map newRegistry = helper.wrapRegistry(id); ModelEvent.ModifyBakingResult postedEvent = new ModelEvent.ModifyBakingResult(newRegistry, bakeEvent.getModelBakery()); - Stopwatch timer = Stopwatch.createStarted(); + Stopwatch timer = times.computeIfAbsent(id, $ -> Stopwatch.createStarted()); try { acceptEv.invoke(mc, postedEvent); } catch(ReflectiveOperationException e) { e.printStackTrace(); } timer.stop(); - if(timer.elapsed(TimeUnit.SECONDS) >= 1) { - ModernFix.LOGGER.warn("Mod '{}' took {} in the model bake event", id, timer); - } }); + globalTimer.stop(); + if (globalTimer.elapsed(TimeUnit.SECONDS) >= 1) { + ModernFix.LOGGER.warn("Posting dynamic ModelEvent.ModifyBakingResult to mods took {}, breakdown below:", globalTimer); + times.entrySet().stream() + .sorted(Comparator., Duration>comparing(e -> e.getValue().elapsed()).reversed()) + .filter(e -> e.getValue().elapsed(TimeUnit.MILLISECONDS) > 50) + .forEach(entry -> { + ModernFix.LOGGER.warn(" {}: {}", entry.getKey(), entry.getValue().toString()); + }); + } } } From 9be61340736c00b652f225aaf45ba97a9a73b3d5 Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Mon, 28 Apr 2025 18:55:23 -0400 Subject: [PATCH 2/2] Remove obsolete resource pack code --- .../resources/CachedResourcePath.java | 108 ------------------ .../resources/NewResourcePackAdapter.java | 17 --- 2 files changed, 125 deletions(-) delete mode 100644 common/src/main/java/org/embeddedt/modernfix/resources/CachedResourcePath.java delete mode 100644 common/src/main/java/org/embeddedt/modernfix/resources/NewResourcePackAdapter.java diff --git a/common/src/main/java/org/embeddedt/modernfix/resources/CachedResourcePath.java b/common/src/main/java/org/embeddedt/modernfix/resources/CachedResourcePath.java deleted file mode 100644 index f82f0521..00000000 --- a/common/src/main/java/org/embeddedt/modernfix/resources/CachedResourcePath.java +++ /dev/null @@ -1,108 +0,0 @@ -package org.embeddedt.modernfix.resources; - -import com.google.common.base.Splitter; -import com.google.common.collect.Interner; -import com.google.common.collect.Interners; -import org.embeddedt.modernfix.util.FileUtil; - -import java.nio.file.Path; -import java.util.Arrays; -import java.util.Collection; - -public class CachedResourcePath { - private final String[] pathComponents; - - public static final Interner PATH_COMPONENT_INTERNER = Interners.newStrongInterner(); - private static final Splitter SLASH_SPLITTER = Splitter.on('/'); - public static final String[] NO_PREFIX = new String[0]; - - public CachedResourcePath(String[] prefix, Path path) { - this(prefix, path, path.getNameCount(), true); - } - - public CachedResourcePath(String s) { - // normalize so we can guarantee there are no empty sections - this(NO_PREFIX, SLASH_SPLITTER.splitToList(FileUtil.normalize(s)), false); - } - - public CachedResourcePath(String[] prefixElements, Collection collection, boolean intern) { - this(prefixElements, collection, collection.size(), intern); - } - - public CachedResourcePath(String[] prefixElements, Iterable path, int count, boolean intern) { - String[] components = new String[prefixElements.length + count]; - int i = 0; - while(i < prefixElements.length) { - components[i] = intern ? PATH_COMPONENT_INTERNER.intern(prefixElements[i]) : prefixElements[i]; - i++; - } - for(Object component : path) { - String s = component.toString(); - if(s.length() == 0) - continue; - components[i] = intern ? PATH_COMPONENT_INTERNER.intern(s) : s; - i++; - } - pathComponents = components; - } - - public CachedResourcePath(String[] prefixElements, CachedResourcePath other) { - String[] components = new String[prefixElements.length + other.pathComponents.length]; - int i = 0; - while(i < prefixElements.length) { - components[i] = PATH_COMPONENT_INTERNER.intern(prefixElements[i]); - i++; - } - System.arraycopy(other.pathComponents, 0, components, i, other.pathComponents.length); - pathComponents = components; - } - - /** - * DOES NOT INTERN! - */ - public CachedResourcePath(String[] pathComponents) { - for(String s : pathComponents) { - if(s.length() == 0) { - // reconstruct the whole array skipping blanks. inefficient, but should not be the common case - pathComponents = Arrays.stream(pathComponents).filter(comp -> comp.length() > 0).toArray(String[]::new); - break; - } - } - this.pathComponents = pathComponents; - } - - @Override - public int hashCode() { - return Arrays.hashCode(pathComponents); - } - - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; - CachedResourcePath that = (CachedResourcePath) o; - return Arrays.equals(pathComponents, that.pathComponents); - } - - public String getFileName() { - return pathComponents[pathComponents.length - 1]; - } - - public int getNameCount() { - return pathComponents.length; - } - - public String getNameAt(int i) { - return pathComponents[i]; - } - - public String getFullPath(int startIndex) { - StringBuilder sb = new StringBuilder(); - for(int i = startIndex; i < pathComponents.length; i++) { - sb.append(pathComponents[i]); - if(i != (pathComponents.length - 1)) - sb.append('/'); - } - return sb.toString(); - } -} diff --git a/common/src/main/java/org/embeddedt/modernfix/resources/NewResourcePackAdapter.java b/common/src/main/java/org/embeddedt/modernfix/resources/NewResourcePackAdapter.java deleted file mode 100644 index fb1d16a6..00000000 --- a/common/src/main/java/org/embeddedt/modernfix/resources/NewResourcePackAdapter.java +++ /dev/null @@ -1,17 +0,0 @@ -package org.embeddedt.modernfix.resources; - -import net.minecraft.resources.ResourceLocation; -import net.minecraft.server.packs.PackResources; -import net.minecraft.server.packs.resources.IoSupplier; - -import java.io.InputStream; -import java.util.Collection; -import java.util.function.Function; - -public class NewResourcePackAdapter { - public static void sendToOutput(Function> streamCreator, PackResources.ResourceOutput output, Collection locations) { - for(ResourceLocation rl : locations) { - output.accept(rl, streamCreator.apply(rl)); - } - } -}