Improving the PacketQue / Removing RefreshQueue

[giwo]

The RefreshQueue was a good idea, and I feel it has serviced UOX3 well.

However it is now my opinion that this step was too little towards what needs to be done.

It is my belief that we need ALL packets to be a part of a single que. This que is populated by a call to a function that would look something like: QueuePacket( UI16 packetNum, void *extInfo, CSocket *sendTo );

My thoughts on how this would work are as follows.

Code: Select all

std::map< void *object, CPacketQueue > packetQueue;

class CPacketQueue
{
	std::map< UI16 packet, CSocket *sendTo > sendList;
};

From there at the end of our checkTimers loop, we would do this:

Code: Select all

std::map< void *object, CPacketQueue >::const_iterator pQIter = packetQueue.begin();
std::map< void *object, CPacketQueue >::const_iterator pQEnd = packetQueue.end();

while( pQIter != pQEnd )
{
	std::map< UI16 packet, CSocket *sendTo >::const_iterator sLIter = pQIter->second.sendList.begin();
	std::map< UI16 packet, CSocket *sendTo >::const_iterator sLEnd = pQIter->second.sendList.end();

	while( sLIter != sLEnd )
	{
		CallPacket( pQIter->first, sLIter->first, sLIter->second );
		++sLIter;
	}
	sendList.clear();
	++pQIter;
}
packetQueue.clear();

That CallPacket() function would then use a switch statement to call the actual packet handler, making use of the void * to pull the object we are sending (if there is any object associated with this packet), the UI16 packet to pull the packet and any subcommand, and the CSocket to know which socket (or NULL for all) to send to.

This is still a VERY rough idea, and one that I only began thinking about last evening when working on the packet issues with the latest client. I have not done any testing and really don't know that this will even work. Mostly I would like feedback and criticism of this method of packet queueing, or thoughts on other ways to accomplish the same goal.

Thanks.

[xir]

That would probably work but why do you need a queue at all? If you are parsing incoming packets, you can just parse it a packet at a time from the data stream and then pass it onto the packet handler. Why do you need to store them?
If you are talking about outgoing packets, you can just create the packet and then merge it into the client's datastream. A timer can then be used to call something like FlushBuffers() which iterates through all the clients and sends as much data as possible. Storing the packets would work the same way, but the timer would merge the packets into the datastream first before sending.

I don't know about storing *all* the packets in one data structure. If you want to store the packets, have each CSocket (or whatever client class there is) to store it's own packets in an std::deque. Merging to the outgoing data stream sounds better to me than storing though.

Also instead of using void* object, you can probably use cPOutputBuffer* (?) object, since all the packets get inherited from this. You can then use the specialised C++ casts to cast down the inheritance chain (providing that the object was of that type in the first place.)

Not really sure whether you mean outgoing or incoming packets though. I suspect it was outgoing?

[giwo]

It was outgoing packets.

Basically the problem as it stands is twofold.

Firstly, we have times where multiple sections of code are executed in one loop forcing a refresh of an item or a character. This means that in just a few ms, we have sometimes 5 or 6 refresh calls to a single item, sending it to every socket in its area. Were 5 people playing, this would mean you would send this same packet 30 times, instead of 5.

The second problem is packet priority. Admittedly this problem can be fixed by simply ensuring you send packets in the proper order, but there are times when this becomes more complex. UOX3, because of how it was written, will often times have long branches of code doing quite a bit of work, and possibly sending packets the programmer who called that branch didn't realize it was sending. These can cause other packets not to function properly if sent before them (as the OSI client is finnicky).

Thus my goal was to reduce the excess sent packets (which is largely already done by the existant refreshQueue), and prioritize packets by sending them all at the same time and in a specific order.

[xir]

I don't know the system or problem as intimately as you do. I assume that you are talking about multiple changes to the state of an item/character within the same loop that clients need to know about. You could probably have a container initialised outside the loop that holds those items/players to be refreshed. When the loop is finished you could iterate through the loop and send the update packet? This would ensure that if an item is updated numerous times within the same loop it will only send one refresh per item/character. Would that work or am I totally off ?

Not sure on the second problem. Probably best thing to do is redo the crappy code to make sure it is consistant instead of coding more code around the crappy code and creating a dependancy on it