Fix/info packets

protocol/osrs-221/osrs-221-model/src/main/kotlin/net/rsprot/protocol/game/outgoing/info/npcinfo/NpcInfo.kt

@@ -135,6 +135,7 @@ public class NpcInfo internal constructor(
      * This exception will be propagated further during the [toNpcInfoPacket] function call,
      * allowing the server to handle it properly at a per-player basis.
      */
+    @Volatile
     internal var exception: Exception? = null
 
     /**
@@ -146,6 +147,8 @@ public class NpcInfo internal constructor(
      */
     private var builtIntoPacket: Boolean = false
 
+    override fun isDestroyed(): Boolean = this.exception != null
+
     /**
      * Returns the backing byte buffer holding all the computed information.
      * @throws IllegalStateException if the buffer is null, meaning it has no yet been

protocol/osrs-221/osrs-221-model/src/main/kotlin/net/rsprot/protocol/game/outgoing/info/npcinfo/NpcInfoProtocol.kt

@@ -75,6 +75,10 @@ public class NpcInfoProtocol(
      * @param info the npc info object to deallocate
      */
     public fun dealloc(info: NpcInfo) {
+        // Prevent returning a destroyed npc info object back into the pool
+        if (info.isDestroyed()) {
+            return
+        }
         npcInfoRepository.dealloc(info.localPlayerIndex)
     }
 

protocol/osrs-221/osrs-221-model/src/main/kotlin/net/rsprot/protocol/game/outgoing/info/playerinfo/PlayerAvatar.kt

@@ -80,8 +80,32 @@ public class PlayerAvatar internal constructor(
             huffmanCodec,
         )
 
+    /**
+     * Whether this avatar is completed hidden from everyone else. Note that this completely skips
+     * sending any information to the client about this given avatar, it is not the same as soft
+     * hiding via the appearance extended info.
+     */
     internal var hidden: Boolean = false
 
+    /**
+     * The [PlayerInfoProtocol.cycleCount] when this avatar was allocated.
+     * We use this to determine whether to perform a re-synchronization of a player,
+     * which can happen when a player is deallocated and reallocated on the same cycle,
+     * which could result in other players not seeing any change take place. While rare,
+     * this possibility exists, and it could result in some rather odd bugs.
+     */
+    internal var allocateCycle: Int = PlayerInfoProtocol.cycleCount
+
+    /**
+     * Sets the avatar as hidden (or unhidden, depending on [hidden]). When hidden via this function,
+     * no information is transmitted to the clients about this avatar. It is a hard-hiding function,
+     * unlike the one via appearance extended info, which strictly only hides client-side, but all
+     * clients still receive information about the client existing.
+     * The benefit to this function is that no plugins or RuneLite implementations can snoop on other
+     * players that are meant to be hidden. The downside, however, is that because the client has no
+     * knowledge of that specific avatar whatsoever, un-hiding while the player is moving is not as
+     * smooth as with the appearance variant, since it first appears as if the player teleported in.
+     */
     public fun setHidden(hidden: Boolean) {
         this.hidden = hidden
     }

protocol/osrs-221/osrs-221-model/src/main/kotlin/net/rsprot/protocol/game/outgoing/info/playerinfo/PlayerInfo.kt

@@ -150,6 +150,7 @@ public class PlayerInfo internal constructor(
      * This exception will be propagated further during the [toPacket] function call,
      * allowing the server to handle it properly at a per-player basis.
      */
+    @Volatile
     internal var exception: Exception? = null
 
     /**
@@ -174,6 +175,8 @@ public class PlayerInfo internal constructor(
     @Throws(IllegalStateException::class)
     public fun backingBuffer(): ByteBuf = checkNotNull(buffer)
 
+    override fun isDestroyed(): Boolean = this.exception != null
+
     /**
      * Updates the build area of this player info object.
      * This will ensure that no players outside of this box will be
@@ -610,6 +613,12 @@ public class PlayerInfo internal constructor(
         if (other.avatar.hidden) {
             return false
         }
+        // If the avatar was allocated on this cycle, ensure we remove (and potentially re-add later)
+        // this avatar. This is due to someone logging out and another player taking the avatar the same
+        // cycle - which would otherwise potentially go by unnoticed, with the client assuming nothing changed.
+        if (other.avatar.allocateCycle == PlayerInfoProtocol.cycleCount) {
+            return false
+        }
         val coord = other.avatar.currentCoord
         if (!coord.inDistance(this.avatar.currentCoord, this.avatar.resizeRange)) {
             return false
@@ -708,6 +717,7 @@ public class PlayerInfo internal constructor(
         this.oldSchoolClientType = oldSchoolClientType
         this.buildArea = BuildArea.INVALID
         avatar.reset()
+        this.avatar.allocateCycle = PlayerInfoProtocol.cycleCount
         lowResolutionIndices.fill(0)
         lowResolutionCount = 0
         highResolutionIndices.fill(0)

protocol/osrs-221/osrs-221-model/src/main/kotlin/net/rsprot/protocol/game/outgoing/info/playerinfo/PlayerInfoProtocol.kt

@@ -101,6 +101,10 @@ public class PlayerInfoProtocol(
      * @param info the player info object
      */
     public fun dealloc(info: PlayerInfo) {
+        // Prevent returning a destroyed player info object back into the pool
+        if (info.isDestroyed()) {
+            return
+        }
         playerInfoRepository.dealloc(info.localIndex)
     }
 
@@ -118,6 +122,7 @@ public class PlayerInfoProtocol(
         prepareExtendedInfo()
         putExtendedInfo()
         postUpdate()
+        cycleCount++
     }
 
     /**
@@ -254,5 +259,18 @@ public class PlayerInfoProtocol(
          */
         public const val PROTOCOL_CAPACITY: Int = 2048
         private val logger: InlineLogger = InlineLogger()
+
+        /**
+         * The number of Player info update cycles that have occurred.
+         * We need to track this to avoid a nasty bug with servers de-allocating + re-allocating
+         * an avatar on the same cycle, in a small area. The effective bug is that another player takes
+         * ones' avatar, which leads to info protocol thinking nothing has changed (assuming the new player
+         * is still within range of the old one, enough to be in high resolution).
+         *
+         * We solve this by forcibly removing a player from high resolution view if the avatar was
+         * allocated on current cycle. If the player is still within range, they will be re-added
+         * later on in the cycle via low resolution updates - but correctly this time around!
+         */
+        internal var cycleCount: Int = 0
     }
 }

