From edca69b7891c776cfa7751b1641ed6e18a1757cc Mon Sep 17 00:00:00 2001 From: James Seibel Date: Sat, 13 May 2023 11:08:22 -0500 Subject: [PATCH] Log QuadTree concurrency issues instead of throwing assertions --- .../core/generation/WorldGenerationQueue.java | 10 ++-- .../seibel/lod/core/render/LodQuadTree.java | 47 +++++++++++++++---- 2 files changed, 43 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/com/seibel/lod/core/generation/WorldGenerationQueue.java b/core/src/main/java/com/seibel/lod/core/generation/WorldGenerationQueue.java index 553aa7005..2f32639be 100644 --- a/core/src/main/java/com/seibel/lod/core/generation/WorldGenerationQueue.java +++ b/core/src/main/java/com/seibel/lod/core/generation/WorldGenerationQueue.java @@ -272,12 +272,13 @@ public class WorldGenerationQueue implements Closeable // remove the task we found, we are going to start it and don't want to run it multiple times + // TODO the setValue can fail if the user is moving and the task that was once in range is no longer in range WorldGenTask removedWorldGenTask = this.waitingTaskQuadTree.setValue(new DhSectionPos(closestTask.pos.detailLevel, closestTask.pos.x, closestTask.pos.z), null); - // removedWorldGenTask can be null // TODO when? + // do we need to modify this task to generate it? - if(canGeneratePos((byte) 0, closestTask.pos)) // TODO should 0 be replaced? + if(this.canGeneratePos((byte) 0, closestTask.pos)) // TODO should detail level 0 be replaced? { // detail level is correct for generation, start generation @@ -307,9 +308,6 @@ public class WorldGenerationQueue implements Closeable // detail level is too high (if the detail level was too low, the generator would've ignored the request), // split up the task - // make sure that we have a task to split up - LodUtil.assertTrue(closestTask == removedWorldGenTask); - // split up the task and add each one to the tree LinkedList> childFutures = new LinkedList<>(); @@ -325,7 +323,7 @@ public class WorldGenerationQueue implements Closeable CompletableFuture newFuture = new CompletableFuture<>(); childFutures.add(newFuture); - + WorldGenTask newGenTask = new WorldGenTask(new DhLodPos(childDhSectionPos.sectionDetailLevel, childDhSectionPos.sectionX, childDhSectionPos.sectionZ), childDhSectionPos.sectionDetailLevel, removedWorldGenTask.taskTracker, newFuture); this.waitingTaskQuadTree.setValue(new DhSectionPos(childDhSectionPos.sectionDetailLevel, childDhSectionPos.sectionX, childDhSectionPos.sectionZ), newGenTask); diff --git a/core/src/main/java/com/seibel/lod/core/render/LodQuadTree.java b/core/src/main/java/com/seibel/lod/core/render/LodQuadTree.java index cab414b39..16d998ddc 100644 --- a/core/src/main/java/com/seibel/lod/core/render/LodQuadTree.java +++ b/core/src/main/java/com/seibel/lod/core/render/LodQuadTree.java @@ -12,7 +12,9 @@ import com.seibel.lod.core.util.objects.quadTree.QuadNode; import com.seibel.lod.core.util.objects.quadTree.QuadTree; import org.apache.logging.log4j.Logger; +import java.util.HashSet; import java.util.Iterator; +import java.util.concurrent.ConcurrentHashMap; /** * This quadTree structure is our core data structure and holds @@ -27,6 +29,12 @@ public class LodQuadTree extends QuadTree implements AutoClose public final int blockRenderDistance; private final ILodRenderSourceProvider renderSourceProvider; + /** + * This holds every {@link DhSectionPos} that should be reloaded next tick.
+ * This is a {@link ConcurrentHashMap} because new sections can be added to this list via the world generator threads. + */ + private final ConcurrentHashMap sectionsToReload = new ConcurrentHashMap<>(); + private final IDhClientLevel level; //FIXME: Proper hierarchy to remove this reference! @@ -79,6 +87,23 @@ public class LodQuadTree extends QuadTree implements AutoClose } private void updateAllRenderSections(DhBlockPos2D playerPos) { + // reload any sections that need reloading + DhSectionPos[] reloadSectionArray = this.sectionsToReload.keySet().toArray(new DhSectionPos[0]); + this.sectionsToReload.clear(); + for (DhSectionPos pos : reloadSectionArray) + { + try + { + LodRenderSection renderSection = this.getValue(pos); + if (renderSection != null) + { + renderSection.reload(this.renderSourceProvider); + } + } + catch (IndexOutOfBoundsException e) { /* the section is now out of bounds, it doesn't need to be reloaded */ } + } + + // walk through each root node Iterator rootPosIterator = this.rootNodePosIterator(); while (rootPosIterator.hasNext()) @@ -91,7 +116,7 @@ public class LodQuadTree extends QuadTree implements AutoClose } QuadNode rootNode = this.getNode(rootPos); - recursivelyUpdateRenderSectionNode(playerPos, rootNode, rootNode, rootNode.sectionPos, false); + this.recursivelyUpdateRenderSectionNode(playerPos, rootNode, rootNode, rootNode.sectionPos, false); } } /** @return whether the current position is able to render (note: not if it IS rendering, just if it is ABLE to.) */ @@ -165,7 +190,7 @@ public class LodQuadTree extends QuadTree implements AutoClose } else { - // all child positions are loaded, disable this section and enable the children. + // all child positions are loaded, disable this section and enable its children. renderSection.disposeRenderData(); renderSection.disableRendering(); @@ -181,9 +206,12 @@ public class LodQuadTree extends QuadTree implements AutoClose boolean childSectionLoaded = this.recursivelyUpdateRenderSectionNode(playerPos, rootNode, childNode, childPos, parentRenderSectionIsEnabled); allChildrenSectionsAreLoaded = childSectionLoaded && allChildrenSectionsAreLoaded; } - // FIXME having world generation enabled in a pre-generated world that doesn't have any DH data can cause this to happen - // surprisingly reloadPos() doesn't appear to be the culprit, maybe there is an issue with reloading/changing the full data source? - LodUtil.assertTrue(allChildrenSectionsAreLoaded, "Potential QuadTree concurrency issue. All child sections should be enabled and ready to render."); + if (!allChildrenSectionsAreLoaded) + { + // FIXME having world generation enabled in a pre-generated world that doesn't have any DH data can cause this to happen + // surprisingly reloadPos() doesn't appear to be the culprit, maybe there is an issue with reloading/changing the full data source? + LOGGER.warn("Potential QuadTree concurrency issue. All child sections should be enabled and ready to render for pos: "+sectionPos); + } // this section is now being rendered via its children @@ -304,11 +332,14 @@ public class LodQuadTree extends QuadTree implements AutoClose */ public void reloadPos(DhSectionPos pos) { - LodRenderSection renderSection = this.getValue(pos); - if (renderSection != null) + if (pos == null) { - renderSection.reload(this.renderSourceProvider); + // shouldn't happen, but James saw it happen once, so this is here just in case + LOGGER.warn("reloadPos given a null pos."); + return; } + + this.sectionsToReload.put(pos, true); }