From 6fc256709c01fc7f08031b1ff14d86ccc24c1695 Mon Sep 17 00:00:00 2001
From: Christoph Oelckers <coelckers@users.noreply.github.com>
Date: Fri, 1 Nov 2024 08:46:02 +0100
Subject: [PATCH] fixed the particle replacement code.

OldestParticle was not properly tracked which could result in circular lists.
To make maintenance easier, the replacement code and the free particle part were merged into one to only have one place where the linked list is modified.
---
 src/playsim/p_effect.cpp | 83 ++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 50 deletions(-)

diff --git a/src/playsim/p_effect.cpp b/src/playsim/p_effect.cpp
index 459d267bef8..bdceae14969 100644
--- a/src/playsim/p_effect.cpp
+++ b/src/playsim/p_effect.cpp
@@ -106,50 +106,45 @@ static const struct ColorList {
 	{NULL, 0, 0, 0 }
 };
 
-inline particle_t *NewParticle (FLevelLocals *Level, bool replace = false)
+static void FreeParticle(FLevelLocals* Level, particle_t* particle)
 {
-	particle_t *result = nullptr;
-	// [MC] Thanks to RaveYard and randi for helping me with this addition.
-	// Array's filled up
-	if (Level->InactiveParticles == NO_PARTICLE)
+	auto prev = particle->tprev == NO_PARTICLE? nullptr : &Level->Particles[particle->tprev];
+	int pindex = (int)(particle - Level->Particles.Data());
+	auto tnext = particle->tnext;
+	assert(!prev || (prev->tnext == pindex));
+	if (prev)
+		prev->tnext = tnext;
+	else
+		Level->ActiveParticles = tnext;
+
+	if (tnext != NO_PARTICLE)
 	{
-		if (replace)
-		{
-			result = &Level->Particles[Level->OldestParticle];
+		particle_t* next = &Level->Particles[tnext];
+		assert(next->tprev == pindex);
+		next->tprev = particle->tprev;
+	}
+	if (Level->OldestParticle == pindex)
+	{
+		assert(tnext == NO_PARTICLE);
+		Level->OldestParticle = particle->tprev;
+	}
+	memset(particle, 0, sizeof(particle_t));
+	particle->tnext = Level->InactiveParticles;
+	Level->InactiveParticles = pindex;
+}
 
-			// There should be NO_PARTICLE for the oldest's tnext
-			if (result->tprev != NO_PARTICLE)
-			{
-				// tnext: youngest to oldest
-				// tprev: oldest to youngest
-				
-				// 2nd oldest -> oldest
-				particle_t *nbottom = &Level->Particles[result->tprev];
-				nbottom->tnext = NO_PARTICLE;
-
-				// now oldest becomes youngest
-				Level->OldestParticle = result->tprev;
-				result->tnext = Level->ActiveParticles;
-				result->tprev = NO_PARTICLE;
-				Level->ActiveParticles = uint32_t(result - Level->Particles.Data());
-
-				// youngest -> 2nd youngest
-				particle_t* ntop = &Level->Particles[result->tnext];
-				ntop->tprev = Level->ActiveParticles;
-			}
-			// [MC] Future proof this by resetting everything when replacing a particle.
-			auto tnext = result->tnext;
-			auto tprev = result->tprev;
-			*result = {};
-			result->tnext = tnext;
-			result->tprev = tprev;
-		}
-		return result;
+static particle_t *NewParticle (FLevelLocals *Level, bool replace = false)
+{
+	// Array's filled up
+	if (Level->InactiveParticles == NO_PARTICLE && Level->OldestParticle != NO_PARTICLE)
+	{
+		if (!replace) return nullptr;
+		FreeParticle(Level, &Level->Particles[Level->OldestParticle]);
 	}
 	
 	// Array isn't full.
 	uint32_t current = Level->ActiveParticles;
-	result = &Level->Particles[Level->InactiveParticles];
+	auto result = &Level->Particles[Level->InactiveParticles];
 	Level->InactiveParticles = result->tnext;
 	result->tnext = current;
 	result->tprev = NO_PARTICLE;
@@ -310,19 +305,7 @@ void P_ThinkParticles (FLevelLocals *Level)
 		particle->size += particle->sizestep;
 		if (particle->alpha <= 0 || --particle->ttl <= 0 || (particle->size <= 0))
 		{ // The particle has expired, so free it
-			*particle = {};
-			if (prev)
-				prev->tnext = i;
-			else
-				Level->ActiveParticles = i;
-
-			if (i != NO_PARTICLE)
-			{
-				particle_t *next = &Level->Particles[i];
-				next->tprev = particle->tprev;
-			}
-			particle->tnext = Level->InactiveParticles;
-			Level->InactiveParticles = (int)(particle - Level->Particles.Data());
+			FreeParticle(Level, particle);
 			continue;
 		}