Implemented feedback from code review

src/main/java/io/github/opencubicchunks/cubicchunks/mixin/core/common/server/MixinMinecraftServer.java

@@ -34,6 +34,8 @@ public abstract class MixinMinecraftServer {
 
     @Shadow protected abstract void waitUntilNextTick();
 
+    // setInitialSpawn
+    // We replace the ChunkPos spawn position with a CubePos spawn position and reuse it later to get the world position.
     @Inject(method = "setInitialSpawn", at = @At(value = "INVOKE", target = "Lnet/minecraft/world/level/ChunkPos;<init>(Lnet/minecraft/core/BlockPos;)V"))
     private static void cc_replaceChunkPosInSetInitialSpawn(ServerLevel serverLevel, ServerLevelData serverLevelData, boolean generateBonusChest, boolean debug, CallbackInfo ci, @Share(
         "cubePos") LocalRef<CubePos> cubePosLocalRef) {
@@ -49,9 +51,12 @@ private static BlockPos cc_replaceGetWorldPositionInSetInitialSpawn(ChunkPos chu
         if (((CanBeCubic) serverLevel).cc_isCubic()) {
             return cubePosLocalRef.get().asChunkPos().getWorldPosition();
         }
-        return null;
+        return original.call(chunkPos);
     }
 
+    /**
+     * This mixin uses SpawnPlaceFinder (core CC2 code) in a similar fashion to the CC2 implementation.
+     */
     @WrapOperation(method = "setInitialSpawn",
         at = @At(value = "INVOKE", target = "Lnet/minecraft/server/level/ServerLevel;getHeight(Lnet/minecraft/world/level/levelgen/Heightmap$Types;II)I"))
     private static int cc_replaceGetHeightWithSpawnPlaceFinder(ServerLevel serverLevel, Heightmap.Types heightmapType, int x, int z, Operation<Integer> original,
@@ -63,20 +68,22 @@ private static int cc_replaceGetHeightWithSpawnPlaceFinder(ServerLevel serverLev
             if (topBlockBisect != null) {
                 return topBlockBisect.getY();
             } else {
-                return serverLevel.getSeaLevel() + 1;
+                return serverLevel.getSeaLevel() + 1; // This is the default value in vanilla
             }
         }
         return original.call(serverLevel, heightmapType, x, z);
     }
 
-    // TODO: Fix this when ServerChunkCache exists
+    // prepareLevels
+    // This mixin is copied from CC2. It fills in a spawnRadiusRef that is used to determine how many cubes we need to generate for spawn to be ready.
     @WrapWithCondition(method = "prepareLevels", at = @At(value = "INVOKE",
         target = "Lnet/minecraft/server/level/ServerChunkCache;addRegionTicket(Lnet/minecraft/server/level/TicketType;Lnet/minecraft/world/level/ChunkPos;ILjava/lang/Object;)V"))
     private <T> boolean cc_replaceAddRegionTicketInPrepareLevels(ServerChunkCache serverChunkCache, TicketType<T> ticketType, ChunkPos chunkPos, int originalSpawnRadius, T unit,
                                                                  @Share("spawnRadius") LocalRef<Integer> spawnRadiusRef) {
         if (((CanBeCubic) serverChunkCache).cc_isCubic()) {
             int spawnRadius = (int) Math.ceil(10 * (16 / (float) CubicConstants.DIAMETER_IN_BLOCKS)); //vanilla is 10, 32: 5, 64: 3
             spawnRadiusRef.set(spawnRadius);
+            // TODO: Fix this when ServerChunkCache exists
             //(CubicServerChunkCache)serverChunkCache.addRegionTicket(CubicTicketType.START, CloPos.cube(overworld().getSharedSpawnPos()), spawnRadius + 1, unit);
             return false;
         }
@@ -87,6 +94,7 @@ private <T> boolean cc_replaceAddRegionTicketInPrepareLevels(ServerChunkCache se
     private void cc_waitUntilCubicGenerationComplete(CallbackInfo ci, @Share("spawnRadius") LocalRef<Integer> spawnRadiusRef) {
         if (((CanBeCubic) overworld().getChunkSource()).cc_isCubic()) {
             int d = spawnRadiusRef.get() * 2 + 1;
+            // TODO: Fix this when ServerChunkCache exists
             //while (this.isRunning() && ((CubicServerChunkCache) overworld().getChunkSource()).getTickingGeneratedCubes() < d * d * d) {
             //    this.nextTickTimeNanos = Util.getMillis() + 10L;
             //    this.waitUntilNextTick();

src/main/java/io/github/opencubicchunks/cubicchunks/mixin/core/common/server/level/MixinPlayerRespawnLogic.java

@@ -15,6 +15,9 @@
 @Mixin(value = PlayerRespawnLogic.class, priority = 999)
 public class MixinPlayerRespawnLogic {
 
+    /**
+     * This mixin uses SpawnPlaceFinder (core CC2 code) in a similar fashion to the CC2 implementation.
+     */
     @Inject(method = "getOverworldRespawnPos", at = @At("HEAD"), cancellable = true)
     private static void getOverworldRespawnPos(ServerLevel level, int posX, int posZ, CallbackInfoReturnable<BlockPos> cir) {
         if (!((CubicLevelHeightAccessor) level).isCubic()) {

src/main/java/io/github/opencubicchunks/cubicchunks/mixin/core/common/world/level/MixinLevel.java

@@ -11,7 +11,6 @@
 import io.github.opencubicchunks.cc_core.utils.Coords;
 import io.github.opencubicchunks.cubicchunks.MarkableAsCubic;
 import io.github.opencubicchunks.cubicchunks.mixin.TransformFrom;
-import io.github.opencubicchunks.cubicchunks.world.level.CubicLevelReader;
 import io.github.opencubicchunks.cubicchunks.world.level.cube.CubicChunkSource;
 import io.github.opencubicchunks.cubicchunks.world.level.CubicLevel;
 import io.github.opencubicchunks.cubicchunks.world.level.cube.CubeAccess;
@@ -54,7 +53,7 @@ public void cc_setCubic() {
     }
 
     public LevelCube cc_getCubeAt(BlockPos blockPos) {
-        return (LevelCube)((CubicLevelReader)this).cc_getCube(Coords.blockToCube(blockPos.getX()), Coords.blockToCube(blockPos.getY()),
+        return this.cc_getCube(Coords.blockToCube(blockPos.getX()), Coords.blockToCube(blockPos.getY()),
             Coords.blockToCube(blockPos.getZ()));
     }
 
@@ -74,6 +73,7 @@ public CubeAccess cc_getCube(int cubeX, int cubeY, int cubeZ, ChunkStatus status
     }
 
     // setBlock
+    // Uses LevelChunk to call setBlockState and markAndNotifyBlock, so we replace it with a LevelCube and call the Cubic variants of those functions.
     @WrapOperation(method = "setBlock(Lnet/minecraft/core/BlockPos;Lnet/minecraft/world/level/block/state/BlockState;II)Z", at = @At(value = "INVOKE", target = "Lnet/minecraft/world/level"
         + "/Level;getChunkAt(Lnet/minecraft/core/BlockPos;)Lnet/minecraft/world/level/chunk/LevelChunk;"))
     private LevelChunk cc_replaceLevelChunkInGetChunkAt(Level level, BlockPos blockPos, Operation<LevelChunk> original, @Share("levelCube") LocalRef<LevelCube> levelCubeLocalRef) {
@@ -88,8 +88,7 @@ private LevelChunk cc_replaceLevelChunkInGetChunkAt(Level level, BlockPos blockP
     private BlockState cc_replaceLevelChunkInSetBlockState(LevelChunk levelChunk, BlockPos blockPos, BlockState blockState, boolean flag1, Operation<BlockState> original,
                                                            @Share("levelCube") LocalRef<LevelCube> levelCubeLocalRef) {
         if(cc_isCubic) {
-            levelCubeLocalRef.get().setBlockState(blockPos, blockState, flag1);
-            return null;
+            return levelCubeLocalRef.get().setBlockState(blockPos, blockState, flag1);
         }
         return original.call(levelChunk, blockPos, blockState, flag1);
     }
@@ -108,6 +107,7 @@ private boolean cc_replaceLevelChunkInMarkAndNotifyBlock(Level level, BlockPos b
     public native void markAndNotifyBlock(BlockPos blockPos, @Nullable LevelCube levelCube, BlockState blockStatePrev, BlockState blockStateNew, int flags, int p_46608_);
 
     // getBlockState
+    // Replaces LevelChunk with a LevelCube to call getBlockState
     @Inject(method = "getBlockState", at = @At(value = "INVOKE", target = "Lnet/minecraft/world/level/Level;getChunk(II)Lnet/minecraft/world/level/chunk/LevelChunk;"))
     private void cc_replaceLevelChunkInGetBlockState(BlockPos blockPos, CallbackInfoReturnable<BlockState> cir, @Share("levelCube") LocalRef<LevelCube> levelCubeLocalRef) {
         if(cc_isCubic) {
@@ -126,6 +126,7 @@ private BlockState cc_replaceLevelChunkInGetBlockState(LevelChunk levelChunk, Bl
     }
 
     // getBlockEntity
+    // Replaces LevelChunk with a LevelCube to call getBlockEntity
     @Inject(method = "getBlockEntity", at = @At(value = "INVOKE", target = "Lnet/minecraft/world/level/Level;getChunkAt(Lnet/minecraft/core/BlockPos;)"
         + "Lnet/minecraft/world/level/chunk/LevelChunk;"), cancellable = true)
     private void cc_replaceGetChunkAtInSetBlockEntity(BlockPos blockPos, CallbackInfoReturnable<BlockEntity> cir) {
@@ -135,6 +136,7 @@ private void cc_replaceGetChunkAtInSetBlockEntity(BlockPos blockPos, CallbackInf
     }
 
     // getFluidState
+    // Replaces LevelChunk with a LevelCube to call getFluidState
     @WrapOperation(method = "getFluidState", at = @At(value = "INVOKE", target = "Lnet/minecraft/world/level/chunk/LevelChunk;getFluidState(Lnet/minecraft/core/BlockPos;)Lnet/minecraft/world/level/material/FluidState;"))
     private FluidState cc_replaceGetChunkAtInGetFluidState(LevelChunk levelChunk, BlockPos blockPos, Operation<FluidState> original) {
         if(cc_isCubic) {
@@ -144,6 +146,7 @@ private FluidState cc_replaceGetChunkAtInGetFluidState(LevelChunk levelChunk, Bl
     }
 
     // setBlockEntity
+    // Replaces LevelChunk with a LevelCube to call addAndRegisterBlockEntity
     @Inject(method = "setBlockEntity", at = @At(value="INVOKE", target="Lnet/minecraft/world/level/Level;getChunkAt(Lnet/minecraft/core/BlockPos;)"
         + "Lnet/minecraft/world/level/chunk/LevelChunk;"), cancellable=true)
     private void cc_replaceLevelChunkInSetBlockEntity(BlockEntity blockEntity, CallbackInfo ci, @Local(ordinal = 0) BlockPos blockPos) {
@@ -154,24 +157,37 @@ private void cc_replaceLevelChunkInSetBlockEntity(BlockEntity blockEntity, Callb
     }
 
     // removeBlockEntity
-    @Inject(method = "removeBlockEntity", at = @At(value="INVOKE", target="Lnet/minecraft/world/level/Level;getChunkAt(Lnet/minecraft/core/BlockPos;)Lnet/minecraft/world/level/chunk/LevelChunk;"), cancellable=true)
-    private void cc_replaceGetChunkAtInRemoveBlockEntity(BlockPos blockPos, CallbackInfo ci) {
+    // Replaces LevelChunk with a LevelCube to call removeBlockEntity, needs a local ref to do so
+    @WrapOperation(method = "removeBlockEntity", at = @At(value="INVOKE", target="Lnet/minecraft/world/level/Level;getChunkAt(Lnet/minecraft/core/BlockPos;)Lnet/minecraft/world/level/chunk/LevelChunk;"))
+    private LevelChunk cc_replaceGetChunkAtInRemoveBlockEntity(Level level, BlockPos pPos, Operation<LevelChunk> original, @Share("levelCube") LocalRef<LevelCube> levelCubeLocalRef) {
         if(cc_isCubic) {
-            this.cc_getCubeAt(blockPos).removeBlockEntity(blockPos);
-            ci.cancel();
+            levelCubeLocalRef.set(cc_getCubeAt(pPos));
+            return null;
+        }
+        return original.call(level, pPos);
+    }
+
+    @WrapOperation(method = "removeBlockEntity", at = @At(value="INVOKE", target="Lnet/minecraft/world/level/chunk/LevelChunk;removeBlockEntity(Lnet/minecraft/core/BlockPos;)V"))
+    private void cc_replaceLevelChunkInRemoveBlockEntity(LevelChunk levelChunk, BlockPos pPos, Operation<Void> original, @Share("levelCube") LocalRef<LevelCube> levelCubeLocalRef) {
+        if(cc_isCubic) {
+            levelCubeLocalRef.get().removeBlockEntity(pPos);
+        } else {
+            original.call(levelChunk, pPos);
         }
     }
 
     // isLoaded
+    // Replaces ChunkSource with a CubicChunkSource to call hasCube
     @WrapOperation(method = "isLoaded", at = @At(value="INVOKE", target="Lnet/minecraft/world/level/chunk/ChunkSource;hasChunk(II)Z"))
     private boolean cc_replaceHasChunkInIsLoaded(ChunkSource chunkSource, int x, int z, Operation<Boolean> original, BlockPos blockPos) {
         if(cc_isCubic) {
-            return ((CubicChunkSource)this.getChunkSource()).cc_hasCube(Coords.blockToCube(blockPos.getX()), Coords.blockToCube(blockPos.getY()), Coords.blockToCube(blockPos.getZ()));
+            return ((CubicChunkSource)chunkSource).cc_hasCube(Coords.blockToCube(blockPos.getX()), Coords.blockToCube(blockPos.getY()), Coords.blockToCube(blockPos.getZ()));
         }
         return false;
     }
 
     // loadedAndEntityCanStandOnFace
+    // Uses an inject here since the entire second half of the method needs to be replaced anyways
     @Inject(method = "loadedAndEntityCanStandOnFace", at = @At(value = "INVOKE", target = "Lnet/minecraft/world/level/Level;getChunk(IILnet/minecraft/world/level/chunk/ChunkStatus;Z)"
         + "Lnet/minecraft/world/level/chunk/ChunkAccess;"), cancellable = true)
     private void cc_replaceGetChunkAtInLoadedAndEntityCanStandOnFace(BlockPos blockPos, Entity entity, Direction direction, CallbackInfoReturnable<Boolean> cir) {
@@ -182,6 +198,7 @@ private void cc_replaceGetChunkAtInLoadedAndEntityCanStandOnFace(BlockPos blockP
     }
 
     // blockEntityChanged
+    // This function is small enough that we can just replace it entirely
     @Inject(method = "blockEntityChanged", at = @At(value = "HEAD"), cancellable = true)
     private void cc_replaceBlockEntityChanged(BlockPos blockPos, CallbackInfo ci) {
         if(cc_isCubic) {
@@ -193,6 +210,8 @@ private void cc_replaceBlockEntityChanged(BlockPos blockPos, CallbackInfo ci) {
     }
 
     // getCurrentDifficultyAt
+    // This function isn't worth trying to wrap due to its complexity, so we just replace it entirely
+    // Local difficulty is not something people mod so this is fine
     @Inject(method = "getCurrentDifficultyAt", at = @At(value = "HEAD"), cancellable = true)
     private void cc_replaceGetCurrentDifficultyAt(BlockPos blockPos, CallbackInfoReturnable<DifficultyInstance> cir) {
         if(cc_isCubic) {