From: Daniel Karbach <daniel.karbach@localhorst.tv>
Date: Wed, 22 Jul 2015 15:32:55 +0000 (+0200)
Subject: made chunk neighbor linkage a little safer
X-Git-Url: https://git.localhorst.tv/?a=commitdiff_plain;h=46b18a88fdda816f3c2c547aba68b0a5ea7970f7;p=blank.git

made chunk neighbor linkage a little safer
---

diff --git a/TODO b/TODO
index 9d43155..7e52a23 100644
--- a/TODO
+++ b/TODO
@@ -76,10 +76,6 @@ chunk traversal
 	profiling indicates that this is not neccessary atm. maybe it will
 	when there's some more action in the world
 
-	I got a segfault (sadly in release mode, so no sensible trace, but
-	it was in ChunkLoader::Update). I suspect it has something to do
-	with how chunks are relinked after a near death experience.
-
 transparency (blocks and entities)
 
 	transparent blocks because awesome
diff --git a/src/world/Chunk.hpp b/src/world/Chunk.hpp
index 02f146e..a75d8e2 100644
--- a/src/world/Chunk.hpp
+++ b/src/world/Chunk.hpp
@@ -102,7 +102,6 @@ public:
 	const Chunk &GetNeighbor(Block::Face f) const noexcept { return *neighbor[f]; }
 	void ClearNeighbors() noexcept;
 	void Unlink() noexcept;
-	void Relink() noexcept;
 
 	// check which faces of a block at given index are obstructed (and therefore invisible)
 	Block::FaceSet Obstructed(const Pos &) const noexcept;
diff --git a/src/world/ChunkLoader.hpp b/src/world/ChunkLoader.hpp
index 98b352f..c9fb8aa 100644
--- a/src/world/ChunkLoader.hpp
+++ b/src/world/ChunkLoader.hpp
@@ -41,8 +41,14 @@ public:
 
 private:
 	Chunk &Generate(const Chunk::Pos &pos);
+	// link given chunk to all loaded neighbors
 	void Insert(Chunk &) noexcept;
-	void Remove(Chunk &) noexcept;
+	// remove a loaded chunk
+	// this unlinks it from its neighbors as well as moves it to the free list
+	// given iterator must point to a chunk from the loaded list
+	// returns an iterator to the chunk following the removed one
+	// in the loaded list (end for the last one)
+	std::list<Chunk>::iterator Remove(std::list<Chunk>::iterator) noexcept;
 
 private:
 	Chunk::Pos base;
diff --git a/src/world/chunk.cpp b/src/world/chunk.cpp
index 75f9c4f..ef3c5fc 100644
--- a/src/world/chunk.cpp
+++ b/src/world/chunk.cpp
@@ -295,23 +295,10 @@ void Chunk::SetNeighbor(Chunk &other) noexcept {
 }
 
 void Chunk::ClearNeighbors() noexcept {
-	for (int i = 0; i < Block::FACE_COUNT; ++i) {
-		neighbor[i] = nullptr;
-	}
-}
-
-void Chunk::Unlink() noexcept {
 	for (int face = 0; face < Block::FACE_COUNT; ++face) {
 		if (neighbor[face]) {
 			neighbor[face]->neighbor[Block::Opposite(Block::Face(face))] = nullptr;
-		}
-	}
-}
-
-void Chunk::Relink() noexcept {
-	for (int face = 0; face < Block::FACE_COUNT; ++face) {
-		if (neighbor[face]) {
-			neighbor[face]->neighbor[Block::Opposite(Block::Face(face))] = this;
+			neighbor[face] = nullptr;
 		}
 	}
 }
@@ -760,8 +747,15 @@ void ChunkLoader::Insert(Chunk &chunk) noexcept {
 	}
 }
 
-void ChunkLoader::Remove(Chunk &chunk) noexcept {
-	chunk.Unlink();
+std::list<Chunk>::iterator ChunkLoader::Remove(std::list<Chunk>::iterator chunk) noexcept {
+	// fetch next entry while chunk's still in the list
+	std::list<Chunk>::iterator next = chunk;
+	++next;
+	// unlink neighbors so they won't reference a dead chunk
+	chunk->ClearNeighbors();
+	// and move it from loaded to free list
+	to_free.splice(to_free.end(), loaded, chunk);
+	return next;
 }
 
 Chunk *ChunkLoader::Loaded(const Chunk::Pos &pos) noexcept {
@@ -818,10 +812,7 @@ void ChunkLoader::Rebase(const Chunk::Pos &new_base) {
 	// unload far away chunks
 	for (auto iter(loaded.begin()), end(loaded.end()); iter != end;) {
 		if (OutOfRange(*iter)) {
-			auto saved = iter;
-			Remove(*saved);
-			++iter;
-			to_free.splice(to_free.end(), loaded, saved);
+			iter = Remove(iter);
 		} else {
 			++iter;
 		}
@@ -844,27 +835,35 @@ void ChunkLoader::GenerateSurrounding(const Chunk::Pos &pos) {
 }
 
 void ChunkLoader::Update(int dt) {
+	// check if a chunk generation is scheduled for this frame
+	// and if there's a chunk waiting to be generated
 	gen_timer.Update(dt);
 	if (!gen_timer.Hit() || to_generate.empty()) {
 		return;
 	}
 
+	// take position of next chunk in queue
 	Chunk::Pos pos(to_generate.front());
 	to_generate.pop_front();
 
+	// look if the same chunk was already generated and still lingering
 	for (auto iter(to_free.begin()), end(to_free.end()); iter != end; ++iter) {
 		if (iter->Position() == pos) {
-			iter->Relink();
 			loaded.splice(loaded.end(), to_free, iter);
+			Insert(loaded.back());
+			return;
 		}
 	}
 
+	// if the free list is empty, allocate a new chunk
+	// otherwise clear an unused one
 	if (to_free.empty()) {
 		loaded.emplace_back(reg);
 	} else {
 		to_free.front().ClearNeighbors();
 		loaded.splice(loaded.end(), to_free, to_free.begin());
 	}
+
 	Chunk &chunk = loaded.back();
 	chunk.Position(pos);
 	gen(chunk);
diff --git a/tst/world/ChunkTest.cpp b/tst/world/ChunkTest.cpp
index 408fec3..6db245a 100644
--- a/tst/world/ChunkTest.cpp
+++ b/tst/world/ChunkTest.cpp
@@ -322,7 +322,6 @@ void ChunkTest::testNeighbor() {
 			"chunk did not link correct neighbor",
 			&*chunk, &neighbor->GetNeighbor(Block::Opposite(face))
 		);
-		chunk->Unlink();
 		chunk->ClearNeighbors();
 	}