Skip to content

Commit

Permalink
Fix load order for empty cubes and chunks
Browse files Browse the repository at this point in the history 
Add additional load order assertions in IntegrationTestCubicChunkMap
  • Loading branch information
@CursedFlames @NotStirred
CursedFlames authored and NotStirred committed Jun 27, 2024
1 parent d8be6ba commit ed1de33
Show file tree
Hide file tree
Showing 3 changed files with 179 additions and 14 deletions.
95 changes: 94 additions & 1 deletion ...a/io/github/opencubicchunks/cubicchunks/mixin/core/common/server/level/MixinChunkMap.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@
import io.github.notstirred.dasm.api.annotations.transform.TransformFromMethod;
import io.github.opencubicchunks.cc_core.api.CubicConstants;
import io.github.opencubicchunks.cc_core.utils.Coords;
import io.github.opencubicchunks.cubicchunks.mixin.core.common.world.level.chunk.storage.MixinChunkStorage;
import io.github.opencubicchunks.cubicchunks.mixin.dasmsets.GeneralSet;
import io.github.opencubicchunks.cubicchunks.mixin.dasmsets.SectionPosToCubeSet;
import io.github.opencubicchunks.cubicchunks.mixin.core.common.world.level.chunk.storage.MixinChunkStorage;
import io.github.opencubicchunks.cubicchunks.server.level.CloTrackingView;
import io.github.opencubicchunks.cubicchunks.server.level.CubicChunkHolder;
import io.github.opencubicchunks.cubicchunks.server.level.CubicChunkMap;
Expand Down Expand Up @@ -83,6 +83,12 @@ public abstract class MixinChunkMap extends MixinChunkStorage implements CubicCh

@Shadow protected abstract ChunkHolder getUpdatingChunkIfPresent(long aLong);

@Shadow private static boolean isChunkDataValid(CompoundTag pTag) {
throw new IllegalStateException("mixin failed to apply");
}

@Shadow @Final private BlockableEventLoop<Runnable> mainThreadExecutor;
@Shadow @Final ServerLevel level;
@AddFieldToSets(sets = GeneralSet.class, owner = @Ref(ChunkMap.class), field = @FieldSig(type = @Ref(ChunkProgressListener.class), name = "progressListener"))
private CubicChunkProgressListener cc_progressListener;

