[Fix] Fix crash from lingering spawn2 reference in NPCs.

[zimp-wow]

Description

In the case of a Zone::Repop() the spawn2 linked list would be cleared, destroying the spawn2 object. However a charmed NPC would survive the repop and continue to hold onto their reference to the spawn2 object, resulting in a variety of crashes.

I waffled around different ways to solve this and ultimately decided to remove the reference and replace it with a real-time lookup of the current spawn2 object in spawn2_list. I do have reservations about the performance cost of this, especially in the case of line 523 in entity.cpp. Might be a point for additional discussion.

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

Testing

Original Reproduction Steps:

1. Charm enemy (#cast 2932)
2. #repop
3. Kill charmed enemy or click off charm
4. #repop

Second repop usually crashes/locks up zone.

Test Cases:

1. Verify repro steps stop working (multiple attempts) - Pass
2. Verify npc respawn timers are correctly set still on death - Pass
3. Verify gm commands still function - Pass

Checklist

• I have tested my changes
• I have performed a self-review of my code. Ensuring variables, functions and methods are named in a human-readable way, comments are added only where naming of variables, functions and methods can't give enough context.
• I own the changes of my code and take responsibility for the potential issues that occur

[Akkadius]

This is gonna be a big concern in a hot path hitting the entity list in your ternary

[Akkadius]

Looking at this I'd prefer that we properly book keep at the event points where references become inconsistent rather than doing expensive calculations realtime.

Doing entity list iterations in the main process loop is going chew through cycles like crazy needlessly. I'd rather us true up data when it goes wrong. (ie repop)

Thank you for putting this up <3

[zimp-wow]

My main concern with trying to retain the reference is it felt like a landmine for future regressions. It's a pretty messy problem when it manifests too as tracing it backwards from the stack traces was a lot of false leads.

I could probably cut down on a lot of the processing cost in that loop by wrapping the call in a closure that only gets called when the conditional below gets that far. That would limit the check to only trigger in zones with no clients in them.

Or another approach would be to use a more efficient structure to search against, be it a hash or an ordered list.

Feels like for long term maintainability it would be easier to just solve the performance cost than try to track ownership and validity of the spawn2 reference.