protocol/osrs-221/osrs-221-model/src/main/kotlin/net/rsprot/protocol/game/outgoing/info/util/ReferencePooledObject.kt

@@ -30,4 +30,14 @@ public interface ReferencePooledObject {
      * during the [onAlloc], to ensure no work is 'wasted'.
      */
     public fun onDealloc()
+
+    /**
+     * Whether this reference pooled object is destroyed.
+     * A destroyed object will not be returned back to the pool and instead will be left off for the
+     * garbage collector to clean up in due time. This condition is only hit when there was some error
+     * thrown during the processing of a given info object. In order to mitigate potential future
+     * problems that might continue to stem from re-using this object, we discard it altogether.
+     * @return whether the info object is destroyed and should not be returned to the pool.
+     */
+    public fun isDestroyed(): Boolean
 }

protocol/osrs-222/osrs-222-model/src/main/kotlin/net/rsprot/protocol/game/outgoing/info/npcinfo/NpcInfo.kt

@@ -58,6 +58,7 @@ public class NpcInfo internal constructor(
      * This exception will be propagated further during the [toNpcInfoPacket] function call,
      * allowing the server to handle it properly at a per-player basis.
      */
+    @Volatile
     internal var exception: Exception? = null
 
     /**
@@ -66,6 +67,8 @@ public class NpcInfo internal constructor(
      */
     internal val details: Array<NpcInfoWorldDetails?> = arrayOfNulls(WORLD_ENTITY_CAPACITY + 1)
 
+    override fun isDestroyed(): Boolean = this.exception != null
+
     /**
      * Updates the build area of a given world to the specified one.
      * This will ensure that no NPCs outside of this box will be

protocol/osrs-222/osrs-222-model/src/main/kotlin/net/rsprot/protocol/game/outgoing/info/npcinfo/NpcInfoProtocol.kt

@@ -78,6 +78,10 @@ public class NpcInfoProtocol(
      * @param info the npc info object to deallocate
      */
     public fun dealloc(info: NpcInfo) {
+        // Prevent returning a destroyed npc info object back into the pool
+        if (info.isDestroyed()) {
+            return
+        }
         npcInfoRepository.dealloc(info.localPlayerIndex)
     }
 

protocol/osrs-222/osrs-222-model/src/main/kotlin/net/rsprot/protocol/game/outgoing/info/playerinfo/PlayerAvatar.kt

@@ -80,8 +80,32 @@ public class PlayerAvatar internal constructor(
             huffmanCodec,
         )
 
+    /**
+     * Whether this avatar is completed hidden from everyone else. Note that this completely skips
+     * sending any information to the client about this given avatar, it is not the same as soft
+     * hiding via the appearance extended info.
+     */
     internal var hidden: Boolean = false
 
+    /**
+     * The [PlayerInfoProtocol.cycleCount] when this avatar was allocated.
+     * We use this to determine whether to perform a re-synchronization of a player,
+     * which can happen when a player is deallocated and reallocated on the same cycle,
+     * which could result in other players not seeing any change take place. While rare,
+     * this possibility exists, and it could result in some rather odd bugs.
+     */
+    internal var allocateCycle: Int = PlayerInfoProtocol.cycleCount
+
+    /**
+     * Sets the avatar as hidden (or unhidden, depending on [hidden]). When hidden via this function,
+     * no information is transmitted to the clients about this avatar. It is a hard-hiding function,
+     * unlike the one via appearance extended info, which strictly only hides client-side, but all
+     * clients still receive information about the client existing.
+     * The benefit to this function is that no plugins or RuneLite implementations can snoop on other
+     * players that are meant to be hidden. The downside, however, is that because the client has no
+     * knowledge of that specific avatar whatsoever, un-hiding while the player is moving is not as
+     * smooth as with the appearance variant, since it first appears as if the player teleported in.
+     */
     public fun setHidden(hidden: Boolean) {
         this.hidden = hidden
     }

protocol/osrs-222/osrs-222-model/src/main/kotlin/net/rsprot/protocol/game/outgoing/info/playerinfo/PlayerInfo.kt

@@ -83,6 +83,7 @@ public class PlayerInfo internal constructor(
      * This exception will be propagated further during the [toPacket] function call,
      * allowing the server to handle it properly at a per-player basis.
      */
+    @Volatile
     internal var exception: Exception? = null
 
     /**
@@ -118,6 +119,8 @@ public class PlayerInfo internal constructor(
         avatar.extendedInfo.invalidateAppearanceCache()
     }
 
+    override fun isDestroyed(): Boolean = this.exception != null
+
     /**
      * Sets an active world in which the player currently resides.
      * @param worldId the world id in which the player resides. A value of -1 implies
@@ -682,6 +685,12 @@ public class PlayerInfo internal constructor(
         if (other.avatar.hidden) {
             return false
         }
+        // If the avatar was allocated on this cycle, ensure we remove (and potentially re-add later)
+        // this avatar. This is due to someone logging out and another player taking the avatar the same
+        // cycle - which would otherwise potentially go by unnoticed, with the client assuming nothing changed.
+        if (other.avatar.allocateCycle == PlayerInfoProtocol.cycleCount) {
+            return false
+        }
         val coord = other.avatar.currentCoord
         if (!details.renderCoord.inDistance(coord, this.avatar.resizeRange)) {
             return false
@@ -798,6 +807,7 @@ public class PlayerInfo internal constructor(
         avatar.extendedInfo.localIndex = index
         this.oldSchoolClientType = oldSchoolClientType
         avatar.reset()
+        this.avatar.allocateCycle = PlayerInfoProtocol.cycleCount
         this.activeWorldId = ROOT_WORLD
         // There is always a root world!
         val rootDetails = protocol.detailsStorage.poll(ROOT_WORLD)

protocol/osrs-222/osrs-222-model/src/main/kotlin/net/rsprot/protocol/game/outgoing/info/playerinfo/PlayerInfoProtocol.kt

@@ -107,6 +107,10 @@ public class PlayerInfoProtocol(
      * @param info the player info object
      */
     public fun dealloc(info: PlayerInfo) {
+        // Prevent returning a destroyed player info object back into the pool
+        if (info.isDestroyed()) {
+            return
+        }
         playerInfoRepository.dealloc(info.localIndex)
     }
 
@@ -124,6 +128,7 @@ public class PlayerInfoProtocol(
         prepareExtendedInfo()
         putExtendedInfo()
         postUpdate()
+        cycleCount++
     }
 
     /**
@@ -277,5 +282,18 @@ public class PlayerInfoProtocol(
          */
         public const val PROTOCOL_CAPACITY: Int = 2048
         private val logger: InlineLogger = InlineLogger()
+
+        /**
+         * The number of Player info update cycles that have occurred.
+         * We need to track this to avoid a nasty bug with servers de-allocating + re-allocating
+         * an avatar on the same cycle, in a small area. The effective bug is that another player takes
+         * ones' avatar, which leads to info protocol thinking nothing has changed (assuming the new player
+         * is still within range of the old one, enough to be in high resolution).
+         *
+         * We solve this by forcibly removing a player from high resolution view if the avatar was
+         * allocated on current cycle. If the player is still within range, they will be re-added
+         * later on in the cycle via low resolution updates - but correctly this time around!
+         */
+        internal var cycleCount: Int = 0
     }
 }