Expand Down Expand Up @@ -279,9 +285,96 @@ private boolean cc_updateChunkScheduling_onForgeHook(ServerLevel level, long pos
@AddTransformToSets(GeneralSet.class) @TransformFromMethod(@MethodSig("schedule(Lnet/minecraft/server/level/ChunkHolder;Lnet/minecraft/world/level/chunk/ChunkStatus;)Ljava/util/concurrent/CompletableFuture;"))
public native CompletableFuture<Either<CloAccess, ChunkHolder.ChunkLoadingFailure>> cc_schedule(ChunkHolder pHolder, ChunkStatus pStatus);

// dasm + mixin
@AddTransformToSets(GeneralSet.class) @TransformFromMethod(@MethodSig("scheduleChunkLoad(Lnet/minecraft/world/level/ChunkPos;)Ljava/util/concurrent/CompletableFuture;"))
private native CompletableFuture<Either<CloAccess, ChunkHolder.ChunkLoadingFailure>> cc_scheduleChunkLoad(CloPos cloPos);

/**
* Loading cubes at EMPTY requires additional logic to ensure that the corresponding chunks are loaded first
* (Unlike other statuses, this dependency is not handled in {@link ChunkMap#getChunkRangeFuture})
*/
@Dynamic @Inject(method = "cc_dasm$cc_scheduleChunkLoad", at = @At("HEAD"), cancellable = true)
private void cc_onScheduleChunkLoad(CloPos pos,
CallbackInfoReturnable<CompletableFuture<Either<CloAccess, ChunkHolder.ChunkLoadingFailure>>> cir) {
if (!pos.isCube()) return;
// TODO this is a bit of a disaster, why did mojang ever design their code around futures of Eithers
// Logic for loading cube-adjacent chunks first, similar to getChunkRangeFuture
List<CompletableFuture<Either<CloAccess, ChunkHolder.ChunkLoadingFailure>>> chunkLoadFutures = new ArrayList<>();

for (int sectionZ = 0; sectionZ < CubicConstants.DIAMETER_IN_SECTIONS; sectionZ++) {
for (int sectionX = 0; sectionX < CubicConstants.DIAMETER_IN_SECTIONS; sectionX++) {
final CloPos chunkPos = CloPos.chunk(Coords.cubeToSection(pos.getX(), sectionX), Coords.cubeToSection(pos.getZ(), sectionZ));
long chunkPosLong = chunkPos.toLong();
ChunkHolder chunkholder = this.getUpdatingChunkIfPresent(chunkPosLong);
if (chunkholder == null) { // This shouldn't occur as DistanceManager should add chunks to the ChunkMap before their corresponding cubes
cir.setReturnValue(CompletableFuture.completedFuture(Either.right(new ChunkHolder.ChunkLoadingFailure() {
@Override
public String toString() {
return "Unloaded " + chunkPos;
}
})));
return;
}

CompletableFuture<Either<CloAccess, ChunkHolder.ChunkLoadingFailure>> completablefuture = ((CubicChunkHolder) chunkholder).cc_getOrScheduleFuture(
ChunkStatus.EMPTY, (ChunkMap) (Object) this
);
chunkLoadFutures.add(completablefuture);
}
}

CompletableFuture<List<Either<CloAccess, ChunkHolder.ChunkLoadingFailure>>> chunkResultsFuture = Util.sequence(chunkLoadFutures);
CompletableFuture<Either<List<CloAccess>, ChunkHolder.ChunkLoadingFailure>> allChunksLoadedFuture = chunkResultsFuture.thenApply(p_183730_ -> {
List<CloAccess> list2 = Lists.newArrayList();
int index = 0;

for(final Either<CloAccess, ChunkHolder.ChunkLoadingFailure> either : p_183730_) {
if (either == null) {
throw this.debugFuturesAndCreateReportedException(new IllegalStateException("At least one of the chunk futures were null"), "n/a");
}

Optional<CloAccess> optional = either.left();
if (optional.isEmpty()) {
final int indexFinal = index; // thanks java
return Either.right(new ChunkHolder.ChunkLoadingFailure() {
@Override
public String toString() {
return "Unloaded chunk for cube " + pos + "offset: " + new ChunkPos(indexFinal % CubicConstants.DIAMETER_IN_SECTIONS, indexFinal / CubicConstants.DIAMETER_IN_SECTIONS) + " " + either.right().get();
}
});
}

list2.add(optional.get());
++index;
}

return Either.left(list2);
});
// Wait for adjacent chunks to load, and then load the cube
// TODO allChunksLoadedFuture and cc_readChunk could (and probably should) run in parallel, it just makes this future logic a bit more complex
cir.setReturnValue(allChunksLoadedFuture
.thenCompose((result) -> result.map(
left -> this.cc_readChunk(pos).thenApply(p_214925_ -> p_214925_.filter(p_214928_ -> {
boolean flag = isChunkDataValid(p_214928_);
if (!flag) {
LOGGER.error("Chunk file at {} is missing level data, skipping", pos);
}
return flag;
})).<Either<CloAccess, ChunkHolder.ChunkLoadingFailure>>thenApplyAsync(p_313584_ -> {
this.level.getProfiler().incrementCounter("chunkLoad");
// TODO (P2) loading save data
// if (p_313584_.isPresent()) {
// ChunkAccess chunkaccess = ChunkSerializer.read(this.level, this.poiManager, pos, p_313584_.get());
// this.markPosition(pos, chunkaccess.getStatus().getChunkType());
// return Either.left(chunkaccess);
// } else {
return Either.left(this.cc_createEmptyChunk(pos));
// }
}, this.mainThreadExecutor).exceptionallyAsync(p_214888_ -> this.cc_handleChunkLoadFailure(p_214888_, pos), this.mainThreadExecutor),
right -> CompletableFuture.completedFuture(Either.right(right)))));

}

@AddTransformToSets(GeneralSet.class) @TransformFromMethod(@MethodSig("handleChunkLoadFailure(Ljava/lang/Throwable;Lnet/minecraft/world/level/ChunkPos;)Lcom/mojang/datafixers/util/Either;"))
private native Either<CloAccess, ChunkHolder.ChunkLoadingFailure> cc_handleChunkLoadFailure(Throwable exception, CloPos cloPos);

Expand Down
94 changes: 81 additions & 13 deletions ...pencubicchunks/cubicchunks/integrationtest/server/level/IntegrationTestCubicChunkMap.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package io.github.opencubicchunks.cubicchunks.integrationtest.server.level;

import static org.junit.jupiter.api.Assertions.assertInstanceOf;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
Expand All @@ -9,6 +10,10 @@

import java.io.IOException;
import java.nio.file.Files;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.CompletableFuture;
import java.util.stream.Stream;

Expand All @@ -18,10 +23,12 @@
import io.github.opencubicchunks.cubicchunks.mixin.test.common.server.level.ChunkMapTestAccess;
import io.github.opencubicchunks.cubicchunks.mixin.test.common.server.level.ServerChunkCacheTestAccess;
import io.github.opencubicchunks.cubicchunks.test.LongRunTest;
import io.github.opencubicchunks.cubicchunks.server.level.CubicChunkHolder;
import io.github.opencubicchunks.cubicchunks.testutils.BaseTest;
import io.github.opencubicchunks.cubicchunks.testutils.CloseableReference;
import io.github.opencubicchunks.cubicchunks.world.level.chunklike.CloPos;
import io.github.opencubicchunks.cubicchunks.world.level.cube.LevelCube;
import it.unimi.dsi.fastutil.longs.Long2ObjectLinkedOpenHashMap;
import net.minecraft.server.level.ChunkHolder;
import net.minecraft.server.level.ChunkLevel;
import net.minecraft.server.level.ChunkMap;
Expand All @@ -43,21 +50,23 @@

@TestInstance(TestInstance.Lifecycle.PER_CLASS)
public class IntegrationTestCubicChunkMap extends BaseTest {
private CloseableReference<ServerChunkCache> createServerChunkCache() throws IOException {
private CloseableReference<ServerChunkCache> createServerChunkCache(boolean vanillaTest) throws IOException {
// Worldgen internals
var randomStateMockedStatic = Mockito.mockStatic(RandomState.class, withSettings().defaultAnswer(Answers.RETURNS_DEEP_STUBS));
NoiseBasedChunkGenerator noiseBasedChunkGeneratorMock = mock(Mockito.RETURNS_DEEP_STUBS);
when(noiseBasedChunkGeneratorMock.createBiomes(any(),any(),any(),any(),any())).thenAnswer(i -> CompletableFuture.completedFuture(i.getArguments()[4]));
when(noiseBasedChunkGeneratorMock.fillFromNoise(any(),any(),any(),any(),any())).thenAnswer(i -> CompletableFuture.completedFuture(i.getArguments()[4]));
NoiseBasedChunkGenerator noiseBasedChunkGeneratorMock = mock();
when(noiseBasedChunkGeneratorMock.generatorSettings()).thenReturn(mock());
if (vanillaTest) {
// These methods are currently only called when running vanilla tests
when(noiseBasedChunkGeneratorMock.createBiomes(any(),any(),any(),any(),any())).thenAnswer(i -> CompletableFuture.completedFuture(i.getArguments()[4]));
when(noiseBasedChunkGeneratorMock.fillFromNoise(any(),any(),any(),any(),any())).thenAnswer(i -> CompletableFuture.completedFuture(i.getArguments()[4]));
}

// Distance manager is responsible for updating chunk levels; we do this manually for testing
var distanceManagerMockedConstruction = Mockito.mockConstruction(ChunkMap.DistanceManager.class, withSettings().defaultAnswer(Answers.RETURNS_DEEP_STUBS));
// Server level
ServerLevel serverLevelMock = mock(Mockito.RETURNS_DEEP_STUBS);
when(serverLevelMock.getHeight()).thenReturn(384);
when(serverLevelMock.getSectionsCount()).thenReturn(24);
// This call MAGICALLY makes things not break, and we have no idea why
// Probably due to threading issues?
serverLevelMock.getServer().getWorldData().worldGenOptions().generateStructures();
// We seem to need an actual directory, not a mock
LevelStorageSource.LevelStorageAccess levelStorageAccessMock = mock(Mockito.RETURNS_DEEP_STUBS);
when(levelStorageAccessMock.getDimensionPath(any())).thenReturn(Files.createTempDirectory("cc_test"));
Expand Down Expand Up @@ -86,7 +95,7 @@ private CloseableReference<ServerChunkCache> createServerChunkCache() throws IOE
* Load all dependencies for a single chunk at a given status (note that that chunk will only reach the status below)
*/
public void singleChunkAllDependenciesForStatusVanilla(ChunkStatus status) throws Exception {
try(var serverChunkCacheRef = createServerChunkCache()) {
try(var serverChunkCacheRef = createServerChunkCache(true)) {
var serverChunkCache = serverChunkCacheRef.value();
var chunkMap = serverChunkCache.chunkMap;

Expand Down Expand Up @@ -133,7 +142,7 @@ public void testSingleChunkAllDependenciesForStatusVanilla(ChunkStatus status) t
* Load a single chunk at full status
*/
@Test public void singleFullChunkVanilla() throws Exception {
try(var serverChunkCacheRef = createServerChunkCache()) {
try(var serverChunkCacheRef = createServerChunkCache(true)) {
var serverChunkCache = serverChunkCacheRef.value();
var chunkMap = serverChunkCache.chunkMap;

Expand Down Expand Up @@ -170,7 +179,7 @@ public void testSingleChunkAllDependenciesForStatusVanilla(ChunkStatus status) t
* Load all dependencies for a single chunk at a given status (note that that chunk will only reach the status below)
*/
public void singleChunkAllDependenciesForStatus(ChunkStatus status) throws Exception {
try(var serverChunkCacheRef = createServerChunkCache()) {
try(var serverChunkCacheRef = createServerChunkCache(false)) {
var serverChunkCache = serverChunkCacheRef.value();
var chunkMap = serverChunkCache.chunkMap;

Expand Down Expand Up @@ -213,7 +222,7 @@ public void testSingleChunkAllDependenciesForStatus(ChunkStatus status) throws E
* Load a single chunk at full status
*/
@Test public void singleFullChunk() throws Exception {
try(var serverChunkCacheRef = createServerChunkCache()) {
try(var serverChunkCacheRef = createServerChunkCache(false)) {
var serverChunkCache = serverChunkCacheRef.value();
var chunkMap = serverChunkCache.chunkMap;

Expand Down Expand Up @@ -253,7 +262,7 @@ public void testSingleChunkAllDependenciesForStatus(ChunkStatus status) throws E
*/
@LongRunTest
@Test public void singleFullCube() throws Exception {
try(var serverChunkCacheRef = createServerChunkCache()) {
try(var serverChunkCacheRef = createServerChunkCache(false)) {
var serverChunkCache = serverChunkCacheRef.value();
var chunkMap = serverChunkCache.chunkMap;

Expand Down Expand Up @@ -292,11 +301,18 @@ public void testSingleChunkAllDependenciesForStatus(ChunkStatus status) throws E

var future = ((ChunkHolderTestAccess) centerHolder).invokeCc_GetOrScheduleFuture(ChunkStatus.FULL, chunkMap);

Map<CloPos, List<ChunkHolder>> chunksByCubeColumn = new HashMap<>();
List<ChunkHolder> cubes = new ArrayList<>();
while (!(future.isDone() || future.isCompletedExceptionally())) {
((ServerChunkCacheTestAccess) serverChunkCache).getMainThreadProcessor().pollTask();
assertChunkCubeLoadOrder(chunkMap, chunksByCubeColumn, cubes);
ServerChunkCache.MainThreadExecutor mainThreadProcessor = ((ServerChunkCacheTestAccess) serverChunkCache).getMainThreadProcessor();
mainThreadProcessor.pollTask();
System.out.println(mainThreadProcessor.getPendingTasksCount());
}
var either = future.get();
assertTrue(either.left().isPresent(), () -> "Full cube future Either should be successful, but was " + either.right().get());
assertTrue(either.left().get().getStatus().isOrAfter(ChunkStatus.FULL),
() -> "Cube should be at full status, but has status " + either.left().get().getStatus());
assertInstanceOf(LevelCube.class, either.left().get());
for (int sectionZ = 0; sectionZ < CubicConstants.DIAMETER_IN_SECTIONS; sectionZ++) {
for (int sectionX = 0; sectionX < CubicConstants.DIAMETER_IN_SECTIONS; sectionX++) {
Expand All @@ -308,6 +324,58 @@ public void testSingleChunkAllDependenciesForStatus(ChunkStatus status) throws E
);
}
}

assertChunkCubeLoadOrder(chunkMap, chunksByCubeColumn, cubes);
}
}

private static void assertChunkCubeLoadOrder(ChunkMap chunkMap, Map<CloPos, List<ChunkHolder>> chunksByCubeColumn, List<ChunkHolder> cubes) {
Long2ObjectLinkedOpenHashMap<ChunkHolder> visibleCloMap = ((ChunkMapTestAccess) chunkMap).visibleChunkMap();
chunksByCubeColumn.clear();
cubes.clear();

// collect cube and chunk holders
visibleCloMap.forEach((cloPosLong, cloHolder) -> {
CloPos cloPos = ((CubicChunkHolder) cloHolder).cc_getPos();
if (cloPos.isChunk()) {
chunksByCubeColumn.computeIfAbsent(cloPos.correspondingCubeCloPos(0), p -> new ArrayList<>())
.add(cloHolder);
} else {
cubes.add(cloHolder);
}
});

// For each cube assert that its chunks exist and are of sufficient status
cubes.forEach(cubeHolder -> {
CloPos cubeCloPos = ((CubicChunkHolder) cubeHolder).cc_getPos();
List<ChunkHolder> chunksInCubeColumn = chunksByCubeColumn.get(cubeCloPos.correspondingCubeCloPos(0));

chunksInCubeColumn.forEach(chunkHolder -> assertChunkHolderValidForCubeHolder(chunkHolder, cubeHolder));
});
}

public static void assertChunkHolderValidForCubeHolder(ChunkHolder chunkHolder, ChunkHolder cubeHolder) {
ChunkStatus cubeStatus = cubeHolder.getLastAvailableStatus();
ChunkStatus chunkStatus = chunkHolder.getLastAvailableStatus();

// if chunk status is null, cube status must also be null.
if (chunkStatus == null) {
assertNull(cubeStatus,
() -> String.format("Chunk (%s) has status null is lower than cube (%s) at status %s",
((CubicChunkHolder) chunkHolder).cc_getPos(), ((CubicChunkHolder) cubeHolder).cc_getPos(), cubeStatus)
);
return;
}

// if the cube status is null, any value for the chunk status is valid.
if (cubeStatus == null) {
return;
}

// Neither are null, assert that statuses are valid.
assertTrue(chunkStatus.isOrAfter(cubeStatus),
() -> String.format("Chunk (%s) at status %s is lower than cube %s at status %s",
((CubicChunkHolder) chunkHolder).cc_getPos(), chunkStatus, ((CubicChunkHolder) cubeHolder).cc_getPos(), cubeStatus)
);
}
}
4 changes: 4 additions & 0 deletions ...github/opencubicchunks/cubicchunks/mixin/test/common/server/level/ChunkMapTestAccess.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,20 @@

import com.mojang.datafixers.util.Either;
import io.github.opencubicchunks.cubicchunks.world.level.chunklike.CloAccess;
import it.unimi.dsi.fastutil.longs.Long2ObjectLinkedOpenHashMap;
import net.minecraft.server.level.ChunkHolder;
import net.minecraft.server.level.ChunkMap;
import net.minecraft.world.level.chunk.ChunkAccess;
import net.minecraft.world.level.chunk.ChunkStatus;
import org.spongepowered.asm.mixin.Dynamic;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.gen.Accessor;
import org.spongepowered.asm.mixin.gen.Invoker;

@Mixin(ChunkMap.class)
public interface ChunkMapTestAccess {
@Accessor("visibleChunkMap") Long2ObjectLinkedOpenHashMap<ChunkHolder> visibleChunkMap();

@Invoker @Nullable ChunkHolder invokeUpdateChunkScheduling(long chunkPos, int newLevel, @Nullable ChunkHolder holder, int oldLevel);

@Invoker CompletableFuture<Either<List<ChunkAccess>, ChunkHolder.ChunkLoadingFailure>> invokeGetChunkRangeFuture(ChunkHolder chunkHolder, int range, IntFunction<ChunkStatus> statusGetter);
Expand Down

0 comments on commit ed1de33

Please sign in to comment.