Fix light emitting blocks in copycats (fixes Creators-of-Create/Creat…

…e#4889 for Copycats+) This bug is caused by a mix of copycats not requesting a light update properly after changing materials, and block entities being inaccessible in worker threads that handle light updates. The former issue is easily fixable, but I'm not exactly sure what's a proper fix for the latter one. I'm accessing the block entity hash map directly from ChunkAccess and wrapping the access in a try-catch in case of concurrency issues, but it might still be unstable in rare cases. This fix only applies to copycats from Copycats+, not Create's copycats.
copycats-plus · Jul 30, 2024 · 0907673 · 0907673
1 parent 1e00124
commit 0907673
Show file tree

Hide file tree

Showing 10 changed files with 105 additions and 37 deletions.
diff --git a/common/src/main/java/com/copycatsplus/copycats/foundation/copycat/ICopycatBlock.java b/common/src/main/java/com/copycatsplus/copycats/foundation/copycat/ICopycatBlock.java
@@ -1,6 +1,7 @@
 package com.copycatsplus.copycats.foundation.copycat;
 
 import com.copycatsplus.copycats.foundation.copycat.multistate.IMultiStateCopycatBlock;
+import com.copycatsplus.copycats.mixin.foundation.copycat.ChunkAccessAccessor;
 import com.copycatsplus.copycats.utility.BlockEntityUtils;
 import com.copycatsplus.copycats.utility.BlockFaceUtils;
 import com.simibubi.create.AllBlocks;
@@ -323,6 +324,16 @@ static BlockState getMaterial(BlockGetter reader, BlockPos targetPos) {
         return Blocks.AIR.defaultBlockState();
     }
 
+    /**
+     * Get the material of the copycat at the given position when not executing on the main thread.
+     * Block entity access is unsafe on other threads, so this method should be used with caution.
+     */
+    static BlockState getMaterialCrossThread(BlockGetter reader, BlockPos targetPos) {
+        if (BlockEntityUtils.getBlockEntityCrossThread(reader, targetPos) instanceof ICopycatBlockEntity cbe)
+            return cbe.getMaterial();
+        return Blocks.AIR.defaultBlockState();
+    }
+
     /**
      * Utility to get the required items for a layer of a block state.
      */

diff --git a/common/src/main/java/com/copycatsplus/copycats/foundation/copycat/ICopycatBlockEntity.java b/common/src/main/java/com/copycatsplus/copycats/foundation/copycat/ICopycatBlockEntity.java
@@ -116,10 +116,7 @@ default void setMaterial(BlockState blockState) {
             }
 
         setMaterialInternal(blockState);
-        if (!getLevel().isClientSide()) {
-            notifyUpdate();
-            return;
-        }
+
         BlockEntityUtils.redraw((BlockEntity) this);
     }
 
@@ -201,7 +198,7 @@ static void read(ICopycatBlockEntity self, CompoundTag tag, boolean clientPacket
             self.setMaterialInternal(AllBlocks.COPYCAT_BASE.getDefaultState());
         }
 
-        if (clientPacket && prevMaterial != self.getMaterial())
+        if (prevMaterial != self.getMaterial())
             BlockEntityUtils.redraw((BlockEntity) self); // not calling self.redraw() because Extended Cogwheels overwrites it to be protected
     }
 

diff --git a/...om/copycatsplus/copycats/foundation/copycat/multistate/IMultiStateCopycatBlockEntity.java b/...om/copycatsplus/copycats/foundation/copycat/multistate/IMultiStateCopycatBlockEntity.java
@@ -174,10 +174,7 @@ default void setMaterial(String property, BlockState blockState) {
         MaterialItemStorage.MaterialItem materialItem = getMaterialItemStorage().getMaterialItem(property);
         materialItem.setMaterial(blockState);
         getMaterialItemStorage().storeMaterialItem(property, materialItem);
-        if (!getLevel().isClientSide()) {
-            notifyUpdate();
-            return;
-        }
+
         BlockEntityUtils.redraw((BlockEntity) this);
     }
 
@@ -238,7 +235,7 @@ static void read(IMultiStateCopycatBlockEntity self, CompoundTag tag, boolean cl
         if (self.getBlockState().getBlock() instanceof IMultiStateCopycatBlock) {
             boolean anyUpdated = self.getMaterialItemStorage().deserialize(tag.getCompound("material_data"));
 
-            if (clientPacket && anyUpdated)
+            if (anyUpdated)
                 BlockEntityUtils.redraw((BlockEntity) self); // not calling self.redraw() because Extended Cogwheels overwrites it to be protected
         }
     }

