Fix client tinted biomes

2025-10-12 14:49:32 -05:00
parent 22a4c6bc79
commit 9cc826f8a9
5 changed files with 145 additions and 24 deletions
@@ -2,15 +2,25 @@ package com.seibel.distanthorizons.common.wrappers.block;

 import com.seibel.distanthorizons.core.config.Config;
 import com.seibel.distanthorizons.core.dataObjects.fullData.sources.FullDataSourceV2;
+import com.seibel.distanthorizons.core.logging.DhLoggerBuilder;
 import com.seibel.distanthorizons.core.pos.DhSectionPos;
 import com.seibel.distanthorizons.core.pos.blockPos.DhBlockPosMutable;
 import com.seibel.distanthorizons.core.util.ColorUtil;
 import com.seibel.distanthorizons.core.util.FullDataPointUtil;
+
+import com.seibel.distanthorizons.core.wrapperInterfaces.world.IClientLevelWrapper;
+import net.minecraft.client.Minecraft;
+import net.minecraft.client.multiplayer.ClientLevel;
 import net.minecraft.core.BlockPos;
 import net.minecraft.world.level.BlockAndTintGetter;
 import net.minecraft.world.level.ColorResolver;
 import net.minecraft.world.level.biome.Biome;

+import java.io.IOException;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import org.apache.logging.log4j.Logger;
+
 #if MC_VER >= MC_1_18_2
 import net.minecraft.core.Holder;
 #endif
