Correct practice for clearing actor refs on level change?

[JPL]

In my mod, I use an "inventory stack"-like Thinker class to keep track of available items in the current level. When the player exits the level, I want to clear this and avoid any broken references to objects from another level. See this minimal example code and attached PK3 (same as below plus a MapInfo that sets the custom player class). The VM abort can be triggered by opening a level, then exiting to a new level and firing a weapon once.

This could be a bug but I wasn't sure what the intended usage/behavior is, or if I'm doing something in a really unwise way.

---

Code: Select all

version "2.5.0"

class TestPlayer : DoomPlayer
{
	Array<TestStack> stacks;
	
	override void PostBeginPlay()
	{
		// put all items in stacks
		ThinkerIterator ItemFinder = ThinkerIterator.Create("Inventory");
		Actor mo;
		while ( mo = Actor(ItemFinder.Next()) )
        {
			// if item type already has a stack, add it to that
			bool existingStack = false;
			for ( int i = 0; i < stacks.Size(); i++ )
			{
				TestStack stack = stacks[i];
				if ( stack.stackItems[0].GetClass() == mo.GetClass() )
				{
					stack.stackItems.Push(mo);
					existingStack = true;
					break;
				}
			}
			// if not, create a new stack
			if ( !(existingStack) )
			{
				TestStack newStack = new("TestStack").Init(mo);
				stacks.Push(newStack);
			}
        }
		Super.PostBeginPlay();
	}
	
	// nothing important about firing weapon here, just something to trigger log
	override void FireWeapon(State stat)
	{
		LogStacks();
		Super.FireWeapon(stat);
	}
	
	// actors in stacks won't exist in the new level, so tear down all stacks
	// BUT this doesn't work - why?
	override void OnDestroy()
	{
		Console.Printf("TestPlayer.OnDestroy");
		stacks.Clear();
		Super.OnDestroy();
	}
	
	void LogStacks()
	{
		Console.Printf("logging %i stacks:", stacks.Size());
		for ( int i = 0; i < stacks.Size(); i++ )
		{
			stacks[i].LogStack(i);
		}
	}
}

class TestStack : Thinker
{
	Array<Actor> stackItems;
	
	// constructor takes an item that will define stack's type
	TestStack Init(Actor item)
	{
		ChangeStatNum(STAT_INFO);
		stackItems.Push(item);
		return self;
	}
	
	// tear down stack list
	override void OnDestroy()
	{
		Console.Printf("TestStack %s OnDestroy", stackItems[0].GetClassName());
		stackItems.Clear();
		Super.OnDestroy();
	}
	
	void LogStack(int index)
	{
		String stackClass = stackItems[0].GetClassName();
		Console.Printf("stack %i: %i %s", index, stackItems.Size(), stackClass);
	}
}

[_mental_]

Before destruction of old player's actor its clone is created in the current level.
This happens after destruction of thinkers and before destruction of old player pawn.
For this reason stack array from TestPlayer objects survives map change.
Although it contains nulls because objects were already destroyed and garbage collected.
Both OnDestroy() overrides are rather pointless, everything will work in the same way without them.
In other words, this is not a problem of dynamic arrays, Clear() or any other method.

And the last but not least. Could you please stop calling VM abort a crash? This is very misleading but continue to happen regularly.
Crash is unhandled exception after which OS has nothing else to do except to terminate guilty process.
When script is trying to access null object, GZDoom handles this gracefully and displays corresponding error(s) in console.

[JPL]

Before destruction of old player's actor its clone is created in the current level.
This happens after destruction of thinkers and before destruction of old player pawn.
For this reason stack array from TestPlayer objects survives map change.
Although it contains nulls because objects were already destroyed and garbage collected.
Both OnDestroy() overrides are rather pointless, everything will work in the same way without them.
In other words, this is not a problem of dynamic arrays, Clear() or any other method.

I wasn't aware it created a copy of the player, is the level load process described anywhere?

I tried replacing the OnDestroy calls with a TestPlayer.BeginPlay override that clears the stacks list, but this didn't seem to change anything.

I added some logging and noticed the flow of operations was this:
(start a new game in MAP01)
TestPlayer.BeginPlay
TestPlayer.PostBeginPlay (stacks are created)
(gameplay begins)
(player fires weapon)
TestPlayer.LogStacks
(player exits level)
(new level MAP02 loads)
TestStacks.OnDestroy (does nothing, I just left in the logging)
TestPlayer.BeginPlay
TestPlayer.BeginPlay
TestPlayer.OnDestroy (does nothing, I just left in the logging)
TestPlayer.OnDestroy
(gameplay begins)
(player fires weapon)
VM Abort during TestPlayer.LogStacks

I can see that a new player is getting created before the old one is destroyed, but why do those two functions run twice? Regardless, I can't figure out why the stacks.Clear() in TestPlayer.BeginPlay doesn't remove the old actor references and prevent the VM abort.

And the last but not least. Could you please stop calling VM abort a crash? This is very misleading but continue to happen regularly.
Crash is unhandled exception after which OS has nothing else to do except to terminate guilty process.
When script is trying to access null object, GZDoom handles this gracefully and displays corresponding error(s) in console.

Okay, I've edited the OP with better language and will use this in the future.

[_mental_]

I wasn't aware it created a copy of the player, is this process described anywhere?

No, I doubt so. The code responsible for whole process is in G_FinishTravel() function.

I can see that a new player is getting created before the old one is destroyed, but why do those two functions run twice?

The same function, these lines to be precise.

I tried replacing the OnDestroy calls with a TestPlayer.BeginPlay override that clears the stacks list, but this didn't seem to change anything.

Regardless, I can't figure out why the stacks.Clear() in TestPlayer.BeginPlay doesn't remove the old actor references and prevent the VM abort.

In both invocations of TestPlayer.BeginPlay() stack is still empty. Its content (bunch of nulls in our case) is copied after exiting from overridden BeginPlay() method.

Player traveling from map to map is quite weird process indeed. I remember some questions about it and suggestions to improve it.
However simplifying this process is quite problematic. There is mostly no chance that it will be changed to be more comprehensible.

[JPL]

Hmm okay, thanks for the info. I was able to avoid the VM abort by moving the "create stacks" logic out of TestPlayer.PostBeginPlay into its own function that is called during TestPlayer.Tick when level.maptime == 0 (ie the first tick), just after a stacks.Clear(). This runs every map and seems safe so far.

My last question is, is there a method that runs on every map load, after the G_FinishTravel logic has finished, that is most appropriate for this kind of bookkeeping? Running on the first tick works but feels kinda hacky.

[_mental_]

You can override StaticEventHandler.PlayerEntered() method. It will be called right after G_FinishTravel() functions.