diff --git a/...src/main/java/com/copycatsplus/copycats/mixin/foundation/copycat/ChunkAccessAccessor.java b/...src/main/java/com/copycatsplus/copycats/mixin/foundation/copycat/ChunkAccessAccessor.java
@@ -0,0 +1,15 @@
+package com.copycatsplus.copycats.mixin.foundation.copycat;
+
+import net.minecraft.core.BlockPos;
+import net.minecraft.world.level.block.entity.BlockEntity;
+import net.minecraft.world.level.chunk.ChunkAccess;
+import org.spongepowered.asm.mixin.Mixin;
+import org.spongepowered.asm.mixin.gen.Accessor;
+
+import java.util.Map;
+
+@Mixin(ChunkAccess.class)
+public interface ChunkAccessAccessor {
+    @Accessor
+    Map<BlockPos, BlockEntity> getBlockEntities();
+}
diff --git a/common/src/main/java/com/copycatsplus/copycats/utility/BlockEntityUtils.java b/common/src/main/java/com/copycatsplus/copycats/utility/BlockEntityUtils.java
@@ -1,10 +1,18 @@
 package com.copycatsplus.copycats.utility;
 
+import com.copycatsplus.copycats.mixin.foundation.copycat.ChunkAccessAccessor;
 import dev.architectury.injectables.annotations.ExpectPlatform;
 import net.minecraft.MethodsReturnNonnullByDefault;
+import net.minecraft.core.BlockPos;
+import net.minecraft.server.level.ServerLevel;
+import net.minecraft.util.profiling.ProfilerFiller;
+import net.minecraft.world.level.BlockGetter;
+import net.minecraft.world.level.Level;
 import net.minecraft.world.level.block.entity.BlockEntity;
 import net.minecraft.world.level.block.state.BlockState;
+import org.apache.logging.log4j.core.jmx.Server;
 
+import javax.annotation.Nullable;
 import javax.annotation.ParametersAreNonnullByDefault;
 
 @MethodsReturnNonnullByDefault