protocol/osrs-222/osrs-222-model/src/main/kotlin/net/rsprot/protocol/game/outgoing/info/util/ReferencePooledObject.kt

@@ -31,4 +31,14 @@ public interface ReferencePooledObject {
      * during the [onAlloc], to ensure no work is 'wasted'.
      */
     public fun onDealloc()
+
+    /**
+     * Whether this reference pooled object is destroyed.
+     * A destroyed object will not be returned back to the pool and instead will be left off for the
+     * garbage collector to clean up in due time. This condition is only hit when there was some error
+     * thrown during the processing of a given info object. In order to mitigate potential future
+     * problems that might continue to stem from re-using this object, we discard it altogether.
+     * @return whether the info object is destroyed and should not be returned to the pool.
+     */
+    public fun isDestroyed(): Boolean
 }

protocol/osrs-222/osrs-222-model/src/main/kotlin/net/rsprot/protocol/game/outgoing/info/worldentityinfo/WorldEntityAvatar.kt

@@ -51,6 +51,15 @@ public class WorldEntityAvatar(
 
     internal var highResolutionBuffer: ByteBuf? = null
 
+    /**
+     * The [WorldEntityProtocol.cycleCount] when this avatar was allocated.
+     * We use this to determine whether to perform a re-synchronization of a worldentity,
+     * which can happen when a worldentity is deallocated and reallocated on the same cycle,
+     * which could result in other clients not seeing any change take place. While rare,
+     * this possibility exists, and it could result in some rather odd bugs.
+     */
+    internal var allocateCycle: Int = WorldEntityProtocol.cycleCount
+
     /**
      * Precomputes the high resolution buffer of this world entity.
      */

protocol/osrs-222/osrs-222-model/src/main/kotlin/net/rsprot/protocol/game/outgoing/info/worldentityinfo/WorldEntityAvatarRepository.kt

@@ -15,19 +15,12 @@ import java.lang.ref.SoftReference
  * As a soft reference queue, it will hold on-to the unused references until the JVM
  * absolutely needs the memory - before that, these can be reused, making it a perfect
  * use case for the pooling mechanism.
- * @property releasedAvatarQueue the avatars that were released within this cycle.
- * These avatars are initially put into a different structure as we cannot immediately
- * release them - they could be picked up by something else the same cycle, which could
- * lead to some weird bugs occurring. Instead, we wait for one cycle to pass before
- * pushing them to the queue to be re-usable. This ensures no one is relying on this
- * same instance still.
  */
 public class WorldEntityAvatarRepository internal constructor(
     private val allocator: ByteBufAllocator,
 ) {
     private val elements: Array<WorldEntityAvatar?> = arrayOfNulls(AVATAR_CAPACITY)
     private val queue: ReferenceQueue<WorldEntityAvatar> = ReferenceQueue<WorldEntityAvatar>()
-    private val releasedAvatarQueue: ArrayDeque<WorldEntityAvatar> = ArrayDeque()
 
     /**
      * Gets a world entity at the provided [idx], or null if it doesn't exist.
@@ -67,6 +60,7 @@ public class WorldEntityAvatarRepository internal constructor(
             existing.currentCoord = CoordGrid(level, x, z)
             existing.lastCoord = existing.currentCoord
             existing.angle = angle
+            existing.allocateCycle = WorldEntityProtocol.cycleCount
             elements[index] = existing
             return existing
         }
@@ -89,21 +83,8 @@ public class WorldEntityAvatarRepository internal constructor(
      */
     public fun release(avatar: WorldEntityAvatar) {
         this.elements[avatar.index] = null
-        releasedAvatarQueue += avatar
-    }
-
-    /**
-     * Transfers the recently released avatars over to the pool, so they can be re-used.
-     */
-    internal fun transferAvatars() {
-        if (releasedAvatarQueue.isEmpty()) {
-            return
-        }
-        while (releasedAvatarQueue.isNotEmpty()) {
-            val avatar = releasedAvatarQueue.removeFirst()
-            val reference = SoftReference(avatar, queue)
-            reference.enqueue()
-        }
+        val reference = SoftReference(avatar, queue)
+        reference.enqueue()
     }
 
     internal companion object {