ProtocolLib

3M Downloads

getScheduledPackets() changed from List to Set, javadoc not updated. Why?

bergerkiller opened this issue ยท 2 comments

commented

Describe the question
NetworkMarker has a getScheduledPackets() method, which you can use to add packets to be sent after the handling of the current packet. This changed, as of the v5 api, to be a Set rather than a List, throwing me off-guard for a second. The javadoc hasn't been updated and still says List, btw.

Looking at the commits responsible for this 2 months ago, I could see no reason why it had to be a set. It also made little sense to use a HashSet here, isn't order important when scheduling packets? And how can a ScheduledPacket ever require a duplicate check? The same packet shouldnt be scheduled twice in the first place.

This has the unfortunate side-effect that what I wanted to do - schedule the packet by adding it to index 0 rather than at the end, so it behaves more like sending a packet while handling another, isn't possible anymore. Probably not a big deal, and I'll be using PacketEvent's schedule method instead. I wasn't even sure if adding it anywhere other than at the end was supported.

Possible bug?
If an unordered set is used, it means multiple schedule() won't have a guaranteed order of sending. This will break things like spawning new entities, for example, where metadata packet must come after the spawn packet. At the very least it should be a LinkedHashSet, if it should be a set at all.

API method(s) used
NetworkMarker::getScheduledPackets()
PacketEvent::schedule()

commented

Probably using a Queue there is the best and most fitting solution? ๐Ÿค”

The javadoc hasn't been updated and still says List, btw.

I normally don't write Javadoc which reference the return type directly, so wasn't even looking at that ๐Ÿ˜‚

commented

I'll leave that up to you to decide. Queue is probably best yeah, if the aim is preventing anyone from inserting packets in the middle of other scheduled packets. But if it isn't a technical limitation that decides that, can't see why it can't be a List instead.

A queue might be more efficient if it's involved in a push/pop kinda arrangement.