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

Jump to bottom

feat(RelevanceSystem): addRelevanceEntity returns the relevant region #4902

Merged
merged 4 commits into from
Sep 29, 2021
Merged

feat(RelevanceSystem): addRelevanceEntity returns the relevant region #4902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
26ecfcd
feat(RelevanceSystem): addRelevanceEntity returns the relevant region
keturn Sep 10, 2021
4877b8c
chore(world.chunks): return ListenableFuture rather than Future
keturn Sep 10, 2021
da9640e
Merge branch 'develop' into chore/relevanceSystem
jdrueckert Sep 13, 2021
7508b92
Merge remote-tracking branch 'origin/develop' into chore/relevanceSystem
keturn Sep 25, 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
4 changes: 2 additions & 2 deletions ...c/main/java/org/terasology/engine/world/chunks/localChunkProvider/LocalChunkProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import com.google.common.collect.Iterators;
import com.google.common.collect.Lists;
import com.google.common.collect.Queues;
import com.google.common.util.concurrent.ListenableFuture;
import gnu.trove.list.TIntList;
import gnu.trove.list.array.TIntArrayList;
import gnu.trove.map.TShortObjectMap;
Expand Down Expand Up @@ -55,7 +56,6 @@
import java.util.Map;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.Future;
import java.util.function.Consumer;

/**
Expand Down Expand Up @@ -115,7 +115,7 @@ public LocalChunkProvider(StorageManager storageManager, EntityManager entityMan
}


protected Future<Chunk> createOrLoadChunk(Vector3ic chunkPos) {
protected ListenableFuture<Chunk> createOrLoadChunk(Vector3ic chunkPos) {
Vector3i pos = new Vector3i(chunkPos);
return loadingPipeline.invokeGeneratorTask(
pos,
Expand Down
22 changes: 13 additions & 9 deletions .../src/main/java/org/terasology/engine/world/chunks/localChunkProvider/RelevanceSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.terasology.engine.world.RelevanceRegionComponent;
import org.terasology.engine.world.WorldComponent;
import org.terasology.engine.world.block.BlockRegion;
import org.terasology.engine.world.block.BlockRegionc;
import org.terasology.engine.world.chunks.Chunk;
import org.terasology.engine.world.chunks.ChunkRegionListener;
import org.terasology.engine.world.chunks.event.BeforeChunkUnload;
Expand Down Expand Up @@ -144,20 +145,22 @@ private void updateRelevance() {
* Add entity to relevance system. create region for it. Update distance if region exists already. Create/Load
* chunks for region.
*
* @param entity entity to add.
* @param distance region's distance.
* @param listener chunk relevance listener.
* @param entity the region will be centered around the LocationComponent of this entity
* @param distance the dimensions of the region, in chunks
* @param listener notified when relevant chunks become available
*
* @return the region of chunks deemed relevant
*/
public void addRelevanceEntity(EntityRef entity, Vector3ic distance, ChunkRegionListener listener) {
public BlockRegionc addRelevanceEntity(EntityRef entity, Vector3ic distance, ChunkRegionListener listener) {
Copy link
Member

Choose a reason for hiding this comment

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

this function returns a new BlockRegion. do you want to change this to BlockRegion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see why you might ask. Not sure. I'll explain my thought process and you can let me know how it lines up with expected usage convention of the joml and block *c types:

ChunkRelevanceRegion is an internal class, and while its getCurrentRegion method is public, I don't know anything about whether its return value is supposed to immutable, or always a current view, or fully mutable. If it's not immutable, I didn't want to have to try to figure out how to document the state changes of this internal class to external users of MTE.

I figure if we return a new copy, it's much easier to explain. We know none of the internals are going to change anything about this new copy.

Now maybe you're thinking that because we made this new object explicitly for this method call, we might as well let the caller take ownership and do whatever they want with it, right? No need to use the *c interface to restrict the caller to a read-only view of it.

On the other hand, there's no reason to give them a mutable object. A person might expect that if they get back an object from RelevanceSystem with state-changing methods on it, that changing the state of it is supposed to actually change something. And it doesn't. That's why I went with BlockRegionc.

I think this is the sort of place that will use a Record class as a return value, when we upgrade to Java 17 and get those.

Copy link
Member

Choose a reason for hiding this comment

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

ooh, see yea I guess that makes sense. record class java 17? what is this record you speak of?

Copy link
Member Author

Choose a reason for hiding this comment

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

Records for immutable data.

if (!entity.exists()) {
return;
return null; // Futures.immediateFailedFuture(new IllegalArgumentException("Entity does not exist."));
}
regionLock.readLock().lock();
try {
ChunkRelevanceRegion region = regions.get(entity);
if (region != null) {
region.setRelevanceDistance(distance);
return;
return new BlockRegion(region.getCurrentRegion()); // Future of “when region.currentRegion is no longer dirty”?
}
} finally {
regionLock.readLock().unlock();
Expand All @@ -175,16 +178,17 @@ public void addRelevanceEntity(EntityRef entity, Vector3ic distance, ChunkRegion

StreamSupport.stream(region.getCurrentRegion().spliterator(), false)
.sorted(new PositionRelevanceComparator()) //<-- this is n^2 cost. not sure why this needs to be sorted like this.
.forEach(
pos -> {
.forEach(pos -> {
Chunk chunk = chunkProvider.getChunk(pos);
if (chunk != null) {
region.checkIfChunkIsRelevant(chunk);
// return Futures.immediateFuture(chunk);
} else {
chunkProvider.createOrLoadChunk(pos);
chunkProvider.createOrLoadChunk(pos); // return this
}
}
);
return new BlockRegion(region.getCurrentRegion()); // whenAllComplete
}

/**
Expand Down
3 changes: 2 additions & 1 deletion ...ne/src/main/java/org/terasology/engine/world/chunks/pipeline/ChunkProcessingPipeline.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.SettableFuture;
import org.joml.Vector3ic;
import org.slf4j.Logger;
Expand Down Expand Up @@ -221,7 +222,7 @@ public ChunkProcessingPipeline addStage(ChunkTaskProvider stage) {
* @param generatorTask ChunkTask which provides new chunk to pipeline
* @return Future of chunk processing.
*/
public Future<Chunk> invokeGeneratorTask(Vector3ic position, Supplier<Chunk> generatorTask) {
public ListenableFuture<Chunk> invokeGeneratorTask(Vector3ic position, Supplier<Chunk> generatorTask) {
Preconditions.checkState(!stages.isEmpty(), "ChunkProcessingPipeline must to have at least one stage");
ChunkProcessingInfo chunkProcessingInfo = chunkProcessingInfoMap.get(position);
if (chunkProcessingInfo != null) {
Expand Down