Use PacketList directly when sending#6831
Conversation
zach2good
left a comment
zach2good
left a comment
There was a problem hiding this comment.
Logically seems fine, I'd just like it to be made as clear and obvious about what's going on at every step as possible
src/map/entities/charentity.cpp
Outdated
| { | ||
| auto existing = PendingCharPackets.find(PChar->id); | ||
| if (existing == PendingCharPackets.end()) | ||
| if (existing == PendingCharPackets.end() || !existing->second) |
There was a problem hiding this comment.
I find this logical block quite hard to reason about, can you break these up even if it ends up with nested conditions?
I think it would also help to rename existing to itr, and break out conditions like:
const bool hasPendingPacket = itr != PendingCharPackets.end();
and to not use the itr pair directly, but to use:
auto& pendingPacket = existing->second;
pendingPacket ->updateWith...
There was a problem hiding this comment.
Updated the logic here, hopefully more clear.
src/map/entities/charentity.h
Outdated
|
|
||
| // TODO: Don't use raw ptrs for this, but don't duplicate whole packets with unique_ptr either. | ||
| CBasicPacket* PendingPositionPacket = nullptr; | ||
| std::deque<std::unique_ptr<CBasicPacket>> PacketList; // The list of packets to be sent to the character during the next network cycle |
There was a problem hiding this comment.
instead of making this public, can you keep it private and access it with: auto getPacketList() -> std::deque<std::unique_ptr<CBasicPacket>>&
There was a problem hiding this comment.
Good call, made it const too.
src/map/map.cpp
Outdated
| packets = 0; | ||
|
|
||
| while (!packetList.empty() && *buffsize + packetList.front()->getSize() < MAX_BUFFER_SIZE && static_cast<size_t>(packets) < PacketCount) | ||
| while (!PChar->PacketList.empty() && *buffsize + PChar->PacketList.front()->getSize() < MAX_BUFFER_SIZE && static_cast<size_t>(packets) < PacketCount) |
|
Please stop commenting on closed PRs, it's very hard for us to track. Open new issues and link prs you want to talk about |
2 participants
I affirm:
What does this pull request do?
Couple edge case fixes if a pending packet ends up being null. Mainly though this makes it so we use the PacketList directly when preparing packets to send instead of making a copy and using that. We currently make a copy of the packet list, process those copies, then delete the real packets. This apparently causes some sync issues (#6829) that should be fixed now.
I'm not sure why exactly this issue seems to be consistent, so this may require some further review, but this seems to fix it.
Steps to test these changes
Log in with 2 characters, go to the same zone, both characters can see each other.