@@ -14,18 +22,53 @@ public class BlockEntityUtils {
      * Invalidate render data caches for the block entity and re-render it.
      */
     public static void redraw(BlockEntity blockEntity) {
-        requestModelDataUpdate(blockEntity);
-        if (blockEntity.getLevel() != null) {
+        Level level = blockEntity.getLevel();
+        if (level != null) {
+            if (level.isClientSide()) {
+                requestModelDataUpdate(blockEntity);
+            } else {
+                blockEntity.setChanged();
+            }
             BlockState state = blockEntity.getBlockState();
-            blockEntity.getLevel().sendBlockUpdated(blockEntity.getBlockPos(), state, state, 16);
-            blockEntity.getLevel().getChunkSource()
-                    .getLightEngine()
-                    .checkBlock(blockEntity.getBlockPos());
+            level.sendBlockUpdated(blockEntity.getBlockPos(), state, state, 16);
+            updateLight(blockEntity);
         }
     }
 
     @ExpectPlatform
     public static void requestModelDataUpdate(BlockEntity blockEntity) {
 
     }
+
+    /**
+     * Get the block entity at the target position while not executing in the main thread.
+     * <p>
+     * Accessing block entities from other threads is unsafe. Use with caution.
+     */
+    @Nullable
+    public static BlockEntity getBlockEntityCrossThread(BlockGetter reader, BlockPos targetPos) {
+        try {
+            if (reader instanceof Level level) {
+                ChunkAccessAccessor chunkAccess = (ChunkAccessAccessor) level.getChunk(targetPos);
+                return chunkAccess.getBlockEntities().get(targetPos);
+            }
+            return null;
+        } catch (Exception $) {
+            return null;
+        }
+    }
+
+    private static void updateLight(BlockEntity blockEntity) {
+        Level level = blockEntity.getLevel();
+        if (level != null) {
+            BlockPos pos = blockEntity.getBlockPos();
+            ProfilerFiller profilerFiller = level.getProfiler();
+            profilerFiller.push("updateSkyLightSources");
+            level.getChunk(pos).getSkyLightSources().update(level, pos.getX() & 0xF, pos.getY(), pos.getZ() & 0xF);
+            profilerFiller.popPush("queueCheckLight");
+            level.getChunkSource().getLightEngine().checkBlock(pos);
+            profilerFiller.pop();
+            level.getChunk(pos).setUnsaved(true);
+        }
+    }
 }
diff --git a/common/src/main/resources/copycats-common.mixins.json b/common/src/main/resources/copycats-common.mixins.json
@@ -12,27 +12,28 @@
     "compat.rubidium.BlockOcclusionCacheMixin",
     "compat.rubidium.BlockRendererMixin",
     "copycat.VoxelShapeAccessor",
+    "copycat.cogwheel.CogWheelBlockItemMixin",
+    "copycat.fluid_pipe.FluidPipeBlockMixin",
+    "copycat.panel.CopycatPanelBlockMixin",
+    "copycat.slab.SeatMovementBehaviourMixin",
+    "copycat.slab.TrackPlacementMixin",
+    "copycat.slab.WalkNodeEvaluatorMixin",
+    "copycat.step.CopycatStepBlockMixin",
+    "copycat.trapdoor.LivingEntityMixin",
+    "entity.CatMixin",
+    "feature_toggle.CreativeModeTabsAccessor",
     "foundation.copycat.BearingContraptionMixin",
     "foundation.copycat.BlockMixin",
     "foundation.copycat.BlockStateBaseCacheMixin",
+    "foundation.copycat.ChunkAccessAccessor",
     "foundation.copycat.ConnectedTextureBehaviourMixin",
     "foundation.copycat.CopycatBlockEntityMixin",
     "foundation.copycat.CopycatBlockMixin",
     "foundation.copycat.kinetic.CreateClientMixin",
     "foundation.copycat.migration.ContraptionMixin",
     "foundation.copycat.migration.LevelChunkMixin",
     "foundation.copycat.migration.StructureTemplateMixin",
-    "foundation.copycat.multistate.AirCurrentMixin",
-    "copycat.cogwheel.CogWheelBlockItemMixin",
-    "copycat.fluid_pipe.FluidPipeBlockMixin",
-    "copycat.panel.CopycatPanelBlockMixin",
-    "copycat.slab.SeatMovementBehaviourMixin",
-    "copycat.slab.TrackPlacementMixin",
-    "copycat.slab.WalkNodeEvaluatorMixin",
-    "copycat.step.CopycatStepBlockMixin",
-    "copycat.trapdoor.LivingEntityMixin",
-    "entity.CatMixin",
-    "feature_toggle.CreativeModeTabsAccessor"
+    "foundation.copycat.multistate.AirCurrentMixin"
   ],
   "client": [
     "foundation.copycat.ClientLevelMixin",

diff --git a/...ain/java/com/copycatsplus/copycats/fabric/mixin/foundation/copycat/CopycatBlockMixin.java b/...ain/java/com/copycatsplus/copycats/fabric/mixin/foundation/copycat/CopycatBlockMixin.java
@@ -91,11 +91,11 @@ public float getFriction(BlockState state, LevelReader level, BlockPos pos, Enti
 
     @Override
     public int getLightEmission(BlockState state, BlockGetter level, BlockPos pos) {
-        return maybeMaterialAs(
-                level, pos, LightEmissiveBlock.class,
-                (material, block) -> block.getLightEmission(material, level, pos),
-                BlockStateBase::getLightEmission
-        );
+        BlockState material = ICopycatBlock.getMaterialCrossThread(level, pos);
+        Block block = material.getBlock();
+        if (block instanceof LightEmissiveBlock lightEmissiveBlock)
+            return lightEmissiveBlock.getLightEmission(material, level, pos);
+        return material.getLightEmission();
     }
 
     @Override

diff --git a/...plus/copycats/fabric/mixin/foundation/copycat/multistate/MultiStateCopycatBlockMixin.java b/...plus/copycats/fabric/mixin/foundation/copycat/multistate/MultiStateCopycatBlockMixin.java
@@ -5,6 +5,7 @@
 import com.copycatsplus.copycats.foundation.copycat.multistate.IMultiStateCopycatBlockEntity;
 import com.copycatsplus.copycats.foundation.copycat.multistate.MultiStateCopycatBlock;
 import com.copycatsplus.copycats.content.copycat.cogwheel.CopycatCogWheelBlock;
+import com.copycatsplus.copycats.utility.BlockEntityUtils;
 import com.simibubi.create.AllBlocks;
 import io.github.fabricators_of_create.porting_lib.block.*;
 import io.github.fabricators_of_create.porting_lib.enchant.EnchantmentBonusBlock;
@@ -23,6 +24,7 @@
 import net.minecraft.world.level.block.Block;
 import net.minecraft.world.level.block.Blocks;
 import net.minecraft.world.level.block.SoundType;
+import net.minecraft.world.level.block.entity.BlockEntity;
 import net.minecraft.world.level.block.state.BlockState;
 import net.minecraft.world.phys.BlockHitResult;
 import net.minecraft.world.phys.HitResult;
@@ -91,8 +93,8 @@ public int getLightEmission(BlockState state, BlockGetter level, BlockPos pos) {
         if (state.getBlock() instanceof IMultiStateCopycatBlock copycatBlock) {
             AtomicInteger light = new AtomicInteger(0);
 
-            IMultiStateCopycatBlockEntity copycatBE = copycatBlock.getCopycatBlockEntity(level, pos);
-            if (copycatBE == null)
+            BlockEntity be = BlockEntityUtils.getBlockEntityCrossThread(level, pos);
+            if (!(be instanceof IMultiStateCopycatBlockEntity copycatBE))
                 return state.getLightEmission();
             copycatBE.getMaterialItemStorage().getAllMaterials().forEach(bs -> {
                 light.accumulateAndGet(bs.getLightEmission(), Math::max);

diff --git a/...main/java/com/copycatsplus/copycats/forge/mixin/foundation/copycat/CopycatBlockMixin.java b/...main/java/com/copycatsplus/copycats/forge/mixin/foundation/copycat/CopycatBlockMixin.java
@@ -76,7 +76,7 @@ public float getFriction(BlockState state, LevelReader level, BlockPos pos, Enti
 
     @Override
     public int getLightEmission(BlockState state, BlockGetter level, BlockPos pos) {
-        return getMaterial(level, pos).getLightEmission(level, pos);
+        return ICopycatBlock.getMaterialCrossThread(level, pos).getLightEmission(level, pos);
     }
 
     @Override

diff --git a/...splus/copycats/forge/mixin/foundation/copycat/multistate/MultiStateCopycatBlockMixin.java b/...splus/copycats/forge/mixin/foundation/copycat/multistate/MultiStateCopycatBlockMixin.java
@@ -5,6 +5,7 @@
 import com.copycatsplus.copycats.foundation.copycat.multistate.IMultiStateCopycatBlockEntity;
 import com.copycatsplus.copycats.foundation.copycat.multistate.MultiStateCopycatBlock;
 import com.copycatsplus.copycats.content.copycat.cogwheel.CopycatCogWheelBlock;
+import com.copycatsplus.copycats.utility.BlockEntityUtils;
 import com.simibubi.create.AllBlocks;
 import net.minecraft.core.BlockPos;
 import net.minecraft.core.Direction;
@@ -19,6 +20,7 @@
 import net.minecraft.world.level.block.Block;
 import net.minecraft.world.level.block.Blocks;
 import net.minecraft.world.level.block.SoundType;
+import net.minecraft.world.level.block.entity.BlockEntity;
 import net.minecraft.world.level.block.state.BlockState;
 import net.minecraft.world.phys.BlockHitResult;
 import net.minecraft.world.phys.HitResult;
@@ -75,8 +77,8 @@ public int getLightEmission(BlockState state, BlockGetter level, BlockPos pos) {
         if (state.getBlock() instanceof IMultiStateCopycatBlock copycatBlock) {
             AtomicInteger light = new AtomicInteger(0);
 
-            IMultiStateCopycatBlockEntity copycatBE = copycatBlock.getCopycatBlockEntity(level, pos);
-            if (copycatBE == null)
+            BlockEntity be = BlockEntityUtils.getBlockEntityCrossThread(level, pos);
+            if (!(be instanceof IMultiStateCopycatBlockEntity copycatBE))
                 return super.getLightEmission(state, level, pos);
             copycatBE.getMaterialItemStorage().getAllMaterials().forEach(bs -> {
                 light.accumulateAndGet(bs.getLightEmission(), Math::max);