Cache the Keep Alive timestamp for forwarding ping (#4002)

* Cache the (clientbound) Keep Alive timestamp and use that for forwarding ping
* Use a Queue of keep alive IDs to handle KeepAlive packets sent in succession
* Don't force NetworkStackLatencyTranslator on the session's event loop
* Send clientbound NetworkStackLatencyPacket immediately
* Avoid sending negative NetworkStackLatencyPackets that are not from the form-image-hack in FormCache
* Downsize timestamps that would lead to overflow on the client

core/src/main/java/org/geysermc/geyser/session/GeyserSession.java

@@ -114,6 +114,7 @@ import org.geysermc.geyser.api.network.AuthType;
 import org.geysermc.geyser.api.network.RemoteServer;
 import org.geysermc.geyser.command.GeyserCommandSource;
 import org.geysermc.geyser.configuration.EmoteOffhandWorkaroundOption;
+import org.geysermc.geyser.configuration.GeyserConfiguration;
 import org.geysermc.geyser.entity.EntityDefinitions;
 import org.geysermc.geyser.entity.attribute.GeyserAttributeType;
 import org.geysermc.geyser.entity.type.Entity;
@@ -155,6 +156,7 @@ import java.nio.charset.StandardCharsets;
 import java.time.Instant;
 import java.util.*;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -564,6 +566,12 @@ public class GeyserSession implements GeyserConnection, GeyserCommandSource {
     @Setter
     private ScheduledFuture<?> mountVehicleScheduledFuture = null;
 
+    /**
+     * A cache of IDs from ClientboundKeepAlivePackets that have been sent to the Bedrock client, but haven't been returned to the server.
+     * Only used if {@link GeyserConfiguration#isForwardPlayerPing()} is enabled.
+     */
+    private final Queue<Long> keepAliveCache = new ConcurrentLinkedQueue<>();
+
     private MinecraftProtocol protocol;
 
     public GeyserSession(GeyserImpl geyser, BedrockServerSession bedrockServerSession, EventLoop eventLoop) {

core/src/main/java/org/geysermc/geyser/session/cache/FormCache.java

@@ -42,6 +42,13 @@ import java.util.concurrent.atomic.AtomicInteger;
 
 @RequiredArgsConstructor
 public class FormCache {
+
+    /**
+     * The magnitude of this doesn't actually matter, but it must be negative so that
+     * BedrockNetworkStackLatencyTranslator can detect the hack.
+     */
+    private static final long MAGIC_FORM_IMAGE_HACK_TIMESTAMP = -1234567890L;
+
     private final FormDefinitions formDefinitions = FormDefinitions.instance();
     private final AtomicInteger formIdCounter = new AtomicInteger(0);
     private final Int2ObjectMap<Form> forms = new Int2ObjectOpenHashMap<>();
@@ -73,7 +80,7 @@ public class FormCache {
         if (form instanceof SimpleForm) {
             NetworkStackLatencyPacket latencyPacket = new NetworkStackLatencyPacket();
             latencyPacket.setFromServer(true);
-            latencyPacket.setTimestamp(-System.currentTimeMillis());
+            latencyPacket.setTimestamp(MAGIC_FORM_IMAGE_HACK_TIMESTAMP);
             session.scheduleInEventLoop(
                     () -> session.sendUpstreamPacket(latencyPacket),
                     500, TimeUnit.MILLISECONDS

core/src/main/java/org/geysermc/geyser/translator/protocol/bedrock/BedrockNetworkStackLatencyTranslator.java

@@ -29,9 +29,7 @@ import com.github.steveice10.mc.protocol.packet.ingame.serverbound.ServerboundKe
 import org.cloudburstmc.protocol.bedrock.data.AttributeData;
 import org.cloudburstmc.protocol.bedrock.packet.NetworkStackLatencyPacket;
 import org.cloudburstmc.protocol.bedrock.packet.UpdateAttributesPacket;
-import org.geysermc.floodgate.util.DeviceOs;
 import org.geysermc.geyser.entity.attribute.GeyserAttributeType;
-import org.geysermc.geyser.network.GameProtocol;
 import org.geysermc.geyser.session.GeyserSession;
 import org.geysermc.geyser.translator.protocol.PacketTranslator;
 import org.geysermc.geyser.translator.protocol.Translator;
@@ -47,42 +45,43 @@ public class BedrockNetworkStackLatencyTranslator extends PacketTranslator<Netwo
 
     @Override
     public void translate(GeyserSession session, NetworkStackLatencyPacket packet) {
-        long pingId;
-        // so apparently, as of 1.16.200
-        // PS4 divides the network stack latency timestamp FOR US!!!
-        // WTF
-        if (GameProtocol.isPre1_20_10(session)) {
-            if (session.getClientData().getDeviceOs().equals(DeviceOs.PS4)) {
-                pingId = packet.getTimestamp();
-            } else {
-                pingId = packet.getTimestamp() / 1000;
-            }
-        } else {
-            // changed in 1.20.10 todo: is ps4 still different?
-            pingId = packet.getTimestamp() / (1000 * 1000 * 1000);
-        }
-
         // negative timestamps are used as hack to fix the url image loading bug
-        if (packet.getTimestamp() > 0) {
+        if (packet.getTimestamp() >= 0) {
             if (session.getGeyser().getConfig().isForwardPlayerPing()) {
-                ServerboundKeepAlivePacket keepAlivePacket = new ServerboundKeepAlivePacket(pingId);
+                // use our cached value because
+                // a) bedrock can be inaccurate with the value returned
+                // b) playstation replies with a different magnitude than other platforms
+                // c) 1.20.10 and later reply with a different magnitude
+                Long keepAliveId = session.getKeepAliveCache().poll();
+                if (keepAliveId == null) {
+                    session.getGeyser().getLogger().debug("Received a latency packet that we don't have a KeepAlive for: " + packet);
+                    return;
+                }
+
+                ServerboundKeepAlivePacket keepAlivePacket = new ServerboundKeepAlivePacket(keepAliveId);
                 session.sendDownstreamPacket(keepAlivePacket);
             }
             return;
         }
 
-        // Hack to fix the url image loading bug
-        UpdateAttributesPacket attributesPacket = new UpdateAttributesPacket();
-        attributesPacket.setRuntimeEntityId(session.getPlayerEntity().getGeyserId());
+        session.scheduleInEventLoop(() -> {
+            // Hack to fix the url image loading bug
+            UpdateAttributesPacket attributesPacket = new UpdateAttributesPacket();
+            attributesPacket.setRuntimeEntityId(session.getPlayerEntity().getGeyserId());
 
-        AttributeData attribute = session.getPlayerEntity().getAttributes().get(GeyserAttributeType.EXPERIENCE_LEVEL);
-        if (attribute != null) {
-            attributesPacket.setAttributes(Collections.singletonList(attribute));
-        } else {
-            attributesPacket.setAttributes(Collections.singletonList(GeyserAttributeType.EXPERIENCE_LEVEL.getAttribute(0)));
-        }
+            AttributeData attribute = session.getPlayerEntity().getAttributes().get(GeyserAttributeType.EXPERIENCE_LEVEL);
+            if (attribute != null) {
+                attributesPacket.setAttributes(Collections.singletonList(attribute));
+            } else {
+                attributesPacket.setAttributes(Collections.singletonList(GeyserAttributeType.EXPERIENCE_LEVEL.getAttribute(0)));
+            }
 
-        session.scheduleInEventLoop(() -> session.sendUpstreamPacket(attributesPacket),
-                500, TimeUnit.MILLISECONDS);
+            session.sendUpstreamPacket(attributesPacket);
+        }, 500, TimeUnit.MILLISECONDS);
+    }
+
+    @Override
+    public boolean shouldExecuteInEventLoop() {
+        return false;
     }
 }

core/src/main/java/org/geysermc/geyser/translator/protocol/java/JavaKeepAliveTranslator.java

@@ -42,10 +42,30 @@ public class JavaKeepAliveTranslator extends PacketTranslator<ClientboundKeepAli
         if (!session.getGeyser().getConfig().isForwardPlayerPing()) {
             return;
         }
+        // We use this once the client replies (see BedrockNetworkStackLatencyTranslator)
+        session.getKeepAliveCache().add(packet.getPingId());
+
+        long timestamp = packet.getPingId();
+
+        // We take the abs because we rely on the client responding with a negative value ONLY when we send
+        // a negative timestamp in the form-image-hack performed in FormCache.
+        // Apart from that case, we don't actually use the value the client responds with, instead using our keep alive cache.
+        if (timestamp == Long.MIN_VALUE) {
+            timestamp = Long.MAX_VALUE; // There is not an abs representation of MIN_VALUE (2's complement)
+        } else {
+            timestamp = Math.absExact(timestamp);
+        }
+
+        // Bedrock will overflow on timestamps that are too large, and respond with a mangled negative value.
+        // Keeping leftmost digits allows for easier debugging
+        while (timestamp > 1e10) {
+            timestamp /= 10;
+        }
+
         NetworkStackLatencyPacket latencyPacket = new NetworkStackLatencyPacket();
         latencyPacket.setFromServer(true);
-        latencyPacket.setTimestamp(packet.getPingId() * 1000);
-        session.sendUpstreamPacket(latencyPacket);
+        latencyPacket.setTimestamp(timestamp);
+        session.sendUpstreamPacketImmediately(latencyPacket);
     }
 
     @Override