@@ -18,10 +28,20 @@ import net.minecraft.core.Holder;

 public abstract class AbstractDhTintGetter implements BlockAndTintGetter
 {
+	private static final Logger LOGGER = DhLoggerBuilder.getLogger();
+	
+	
 	protected final BiomeWrapper biomeWrapper;
 	
 	protected final int smoothingRadiusInBlocks;
 	protected final FullDataSourceV2 fullDataSource;
+	protected final IClientLevelWrapper clientLevelWrapper;
+	
+	#if MC_VER < MC_1_18_2
+	public static final ConcurrentMap<String, Biome> BIOME_BY_RESOURCE_STRING = new ConcurrentHashMap<>();
+	#else
+	public static final ConcurrentMap<String, Holder<Biome>> BIOME_BY_RESOURCE_STRING = new ConcurrentHashMap<>();
+    #endif
 	
 	
 	
@@ -29,10 +49,11 @@ public abstract class AbstractDhTintGetter implements BlockAndTintGetter
 	// constructor //
 	//=============//
 	
-	public AbstractDhTintGetter(BiomeWrapper biomeWrapper, FullDataSourceV2 fullDataSource)
+	public AbstractDhTintGetter(BiomeWrapper biomeWrapper, FullDataSourceV2 fullDataSource, IClientLevelWrapper clientLevelWrapper)
 	{
 		this.biomeWrapper = biomeWrapper;
 		this.fullDataSource = fullDataSource;
+		this.clientLevelWrapper = clientLevelWrapper;
 		this.smoothingRadiusInBlocks = Config.Client.Advanced.Graphics.Quality.lodBiomeBlending.get();
 	}
 	
@@ -57,7 +78,7 @@ public abstract class AbstractDhTintGetter implements BlockAndTintGetter
 		if (this.smoothingRadiusInBlocks == 0
 			|| dataSourceLodWidthInBlocks > this.smoothingRadiusInBlocks)
 		{
-			return colorResolver.getColor(this.unwrapBiome(this.biomeWrapper.biome), blockPos.getX(), blockPos.getZ());
+			return colorResolver.getColor(unwrapClientBiome(this.biomeWrapper.biome, this.clientLevelWrapper), blockPos.getX(), blockPos.getZ());
 		}
 		
 		
@@ -93,7 +114,7 @@ public abstract class AbstractDhTintGetter implements BlockAndTintGetter
 				// get the color for this nearby position
 				int id = FullDataPointUtil.getId(dataPoint);
 				BiomeWrapper biomeWrapper = (BiomeWrapper) this.fullDataSource.mapping.getBiomeWrapper(id);
-				int color = colorResolver.getColor(this.unwrapBiome(biomeWrapper.biome), mutableBlockPos.getX(), mutableBlockPos.getZ());
+				int color = colorResolver.getColor(unwrapClientBiome(biomeWrapper.biome, this.clientLevelWrapper), mutableBlockPos.getX(), mutableBlockPos.getZ());
 				
 				
 				// rolling average
@@ -110,7 +131,7 @@ public abstract class AbstractDhTintGetter implements BlockAndTintGetter
 		// just use the default center's color
 		if (dataPointCount == 0)
 		{
-			return colorResolver.getColor(this.unwrapBiome(this.biomeWrapper.biome), blockPos.getX(), blockPos.getZ());
+			return colorResolver.getColor(unwrapClientBiome(this.biomeWrapper.biome, this.clientLevelWrapper), blockPos.getX(), blockPos.getZ());
 		}
 		
 		int colorInt = ColorUtil.argbToInt(
@@ -121,7 +142,23 @@ public abstract class AbstractDhTintGetter implements BlockAndTintGetter
 		return colorInt;
 	}
 	
-	protected Biome unwrapBiome(#if MC_VER >= MC_1_18_2 Holder<Biome> #else Biome #endif biome)
+	protected static Biome unwrapClientBiome(#if MC_VER >= MC_1_18_2 Holder<Biome> #else Biome #endif biome, IClientLevelWrapper clientLevelWrapper)
+	{
+		BiomeWrapper biomeWrapper = (BiomeWrapper)BiomeWrapper.getBiomeWrapper(biome, clientLevelWrapper);
+		
+		String biomeString = biomeWrapper.getSerialString();
+		if (biomeString == null
+				|| biomeString.isEmpty()
+				|| biomeString.equals(BiomeWrapper.EMPTY_BIOME_STRING))
+		{
+			// default to "plains" for empty/invalid biomes
+			biomeString = "minecraft:plains";
+		}
+		
+		return unwrapBiome(getClientBiome(biomeString));
+	}
+	
+	protected static Biome unwrapBiome(#if MC_VER >= MC_1_18_2 Holder<Biome> #else Biome #endif biome)
 	{
 		#if MC_VER >= MC_1_18_2
 		return biome.value();
@@ -130,6 +167,88 @@ public abstract class AbstractDhTintGetter implements BlockAndTintGetter
 		#endif
 	}
 	
+	/**
+	 * <p>Previously, this class might have immediately unwrapped the Holder like this:</p>
+	 * <pre>{@code
+	 * // Inside constructor (OLD WAY - PROBLEMATIC):
+	 * Holder<Biome> biomeHolder = getTheHolderFromSomewhere();
+	 * this.biome = biomeHolder.value(); // <-- PROBLEM HERE
+	 * }</pre>
+	 *
+	 * <p>This approach is problematic because the {@link net.minecraft.core.Holder} system,
+	 * particularly {@code Holder.Reference}, is designed for <strong>late binding</strong>. Here's why storing
+	 * the Holder itself is now necessary:</p>
+	 * <ol>
+	 *   <li>A {@code Holder.Reference<Biome>} might be created initially just with a
+	 *       {@link net.minecraft.resources.ResourceKey} (like {@code minecraft:plains}), but its actual
+	 *       {@link net.minecraft.core.Holder#value() value()} (the {@code Biome} object itself) might be {@code null}
+	 *       at construction time.</li>
+	 *   <li>Later, during game loading, registry population, or potentially due to modifications by other mods
+	 *       (e.g., Polytone), the system calls internal binding methods (like {@code bindValue(Biome)})
+	 *       on the {@code Holder} instance. This sets or <strong>updates</strong> the internal reference to the
+	 *       actual {@code Biome} object.</li>
+	 *   <li>Crucially, the binding process might assign a completely <strong>new</strong> {@code Biome} object
+	 *       instance to the {@code Holder} reference, replacing any previous one.</li>
+	 * </ol>
+	 *
+	 * <p>If we unwrapped the {@code Holder} using {@code .value()} within the constructor (the old way),
+	 * our class's internal {@code biome} field would permanently store a reference to whatever {@code Biome}
+	 * object the {@code Holder} pointed to *at that exact moment*. It would have no link back to the
+	 * {@code Holder} and would be unaware if the {@code Holder} was later updated to point to a different
+	 * (or the initially missing) {@code Biome} object. This would lead to using stale or even {@code null} data.</p>
+	 *
+	 * <p>By storing the {@code Holder<Biome>} itself, this class can call {@link net.minecraft.core.Holder#value()}
+	 * whenever the biome information is needed, ensuring it always retrieves the most current {@code Biome}
+	 * instance associated with the holder at that time.</p>
+	 */
+	private static #if MC_VER < MC_1_18_2 Biome #else Holder<Biome> #endif getClientBiome(String biomeResourceString)
+	{
+		// cache the client biomes so we don't have to re-parse the resource location every time
+		return BIOME_BY_RESOURCE_STRING.compute(biomeResourceString,
+				(resourceString, existingBiome) ->
+				{
+					if (existingBiome != null)
+					{
+						return existingBiome;
+					}
+					
+					ClientLevel clientLevel = Minecraft.getInstance().level;
+					if (clientLevel == null)
+					{
+						// shouldn't happen, but just in case
+						throw new IllegalStateException("Attempted to get client biome when no client level was loaded.");
+					}
+					
+					BiomeWrapper.BiomeDeserializeResult result;
+					try
+					{
+						result = BiomeWrapper.deserializeBiome(resourceString, clientLevel.registryAccess());
+					}
+					catch (Exception e)
+					{
+						LOGGER.warn("Unable to deserialize client biome ["+resourceString+"], using fallback...");
+						
+						try
+						{
+							result = BiomeWrapper.deserializeBiome(BiomeWrapper.PLAINS_RESOURCE_LOCATION_STRING, clientLevel.registryAccess());
+						}
+						catch (IOException ex)
+						{
+							// should never happen, if it does this log will explode, but just in case
+							LOGGER.error("Unable to deserialize fallback client biome ["+BiomeWrapper.PLAINS_RESOURCE_LOCATION_STRING+"], returning NULL.");
+							return null;
+						}
+					}
+					
+					if (result.success)
+					{
+						existingBiome = result.biome;
+					}
+					
+					return existingBiome;
+				});
+	}
+	
 	
 	
 }
@@ -103,6 +103,7 @@ public class BiomeWrapper implements IBiomeWrapper
 	// constructors //
 	//==============//
 	
+	// TODO why not just return BiomeWrapper?
 	static public IBiomeWrapper getBiomeWrapper(#if MC_VER < MC_1_18_2 Biome #else Holder<Biome> #endif biome, ILevelWrapper levelWrapper)
 	{
 		if (biome == null)
@@ -20,7 +20,6 @@
 package com.seibel.distanthorizons.common.wrappers.block;

 import com.seibel.distanthorizons.common.wrappers.McObjectConverter;
-import com.seibel.distanthorizons.core.config.Config;
 import com.seibel.distanthorizons.core.dataObjects.fullData.sources.FullDataSourceV2;
 import com.seibel.distanthorizons.core.logging.DhLoggerBuilder;
 import com.seibel.distanthorizons.core.pos.blockPos.DhBlockPos;
@@ -464,7 +463,7 @@ public class ClientBlockStateColorCache
 	// public getter //
 	//===============//
 	
-	public int getColor(BiomeWrapper biome, FullDataSourceV2 fullDataSource, DhBlockPos pos)
+	public int getColor(BiomeWrapper biomeWrapper, FullDataSourceV2 fullDataSource, DhBlockPos pos)
 	{
 		// only get the tint if the block needs to be tinted
 		if (!this.needPostTinting)
@@ -490,14 +489,14 @@ public class ClientBlockStateColorCache
 				{
 					tintColor = Minecraft.getInstance().getBlockColors()
 							.getColor(this.blockState, 
-									new TintWithoutLevelSmoothOverrider(biome, fullDataSource), // TODO can this object be cached?
+									new TintWithoutLevelOverrider(biomeWrapper, fullDataSource, this.clientLevelWrapper), // TODO can this object be cached?
 									McObjectConverter.Convert(pos), 
 									this.tintIndex);
 				}
 				catch (UnsupportedOperationException e)
 				{
 					// this exception generally occurs if the tint requires other blocks besides itself
-					LOGGER.debug("Unable to use ["+TintWithoutLevelSmoothOverrider.class.getSimpleName()+"] to get the block tint for block: [" + this.blockState + "] and biome: [" + biome + "] at pos: " + pos + ". Error: [" + e.getMessage() + "]. Attempting to use backup method...", e);
+					LOGGER.debug("Unable to use ["+ TintWithoutLevelOverrider.class.getSimpleName()+"] to get the block tint for block: [" + this.blockState + "] and biome: [" + biomeWrapper + "] at pos: " + pos + ". Error: [" + e.getMessage() + "]. Attempting to use backup method...", e);
 					BLOCK_STATES_THAT_NEED_LEVEL.add(this.blockState);
 				}
 			}
@@ -509,7 +508,7 @@ public class ClientBlockStateColorCache
 				// specifically oceans don't render correctly
 				tintColor = Minecraft.getInstance().getBlockColors()
 						.getColor(this.blockState, 
-								new TintGetterOverrideSmooth(this.level, biome, fullDataSource), // TODO can this object be cached?
+								new TintGetterOverride(this.level, biomeWrapper, fullDataSource, this.clientLevelWrapper), // TODO can this object be cached?
 								McObjectConverter.Convert(pos), 
 								this.tintIndex);
 			}
@@ -519,7 +518,7 @@ public class ClientBlockStateColorCache
 			// only display the error once per block/biome type to reduce log spam
 			if (!BROKEN_BLOCK_STATES.contains(this.blockState))
 			{
-				LOGGER.warn("Failed to get block color for block: [" + this.blockState + "] and biome: [" + biome + "] at pos: " + pos + ". Error: ["+e.getMessage() + "]. Note: future errors for this block/biome will be ignored.", e);
+				LOGGER.warn("Failed to get block color for block: [" + this.blockState + "] and biome: [" + biomeWrapper + "] at pos: " + pos + ". Error: ["+e.getMessage() + "]. Note: future errors for this block/biome will be ignored.", e);
 				BROKEN_BLOCK_STATES.add(this.blockState);
 			}
 		}
@@ -20,6 +20,7 @@
 package com.seibel.distanthorizons.common.wrappers.block;

 import com.seibel.distanthorizons.core.dataObjects.fullData.sources.FullDataSourceV2;
+import com.seibel.distanthorizons.core.wrapperInterfaces.world.IClientLevelWrapper;
 import net.minecraft.core.BlockPos;
 import net.minecraft.core.Direction;
 import net.minecraft.world.level.*;
@@ -38,7 +39,7 @@ import java.util.Optional;
 import java.util.function.Supplier;
 import java.util.stream.Stream;

-public class TintGetterOverrideSmooth extends AbstractDhTintGetter
+public class TintGetterOverride extends AbstractDhTintGetter
 {
 	private final LevelReader parent;
 	
@@ -48,9 +49,9 @@ public class TintGetterOverrideSmooth extends AbstractDhTintGetter
 	// constructor //
 	//=============//
 	
-	public TintGetterOverrideSmooth(LevelReader parent, BiomeWrapper biomeWrapper, FullDataSourceV2 fullDataSource)
+	public TintGetterOverride(LevelReader parent, BiomeWrapper biomeWrapper, FullDataSourceV2 fullDataSource, IClientLevelWrapper clientLevelWrapper)
 	{ 
-		super(biomeWrapper, fullDataSource); 
+		super(biomeWrapper, fullDataSource, clientLevelWrapper); 
 		this.parent = parent;
 	}
 	
@@ -20,6 +20,7 @@
 package com.seibel.distanthorizons.common.wrappers.block;

 import com.seibel.distanthorizons.core.dataObjects.fullData.sources.FullDataSourceV2;
+import com.seibel.distanthorizons.core.wrapperInterfaces.world.IClientLevelWrapper;
 import net.minecraft.core.BlockPos;
 import net.minecraft.core.Direction;
 import net.minecraft.world.level.block.entity.BlockEntity;
@@ -28,15 +29,15 @@ import net.minecraft.world.level.lighting.LevelLightEngine;
 import net.minecraft.world.level.material.FluidState;
 import org.jetbrains.annotations.Nullable;

-public class TintWithoutLevelSmoothOverrider extends AbstractDhTintGetter
+public class TintWithoutLevelOverrider extends AbstractDhTintGetter
 {
 	
 	//=============//
 	// constructor //
 	//=============//
 	
-	public TintWithoutLevelSmoothOverrider(BiomeWrapper biomeWrapper, FullDataSourceV2 fullDataSource)
-	{ super(biomeWrapper, fullDataSource); }
+	public TintWithoutLevelOverrider(BiomeWrapper biomeWrapper, FullDataSourceV2 fullDataSource, IClientLevelWrapper clientLevelWrapper)
+	{ super(biomeWrapper, fullDataSource, clientLevelWrapper); }
 	
 	
 	
@@ -46,21 +47,21 @@ public class TintWithoutLevelSmoothOverrider extends AbstractDhTintGetter
 	
 	@Override
 	public float getShade(Direction direction, boolean shade)
-	{ throw new UnsupportedOperationException("ERROR: getShade() called on TintWithoutLevelSmoothOverrider. Object is for tinting only."); }
+	{ throw new UnsupportedOperationException("ERROR: getShade() called on TintWithoutLevelOverrider. Object is for tinting only."); }
 	@Override
 	public LevelLightEngine getLightEngine()
-	{ throw new UnsupportedOperationException("ERROR: getLightEngine() called on TintWithoutLevelSmoothOverrider. Object is for tinting only."); }
+	{ throw new UnsupportedOperationException("ERROR: getLightEngine() called on TintWithoutLevelOverrider. Object is for tinting only."); }
 	@Nullable
 	@Override
 	public BlockEntity getBlockEntity(BlockPos pos)
-	{ throw new UnsupportedOperationException("ERROR: getBlockEntity() called on TintWithoutLevelSmoothOverrider. Object is for tinting only."); }
+	{ throw new UnsupportedOperationException("ERROR: getBlockEntity() called on TintWithoutLevelOverrider. Object is for tinting only."); }
 	
 	@Override
 	public BlockState getBlockState(BlockPos pos)
-	{ throw new UnsupportedOperationException("ERROR: getBlockState() called on TintWithoutLevelSmoothOverrider. Object is for tinting only."); }
+	{ throw new UnsupportedOperationException("ERROR: getBlockState() called on TintWithoutLevelOverrider. Object is for tinting only."); }
 	@Override
 	public FluidState getFluidState(BlockPos pos)
-	{ throw new UnsupportedOperationException("ERROR: getFluidState() called on TintWithoutLevelSmoothOverrider. Object is for tinting only."); }
+	{ throw new UnsupportedOperationException("ERROR: getFluidState() called on TintWithoutLevelOverrider. Object is for tinting only."); }
 	
 	
 	//==============//
@@ -71,7 +72,7 @@ public class TintWithoutLevelSmoothOverrider extends AbstractDhTintGetter
 	
 	@Override
 	public int getHeight()
-	{ throw new UnsupportedOperationException("ERROR: getHeight() called on TintWithoutLevelSmoothOverrider. Object is for tinting only."); }
+	{ throw new UnsupportedOperationException("ERROR: getHeight() called on TintWithoutLevelOverrider. Object is for tinting only."); }
 	
 	#if MC_VER < MC_1_21_3
 	@Override
@@ -80,7 +81,7 @@ public class TintWithoutLevelSmoothOverrider extends AbstractDhTintGetter
 	#else
 	@Override
 	public int getMinY()
-	{ throw new UnsupportedOperationException("ERROR: getMinY() called on TintWithoutLevelSmoothOverrider. Object is for tinting only."); }
+	{ throw new UnsupportedOperationException("ERROR: getMinY() called on TintWithoutLevelOverrider. Object is for tinting only."); }
 	#endif
 	
 	#endif