🐛 Mobs/NPCs not clearing when out of range

[cocosolos]

I affirm:

• I understand that if I do not agree to the following points by completing the checkboxes my issue will be ignored.
• I have read and understood the Contributing Guide and the Code of Conduct.
• I have searched existing issues to see if the issue has already been opened, and I have checked the commit log to see if the issue has been resolved since my server was last updated.

OS / platform the server is running (if known)

Windows/Linux

Branch affected by issue

base

Steps to reproduce

When a mob or npc moves out of your viewing range (50') the model doesn't despawn. You don't get any more updates, but the model stays where it was when it left your view until you get within range of the actual entity again. This only seems to apply when the mob/npc is the one to move out of your range, as they disappear properly if you move out of their range.

Cactrot Rapido (17244539) and Raminel (17719326) make this easy to test. Just target them and watch as they move 50' away from you.

Maybe something to do with SpawnNPCList and SpawnMOBList. Unknown if pet, trust, and PC are affected as well.

Expected behavior

Mobs/NPCs should disappear when they get out of your viewing range.

[WinterSolstice8]

Strange, not sure anything was modified there recently? Not yet at least

[StabbyCutyou]

That sounds somewhat like how I encountered a "ghost" version of a pc when trying to reproduce a different bug with party invites: #6746 (comment)

[cocosolos]

Strange, not sure anything was modified there recently? Not yet at least

Maybe #6713 but I have no idea, just the only think I see touching packet handling (idk if it's packet handling or spawn list handling that's messing up).

[cocosolos]

I built b638572 and 72170d4 and the issue does appear to have been introduced in #6713. Not sure what the issue is exactly though.

[zach2good]

Almost 100% caused by the logic in

https://github.com/LandSandBoat/server/pull/6713/files#diff-81491448fbf9d158d4cc9e7df3822956203b840795b53b030ee1be7f250dd7cfR408-R494

relating to copies of packets in PendingCharPackets, PendingPositionPacket, and PacketList

If a despawn packet is pending and is then updated with an update, it'll get "upgraded" to being an update and the despawn will never happen - to my understanding

[zach2good]

Not ideal, but I could revert PendingCharPackets and PendingPositionPacket to use raw ptrs, then there won't be any clones of packets kicking around

[zach2good]

The difference between the original:

            PendingPositionPacket = packet;
            PacketList.emplace_back(packet);

and the new:

            PendingPositionPacket = packet->copy();
            PacketList.emplace_back(std::move(packet));

Ownership is "handed" to PacketList, but in the new each get a copy and manage the lifetimes on their own

[cocosolos]

That's the spot I was looking at too but I'm not sure I fully understand the logic and I'm not super familiar with unique_ptr. I'm not sure if PendingPositionPacket is a problem as much as PendingCharPackets and PendingEntityPackets. PendingPositionPacket looks like it's just a single packet dealing with the chars self position. If I'm reading correctly for the other 2, a new packet is made and added to both PacketList and the pending list then in popPacket it pops the packet from PacketList and erases the one from pending. In map.cpp, in send_parse we use a copy of the packet list to send and then use erasePackets which calls popPacket. This all seems correct.

If we already have an existing packet for the entity we use updateWith. Is it possible we're sending a copy of the old version of the packet and at the same time updating the actual packet with the despawn info, then deleting that despawn packet after we send the copy of the old packet in send_parse? Thus deleting the despawn packet before it's sent?

[zach2good]

I've opened this: #6801

Which might help, but I haven't had time to test it apart from ensuring it compiles