Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: order chunks on demand #4773

Closed
wants to merge 15 commits into from
Closed

feat: order chunks on demand #4773

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
29cce78
feat: order chunks on-demand during generation
naalit Jun 16, 2021
fbc652a
clean up unused imports
naalit Jun 16, 2021
f637a98
remove future from ChunkProcessingInfo
naalit Jun 16, 2021
1907954
fix engine tests
naalit Jun 17, 2021
a037898
Merge branch 'develop' into feat/order-chunks-on-demand
naalit Jun 17, 2021
1f6db11
don't interrupt chunk threads on pipeline shutdown
naalit Jun 17, 2021
c0aefbe
add documentation for new items
naalit Jun 17, 2021
d890029
enable multithreaded chunk generation
naalit Jun 17, 2021
a3cd2ea
clean up code based on feedback
naalit Jun 18, 2021
a67f4a1
add condition for waiting until generation finishes, for tests
naalit Jun 18, 2021
58ed505
fix java 8 build on Jenkins
naalit Jun 18, 2021
52ea11f
Merge branch 'develop' into feat/order-chunks-on-demand
skaldarnar Jun 20, 2021
87e8511
Merge branch 'develop' of github.com:MovingBlocks/Terasology into fea…
pollend Jun 20, 2021
d738be2
Merge branch 'feat/order-chunks-on-demand' of github.com:MovingBlocks…
pollend Jun 20, 2021
a229fea
Merge branch 'develop' into feat/order-chunks-on-demand
pollend Jun 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 33 additions & 24 deletions ...st/java/org/terasology/engine/world/chunks/localChunkProvider/LocalChunkProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package org.terasology.engine.world.chunks.localChunkProvider;

import com.google.common.collect.Maps;
import org.joml.Vector3f;
import org.joml.Vector3i;
import org.joml.Vector3ic;
import org.junit.jupiter.api.AfterEach;
Expand All @@ -13,11 +14,11 @@
import org.terasology.engine.entitySystem.entity.EntityManager;
import org.terasology.engine.entitySystem.entity.EntityRef;
import org.terasology.engine.entitySystem.event.Event;
import org.terasology.engine.logic.location.LocationComponent;
import org.terasology.engine.world.BlockEntityRegistry;
import org.terasology.engine.world.block.BeforeDeactivateBlocks;
import org.terasology.engine.world.block.Block;
import org.terasology.engine.world.block.BlockManager;
import org.terasology.engine.world.block.BlockRegion;
import org.terasology.engine.world.block.OnActivatedBlocks;
import org.terasology.engine.world.block.OnAddedBlocks;
import org.terasology.engine.world.chunks.Chunk;
Expand All @@ -36,17 +37,16 @@
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;

import static org.mockito.Mockito.atLeast;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

