diff --git a/src/world/ChunkLockId.php b/src/world/ChunkLockId.php new file mode 100644 index 000000000..312b8c910 --- /dev/null +++ b/src/world/ChunkLockId.php @@ -0,0 +1,38 @@ +> Chunk::COORD_BIT_SIZE; $chunkZ = $z >> Chunk::COORD_BIT_SIZE; - if($this->isChunkLocked($chunkX, $chunkZ)){ - throw new WorldException("Terrain is locked for generation/population"); - } if($this->loadChunk($chunkX, $chunkZ) === null){ //current expected behaviour is to try to load the terrain synchronously throw new WorldException("Cannot set a block in un-generated terrain"); } $this->timings->setBlock->startTiming(); + $this->unlockChunk($chunkX, $chunkZ, null); + $oldBlock = $this->getBlockAt($x, $y, $z, true, false); $block = clone $block; @@ -2046,10 +2045,7 @@ class World implements ChunkManager{ public function setBiomeId(int $x, int $z, int $biomeId) : void{ $chunkX = $x >> Chunk::COORD_BIT_SIZE; $chunkZ = $z >> Chunk::COORD_BIT_SIZE; - if($this->isChunkLocked($chunkX, $chunkZ)){ - //the changes would be overwritten when the generation finishes - throw new WorldException("Chunk is currently locked for async generation/population"); - } + $this->unlockChunk($chunkX, $chunkZ, null); if(($chunk = $this->loadChunk($chunkX, $chunkZ)) !== null){ $chunk->setBiomeId($x & Chunk::COORD_MASK, $z & Chunk::COORD_MASK, $biomeId); }else{ @@ -2103,18 +2099,50 @@ class World implements ChunkManager{ return $result; } - public function lockChunk(int $chunkX, int $chunkZ) : void{ + /** + * Flags a chunk as locked, usually for async modification. + * + * This is an **advisory lock**. This means that the lock does **not** prevent the chunk from being modified on the + * main thread, such as by setBlock() or setBiomeId(). However, you can use it to detect when such modifications + * have taken place - unlockChunk() with the same lockID will fail and return false if this happens. + * + * This is used internally by the generation system to ensure that two PopulationTasks don't try to modify the same + * chunk at the same time. Generation will respect these locks and won't try to do generation of chunks over which + * a lock is held. + * + * WARNING: Be sure to release all locks once you're done with them, or you WILL have problems with terrain not + * being generated. + */ + public function lockChunk(int $chunkX, int $chunkZ, ChunkLockId $lockId) : void{ $chunkHash = World::chunkHash($chunkX, $chunkZ); if(isset($this->chunkLock[$chunkHash])){ throw new \InvalidArgumentException("Chunk $chunkX $chunkZ is already locked"); } - $this->chunkLock[$chunkHash] = true; + $this->chunkLock[$chunkHash] = $lockId; } - public function unlockChunk(int $chunkX, int $chunkZ) : void{ - unset($this->chunkLock[World::chunkHash($chunkX, $chunkZ)]); + /** + * Unlocks a chunk previously locked by lockChunk(). + * + * You must provide the same lockID as provided to lockChunk(). + * If a null lockID is given, any existing lock will be removed from the chunk, regardless of who owns it. + * + * Returns true if unlocking was successful, false otherwise. + */ + public function unlockChunk(int $chunkX, int $chunkZ, ?ChunkLockId $lockId) : bool{ + $chunkHash = World::chunkHash($chunkX, $chunkZ); + if(isset($this->chunkLock[$chunkHash]) && ($lockId === null || $this->chunkLock[$chunkHash] === $lockId)){ + unset($this->chunkLock[$chunkHash]); + return true; + } + return false; } + /** + * Returns whether anyone currently has a lock on the chunk at the given coordinates. + * You should check this to make sure that population tasks aren't currently modifying chunks that you want to use + * in async tasks. + */ public function isChunkLocked(int $chunkX, int $chunkZ) : bool{ return isset($this->chunkLock[World::chunkHash($chunkX, $chunkZ)]); } @@ -2814,16 +2842,18 @@ class World implements ChunkManager{ $this->chunkPopulationRequestMap[$index] = $resolver; } + $chunkPopulationLockId = new ChunkLockId(); + for($xx = -1; $xx <= 1; ++$xx){ for($zz = -1; $zz <= 1; ++$zz){ - $this->lockChunk($x + $xx, $z + $zz); + $this->lockChunk($x + $xx, $z + $zz, $chunkPopulationLockId); if($xx !== 0 || $zz !== 0){ //avoid registering it twice for the center chunk; we already did that above $this->registerChunkLoader($temporaryChunkLoader, $x + $xx, $z + $zz); } } } - $task = new PopulationTask($this, $x, $z, $chunk, $temporaryChunkLoader); + $task = new PopulationTask($this, $x, $z, $chunk, $temporaryChunkLoader, $chunkPopulationLockId); $workerId = $this->workerPool->selectWorker(); if(!isset($this->generatorRegisteredWorkers[$workerId])){ $this->registerGeneratorToWorker($workerId); @@ -2843,10 +2873,10 @@ class World implements ChunkManager{ } /** - * @param Chunk[] $adjacentChunks + * @param Chunk[] $adjacentChunks chunkHash => chunk * @phpstan-param array $adjacentChunks */ - public function generateChunkCallback(int $x, int $z, Chunk $chunk, array $adjacentChunks, ChunkLoader $temporaryChunkLoader) : void{ + public function generateChunkCallback(ChunkLockId $chunkLockId, int $x, int $z, Chunk $chunk, array $adjacentChunks, ChunkLoader $temporaryChunkLoader) : void{ Timings::$generationCallback->startTiming(); for($xx = -1; $xx <= 1; ++$xx){ @@ -2856,31 +2886,52 @@ class World implements ChunkManager{ } if(isset($this->chunkPopulationRequestMap[$index = World::chunkHash($x, $z)]) && isset($this->activeChunkPopulationTasks[$index])){ + $dirtyChunks = 0; for($xx = -1; $xx <= 1; ++$xx){ for($zz = -1; $zz <= 1; ++$zz){ - $this->unlockChunk($x + $xx, $z + $zz); + if(!$this->unlockChunk($x + $xx, $z + $zz, $chunkLockId)){ + $dirtyChunks++; + } } } + if($dirtyChunks === 0){ + $oldChunk = $this->loadChunk($x, $z); + $this->setChunk($x, $z, $chunk); - $oldChunk = $this->loadChunk($x, $z); - $this->setChunk($x, $z, $chunk); - - foreach($adjacentChunks as $adjacentChunkHash => $adjacentChunk){ - World::getXZ($adjacentChunkHash, $xAdjacentChunk, $zAdjacentChunk); - $this->setChunk($xAdjacentChunk, $zAdjacentChunk, $adjacentChunk); - } - - if(($oldChunk === null or !$oldChunk->isPopulated()) and $chunk->isPopulated()){ - (new ChunkPopulateEvent($this, $x, $z, $chunk))->call(); - - foreach($this->getChunkListeners($x, $z) as $listener){ - $listener->onChunkPopulated($x, $z, $chunk); + foreach($adjacentChunks as $adjacentChunkHash => $adjacentChunk){ + World::getXZ($adjacentChunkHash, $xAdjacentChunk, $zAdjacentChunk); + $this->setChunk($xAdjacentChunk, $zAdjacentChunk, $adjacentChunk); } + + if(($oldChunk === null or !$oldChunk->isPopulated()) and $chunk->isPopulated()){ + (new ChunkPopulateEvent($this, $x, $z, $chunk))->call(); + + foreach($this->getChunkListeners($x, $z) as $listener){ + $listener->onChunkPopulated($x, $z, $chunk); + } + } + }else{ + $this->logger->debug("Discarding population result for chunk x=$x,z=$z - terrain was modified on the main thread before async population completed"); } + + //This needs to be in this specific spot because user code might call back to orderChunkPopulation(). + //If it does, and finds the promise, and doesn't find an active task associated with it, it will schedule + //another PopulationTask. We don't want that because we're here processing the results. + //We can't remove the promise from the array before setting the chunks in the world because that would lead + //to the same problem. Therefore, it's necessary that this code be split into two if/else, with this in the + //middle. unset($this->activeChunkPopulationTasks[$index]); - $promise = $this->chunkPopulationRequestMap[$index]; - unset($this->chunkPopulationRequestMap[$index]); - $promise->resolve($chunk); + + if($dirtyChunks === 0){ + $promise = $this->chunkPopulationRequestMap[$index]; + unset($this->chunkPopulationRequestMap[$index]); + $promise->resolve($chunk); + }else{ + //request failed, stick it back on the queue + //we didn't resolve the promise or touch it in any way, so any fake chunk loaders are still valid and + //don't need to be added a second time. + $this->chunkPopulationRequestQueue->enqueue($index); + } $this->drainPopulationRequestQueue(); } diff --git a/src/world/generator/PopulationTask.php b/src/world/generator/PopulationTask.php index 19c0038fa..5a628248d 100644 --- a/src/world/generator/PopulationTask.php +++ b/src/world/generator/PopulationTask.php @@ -27,6 +27,7 @@ use pocketmine\data\bedrock\BiomeIds; use pocketmine\scheduler\AsyncTask; use pocketmine\utils\AssumptionFailedError; use pocketmine\world\ChunkLoader; +use pocketmine\world\ChunkLockId; use pocketmine\world\format\BiomeArray; use pocketmine\world\format\Chunk; use pocketmine\world\format\io\FastChunkSerializer; @@ -40,6 +41,7 @@ use function intdiv; class PopulationTask extends AsyncTask{ private const TLS_KEY_WORLD = "world"; private const TLS_KEY_CHUNK_LOADER = "chunkLoader"; + private const TLS_KEY_LOCK_ID = "chunkLockId"; /** @var int */ public $worldId; @@ -53,7 +55,7 @@ class PopulationTask extends AsyncTask{ private string $adjacentChunks; - public function __construct(World $world, int $chunkX, int $chunkZ, ?Chunk $chunk, ChunkLoader $temporaryChunkLoader){ + public function __construct(World $world, int $chunkX, int $chunkZ, ?Chunk $chunk, ChunkLoader $temporaryChunkLoader, ChunkLockId $chunkLockId){ $this->worldId = $world->getId(); $this->chunkX = $chunkX; $this->chunkZ = $chunkZ; @@ -66,6 +68,7 @@ class PopulationTask extends AsyncTask{ $this->storeLocal(self::TLS_KEY_WORLD, $world); $this->storeLocal(self::TLS_KEY_CHUNK_LOADER, $temporaryChunkLoader); + $this->storeLocal(self::TLS_KEY_LOCK_ID, $chunkLockId); } public function onRun() : void{ @@ -131,6 +134,8 @@ class PopulationTask extends AsyncTask{ $world = $this->fetchLocal(self::TLS_KEY_WORLD); /** @var ChunkLoader $temporaryChunkLoader */ $temporaryChunkLoader = $this->fetchLocal(self::TLS_KEY_CHUNK_LOADER); + /** @var ChunkLockId $lockId */ + $lockId = $this->fetchLocal(self::TLS_KEY_LOCK_ID); if($world->isLoaded()){ $chunk = $this->chunk !== null ? FastChunkSerializer::deserializeTerrain($this->chunk) : @@ -151,7 +156,7 @@ class PopulationTask extends AsyncTask{ } } - $world->generateChunkCallback($this->chunkX, $this->chunkZ, $chunk, $adjacentChunks, $temporaryChunkLoader); + $world->generateChunkCallback($lockId, $this->chunkX, $this->chunkZ, $chunk, $adjacentChunks, $temporaryChunkLoader); } } }