class LocalChunkProviderTest {

private static final int WAIT_CHUNK_IS_READY_IN_SECONDS = 30;
private static final int WAIT_CHUNK_IS_READY_IN_SECONDS = 5;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a way more reasonable time to wait for chunks to be ready 👍


private LocalChunkProvider chunkProvider;
private EntityManager entityManager;
Expand All @@ -58,6 +58,8 @@ class LocalChunkProviderTest {
private Block blockAtBlockManager;
private TestStorageManager storageManager;
private TestWorldGenerator generator;
private RelevanceSystem relevanceSystem;
private EntityRef playerEntity;

@BeforeEach
public void setUp() {
Expand All @@ -81,36 +83,33 @@ public void setUp() {
chunkCache);
chunkProvider.setBlockEntityRegistry(blockEntityRegistry);
chunkProvider.setWorldEntity(worldEntity);
chunkProvider.setRelevanceSystem(new RelevanceSystem(chunkProvider)); // workaround. initialize loading pipeline
relevanceSystem = new RelevanceSystem(chunkProvider);
chunkProvider.setRelevanceSystem(relevanceSystem); // workaround. initialize loading pipeline
}

@AfterEach
public void tearDown() {
chunkProvider.shutdown();
}

private Future<Chunk> requestCreatingOrLoadingArea(Vector3ic chunkPosition, int radius) {
Future<Chunk> chunkFuture = chunkProvider.createOrLoadChunk(chunkPosition);
BlockRegion extentsRegion = new BlockRegion(
chunkPosition.x() - radius, chunkPosition.y() - radius, chunkPosition.z() - radius,
chunkPosition.x() + radius, chunkPosition.y() + radius, chunkPosition.z() + radius);

extentsRegion.iterator().forEachRemaining(pos -> {
if (!pos.equals(chunkPosition)) { // remove center. we takes future for it already.
chunkProvider.createOrLoadChunk(pos);
}
});
return chunkFuture;
private void requestCreatingOrLoadingArea(Vector3ic chunkPosition, int radius) {
playerEntity = mock(EntityRef.class);
when(playerEntity.exists()).thenReturn(true);
when(playerEntity.getComponent(LocationComponent.class)).thenReturn(new LocationComponent(new Vector3f(chunkPosition)));
Vector3i distance = new Vector3i(radius * 2, radius * 2, radius * 2);
relevanceSystem.addRelevanceEntity(playerEntity, distance, null);
}

private Future<Chunk> requestCreatingOrLoadingArea(Vector3ic chunkPosition) {
return requestCreatingOrLoadingArea(chunkPosition, 1);
private void requestCreatingOrLoadingArea(Vector3ic chunkPosition) {
requestCreatingOrLoadingArea(chunkPosition, 1);
}

@Test
void testGenerateSingleChunk() throws InterruptedException, ExecutionException, TimeoutException {
Vector3i chunkPosition = new Vector3i(0, 0, 0);
requestCreatingOrLoadingArea(chunkPosition).get(WAIT_CHUNK_IS_READY_IN_SECONDS, TimeUnit.SECONDS);
requestCreatingOrLoadingArea(chunkPosition);
chunkProvider.notifyRelevanceChanged();
Thread.sleep(WAIT_CHUNK_IS_READY_IN_SECONDS * 1000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll want to find a way to write these tests without Thread.sleep.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference between future.get(WAIT_…) and Thread.sleep(WAIT_…) is that the get will take at most that much time to execute, but the sleep will take at least that much time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's annoying that they take so long now. Unfortunately there aren't any futures to wait on anymore, but I guess we could add an object to wait on in the ChunkProcessingPipeline, or use a loop.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to ask about that: Why did you remove the futures?

My guess would have been because you only call the methods involved from a thread dedicated to doing that work, so they're all blocking and don't return until the result is available. But the number of times the tests rely on Thread.sleep suggests that's not the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, there's just no need for futures currently. Everything is just scheduled for after the chunk is generated via stages. Futures are only needed in the tests, because the rest of the code just submits chunks for generation asynchronously.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added another object which is notified when generation finishes, which fixes the tests.

chunkProvider.update();

final ArgumentCaptor<Event> eventArgumentCaptor = ArgumentCaptor.forClass(Event.class);
Expand Down Expand Up @@ -139,7 +138,9 @@ void testGenerateSingleChunkWithBlockLifeCycle() throws InterruptedException, Ex
Vector3i chunkPosition = new Vector3i(0, 0, 0);
blockAtBlockManager.setLifecycleEventsRequired(true);
blockAtBlockManager.setEntity(mock(EntityRef.class));
requestCreatingOrLoadingArea(chunkPosition).get(WAIT_CHUNK_IS_READY_IN_SECONDS, TimeUnit.SECONDS);
requestCreatingOrLoadingArea(chunkPosition);
chunkProvider.notifyRelevanceChanged();
Thread.sleep(WAIT_CHUNK_IS_READY_IN_SECONDS * 1000);
chunkProvider.update();

final ArgumentCaptor<Event> worldEventCaptor = ArgumentCaptor.forClass(Event.class);
Expand Down Expand Up @@ -181,7 +182,9 @@ void testLoadSingleChunk() throws InterruptedException, ExecutionException, Time
generator.createChunk(chunk, null);
storageManager.add(chunk);

requestCreatingOrLoadingArea(chunkPosition).get(WAIT_CHUNK_IS_READY_IN_SECONDS, TimeUnit.SECONDS);
requestCreatingOrLoadingArea(chunkPosition);
chunkProvider.notifyRelevanceChanged();
Thread.sleep(WAIT_CHUNK_IS_READY_IN_SECONDS * 1000);
chunkProvider.update();

Assertions.assertTrue(((TestChunkStore) storageManager.loadChunkStore(chunkPosition)).isEntityRestored(),
Expand All @@ -206,7 +209,9 @@ void testLoadSingleChunkWithBlockLifecycle() throws InterruptedException, Execut
blockAtBlockManager.setLifecycleEventsRequired(true);
blockAtBlockManager.setEntity(mock(EntityRef.class));

requestCreatingOrLoadingArea(chunkPosition).get(WAIT_CHUNK_IS_READY_IN_SECONDS, TimeUnit.SECONDS);
requestCreatingOrLoadingArea(chunkPosition);
chunkProvider.notifyRelevanceChanged();
Thread.sleep(WAIT_CHUNK_IS_READY_IN_SECONDS * 1000);
chunkProvider.update();

Assertions.assertTrue(((TestChunkStore) storageManager.loadChunkStore(chunkPosition)).isEntityRestored(),
Expand Down Expand Up @@ -249,7 +254,11 @@ void testUnloadChunkAndDeactivationBlock() throws InterruptedException, TimeoutE
blockAtBlockManager.setLifecycleEventsRequired(true);
blockAtBlockManager.setEntity(mock(EntityRef.class));

requestCreatingOrLoadingArea(chunkPosition).get(WAIT_CHUNK_IS_READY_IN_SECONDS, TimeUnit.SECONDS);
requestCreatingOrLoadingArea(chunkPosition);
chunkProvider.notifyRelevanceChanged();
Thread.sleep(WAIT_CHUNK_IS_READY_IN_SECONDS * 1000);
relevanceSystem.removeRelevanceEntity(playerEntity);
chunkProvider.notifyRelevanceChanged();

//Wait BeforeDeactivateBlocks event
Assertions.assertTimeoutPreemptively(Duration.of(WAIT_CHUNK_IS_READY_IN_SECONDS, ChronoUnit.SECONDS),
Expand Down
103 changes: 56 additions & 47 deletions ...rc/test/java/org/terasology/engine/world/chunks/pipeline/ChunkProcessingPipelineTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

package org.terasology.engine.world.chunks.pipeline;

import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import org.joml.Vector3i;
Expand All @@ -12,7 +13,6 @@
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
import org.terasology.gestalt.assets.management.AssetManager;
import org.terasology.engine.TerasologyTestingEnvironment;
import org.terasology.engine.registry.CoreRegistry;
import org.terasology.engine.world.block.BlockManager;
Expand All @@ -22,22 +22,23 @@
import org.terasology.engine.world.chunks.blockdata.ExtraBlockDataManager;
import org.terasology.engine.world.chunks.internal.ChunkImpl;
import org.terasology.engine.world.chunks.pipeline.stages.ChunkTaskProvider;
import org.terasology.engine.world.chunks.remoteChunkProvider.ReceivedChunkInitialProvider;
import org.terasology.gestalt.assets.management.AssetManager;

import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Random;
import java.util.Set;
import java.util.concurrent.CancellationException;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.Stream;

@TestInstance(TestInstance.Lifecycle.PER_METHOD)
@Tag("TteTest")
Expand All @@ -50,23 +51,30 @@ class ChunkProcessingPipelineTest extends TerasologyTestingEnvironment {

@Test
void simpleProcessingSuccess() throws ExecutionException, InterruptedException, TimeoutException {
pipeline = new ChunkProcessingPipeline((p) -> null, (o1, o2) -> 0);
ReceivedChunkInitialProvider initialProvider = new ReceivedChunkInitialProvider((o1, o2) -> 0);
pipeline = new ChunkProcessingPipeline((p) -> null, initialProvider);

Vector3i chunkPos = new Vector3i(0, 0, 0);
Chunk chunk = createChunkAt(chunkPos);

pipeline.addStage(ChunkTaskProvider.create("dummy task", (c) -> c));
List<Chunk> result = new ArrayList<>();
pipeline.addStage(ChunkTaskProvider.create("Chunk ready", (Consumer<Chunk>) result::add));

initialProvider.submit(chunk);
pipeline.start();

Future<Chunk> chunkFuture = pipeline.invokeGeneratorTask(new Vector3i(0, 0, 0), () -> chunk);
Chunk chunkAfterProcessing = chunkFuture.get(1, TimeUnit.SECONDS);
Thread.sleep(1000);
Chunk chunkAfterProcessing = result.get(0);

Assertions.assertEquals(chunkAfterProcessing.getPosition(new Vector3i()), chunk.getPosition(new Vector3i()),
Assertions.assertEquals(chunkAfterProcessing.getPosition(), chunk.getPosition(),
"Chunk after processing must have equals position, probably pipeline lost you chunk");
}

@Test
void simpleStopProcessingSuccess() {
pipeline = new ChunkProcessingPipeline((p) -> null, (o1, o2) -> 0);
ReceivedChunkInitialProvider initialProvider = new ReceivedChunkInitialProvider((o1, o2) -> 0);
pipeline = new ChunkProcessingPipeline((p) -> null, initialProvider);

Vector3i position = new Vector3i(0, 0, 0);
Chunk chunk = createChunkAt(position);
Expand All @@ -80,13 +88,10 @@ void simpleStopProcessingSuccess() {
return c;
}));

Future<Chunk> chunkFuture = pipeline.invokeGeneratorTask(position, () -> chunk);
initialProvider.submit(chunk);
pipeline.stopProcessingAt(position);
Assertions.assertThrows(
CancellationException.class,
() -> chunkFuture.get(1, TimeUnit.SECONDS),
"chunkFuture must be cancelled, when processing stopped"
);

Assertions.assertFalse(pipeline.isPositionProcessing(position));
}

/**
Expand All @@ -105,20 +110,26 @@ void multiRequirementsChunksExistsSuccess() throws ExecutionException, Interrupt
Function.identity()
));

pipeline = new ChunkProcessingPipeline(chunkCache::get, (o1, o2) -> 0);
ReceivedChunkInitialProvider initialProvider = new ReceivedChunkInitialProvider((o1, o2) -> 0);
pipeline = new ChunkProcessingPipeline(chunkCache::get, initialProvider);
pipeline.addStage(ChunkTaskProvider.createMulti(
"flat merging task",
(chunks) -> chunks.stream()
.filter((c) -> c.getPosition().equals(positionToGenerate))
.findFirst() // return central chunk.
.get(),
this::getNearChunkPositions));
List<Chunk> result = new ArrayList<>();
pipeline.addStage(ChunkTaskProvider.create("Chunk ready", (Consumer<Chunk>) result::add));

Chunk chunk = createChunkAt(positionToGenerate);
Future<Chunk> chunkFuture = pipeline.invokeGeneratorTask(new Vector3i(0, 0, 0), () -> chunk);
Chunk chunkAfterProcessing = chunkFuture.get(1, TimeUnit.SECONDS);
initialProvider.submit(chunk);
pipeline.start();

Thread.sleep(1000);
Chunk chunkAfterProcessing = result.get(0);

Assertions.assertEquals(chunkAfterProcessing.getPosition(new Vector3i()), chunk.getPosition(new Vector3i()),
Assertions.assertEquals(chunkAfterProcessing.getPosition(), chunk.getPosition(),
"Chunk after processing must have equals position, probably pipeline lost you chunk");
}

Expand All @@ -139,42 +150,47 @@ void multiRequirementsChunksWillGeneratedSuccess() throws ExecutionException, In
Function.identity()
));

pipeline = new ChunkProcessingPipeline((p) -> null, (o1, o2) -> 0);
ReceivedChunkInitialProvider initialProvider = new ReceivedChunkInitialProvider((o1, o2) -> 0);
pipeline = new ChunkProcessingPipeline((p) -> null, initialProvider);
pipeline.addStage(ChunkTaskProvider.createMulti(
"flat merging task",
(chunks) -> chunks.stream()
.filter((c) -> c.getPosition().equals(positionToGenerate)).findFirst() // return central chunk.
.get(),
this::getNearChunkPositions));
List<Chunk> result = new ArrayList<>();
pipeline.addStage(ChunkTaskProvider.create("Chunk ready", (Consumer<Chunk>) result::add));

Chunk chunk = createChunkAt(positionToGenerate);
Future<Chunk> chunkFuture = pipeline.invokeGeneratorTask(new Vector3i(0, 0, 0), () -> chunk);
initialProvider.submit(chunk);
pipeline.start();

Thread.sleep(1_000); // sleep 1 second. and check future.
Assertions.assertFalse(chunkFuture.isDone(), "Chunk must be not generated, because ChunkTask have not exists " +
"neighbors in requirements");
Thread.sleep(1_000); // sleep 1 second, and check result.
Assertions.assertTrue(result.isEmpty(), "Chunk must be not generated, because the ChunkTask's requirements don't exist");

chunkToGenerate.forEach((position, neighborChunk) -> pipeline.invokeGeneratorTask(position,
() -> neighborChunk));
chunkToGenerate.forEach((position, neighborChunk) -> initialProvider.submit(neighborChunk));
pipeline.notifyUpdate();

Chunk chunkAfterProcessing = chunkFuture.get(1, TimeUnit.SECONDS);
Thread.sleep(1000);
Assertions.assertEquals(result.size(), 1, "Only one chunk has its requirements fulfilled");
Chunk chunkAfterProcessing = result.get(0);

Assertions.assertEquals(chunkAfterProcessing.getPosition(new Vector3i()), chunk.getPosition(new Vector3i()),
Assertions.assertEquals(chunkAfterProcessing.getPosition(), chunk.getPosition(),
"Chunk after processing must have equals position, probably pipeline lost you chunk");
}

@Test
void emulateEntityMoving() throws InterruptedException {
final AtomicReference<Vector3ic> position = new AtomicReference<>();
Map<Vector3ic, Future<Chunk>> futures = Maps.newHashMap();
Map<Vector3ic, Chunk> chunkCache = Maps.newConcurrentMap();
pipeline = new ChunkProcessingPipeline(chunkCache::get, (o1, o2) -> {
ReceivedChunkInitialProvider initialProvider = new ReceivedChunkInitialProvider((o1, o2) -> {
if (position.get() != null) {
Vector3ic entityPos = position.get();
return (int) (entityPos.distance(((PositionFuture<?>) o1).getPosition()) - entityPos.distance(((PositionFuture<?>) o2).getPosition()));
return (int) (entityPos.distance(o1.getPosition()) - entityPos.distance(o2.getPosition()));
}
return 0;
});
pipeline = new ChunkProcessingPipeline(chunkCache::get, initialProvider);
pipeline.addStage(ChunkTaskProvider.createMulti(
"flat merging task",
(chunks) -> chunks.stream()
Expand All @@ -188,19 +204,17 @@ void emulateEntityMoving() throws InterruptedException {
this::getNearChunkPositions));
pipeline.addStage(ChunkTaskProvider.create("finish chunk", (c) -> {
c.markReady();
chunkCache.put(c.getPosition(new Vector3i()), c);
chunkCache.put(c.getPosition(), c);
}));

pipeline.start();

Set<Vector3ic> relativeRegion = Collections.emptySet();
for (int i = 0; i < 10; i++) {
position.set(new Vector3i(i, 0, 0));
Set<Vector3ic> newRegion = getNearChunkPositions(position.get(), 10);
Sets.difference(newRegion, relativeRegion).forEach(// load new chunks.
(pos) -> {
Future<Chunk> future = pipeline.invokeGeneratorTask(new Vector3i(pos),
() -> createChunkAt(pos));
futures.put(pos, future);
initialProvider.submit(createChunkAt(pos));
}
);

Expand All @@ -212,23 +226,18 @@ void emulateEntityMoving() throws InterruptedException {
}
}
);

pipeline.notifyUpdate();

relativeRegion = newRegion;

Assertions.assertTrue(Sets.difference(chunkCache.keySet(), relativeRegion).isEmpty(), "We must haven't " +
"chunks not related to relativeRegion");
Assertions.assertTrue(Sets.difference(Sets.newHashSet(pipeline.getProcessingPosition()), relativeRegion).isEmpty(),
"We must haven't chunks in processing not related to relativeRegion");

Stream<Future<Chunk>> relativeFutures = relativeRegion.stream().map(futures::get);
Assertions.assertTrue(
relativeFutures.noneMatch(Future::isCancelled),
"Relative futures must be not cancelled");

Stream<Future<Chunk>> nonRelativeFutures =
Sets.difference(futures.keySet(), relativeRegion).stream().map(futures::get);
Assertions.assertTrue(
nonRelativeFutures.allMatch((f) -> f.isCancelled() || f.isDone()),
"Non relative futures must be cancelled or done");
Assertions.assertTrue(relativeRegion.containsAll(Lists.newArrayList(pipeline.getProcessingPosition())),
"No non-relative chunks should be processing");

Thread.sleep(new Random().nextInt(500)); //think time
}
Expand Down
Loading