refactor of vehicle packet handling

include/TServer.h

@@ -43,8 +43,8 @@ private:
     static bool ShouldSpawn(TClient& c, const std::string& CarJson, int ID);
     static bool IsUnicycle(TClient& c, const std::string& CarJson);
     static void Apply(TClient& c, int VID, const std::string& pckt);
-    static bool HandlePosition(TClient& c, const std::string& Packet);
+    static bool HandlePosition(TClient& c, const std::string& PacketStr);
-    static bool HandleVehicleUpdate(TClient& c, const std::string& Packet);
+    static bool HandleVehicleUpdate(const std::string& PacketStr, const int playerID);
 };
 
 struct BufferView {

src/TConsole.cpp

@@ -24,6 +24,17 @@ static inline bool StringStartsWith(const std::string& What, const std::string&
     return What.size() >= StartsWith.size() && What.substr(0, StartsWith.size()) == StartsWith;
 }
 
+static inline bool StringStartsWithLower(const std::string& Name1, const std::string& Name2) {
+    std::string Name1Lower = boost::algorithm::to_lower_copy(Name1);
+    return StringStartsWith(Name1Lower, Name2) || StringStartsWith(Name2, Name1Lower);
+};
+
+static inline bool StringStartsWithLowerBoth(const std::string& Name1, const std::string& Name2) {
+    std::string Name1Lower = boost::algorithm::to_lower_copy(Name1);
+    std::string Name2Lower = boost::algorithm::to_lower_copy(Name2);
+    return StringStartsWith(Name1Lower, Name2Lower) || StringStartsWith(Name2Lower, Name1Lower);
+};
+
 TEST_CASE("StringStartsWith") {
     CHECK(StringStartsWith("Hello, World", "Hello"));
     CHECK(StringStartsWith("Hello, World", "H"));
@@ -363,23 +374,18 @@ void TConsole::Command_Kick(const std::string&, const std::vector<std::string>&
     if (!EnsureArgsCount(args, 1, size_t(-1))) {
         return;
     }
-    auto Name = args.at(0);
+    auto Name = boost::algorithm::to_lower_copy(args.at(0));
     std::string Reason = "Kicked by server console";
     if (args.size() > 1) {
         Reason = ConcatArgs({ args.begin() + 1, args.end() });
     }
     beammp_trace("attempt to kick '" + Name + "' for '" + Reason + "'");
     bool Kicked = false;
-    // TODO: this sucks, tolower is locale-dependent.
+
-    auto NameCompare = [](std::string Name1, std::string Name2) -> bool {
-        std::string Name1Lower = boost::algorithm::to_lower_copy(Name1);
-        std::string Name2Lower = boost::algorithm::to_lower_copy(Name2);
-        return StringStartsWith(Name1Lower, Name2Lower) || StringStartsWith(Name2Lower, Name1Lower);
-    };
     mLuaEngine->Server().ForEachClient([&](std::weak_ptr<TClient> Client) -> bool {
         auto Locked = Client.lock();
         if (Locked) {
-            if (NameCompare(Locked->GetName(), Name)) {
+            if (StringStartsWithLower(Locked->GetName(), Name)) {
                 mLuaEngine->Network().ClientKick(*Locked, Reason);
                 Kicked = true;
                 return false;
@@ -666,15 +672,10 @@ void TConsole::Autocomplete_Lua(const std::string& stub, std::vector<std::string
 void TConsole::Autocomplete_Kick(const std::string& stub, std::vector<std::string>& suggestions) {
     std::string stub_lower = boost::algorithm::to_lower_copy(stub);
 
-    auto LowercaseCompare = [](std::string Name1, std::string Name2) -> bool {
-        std::string NameLower = boost::algorithm::to_lower_copy(Name1);
-        return StringStartsWith(NameLower, Name2);
-    };
-
     mLuaEngine->Server().ForEachClient([&](std::weak_ptr<TClient> Client) -> bool {
         auto Locked = Client.lock();
         if (Locked) {
-            if (LowercaseCompare(Locked->GetName(), stub_lower)) {
+            if (StringStartsWithLower(Locked->GetName(), stub_lower)) {
                 suggestions.push_back("kick " + Locked->GetName());
             }
         }
@@ -691,16 +692,11 @@ void TConsole::Autocomplete_Settings(const std::string& stub, std::vector<std::s
     std::string arg;
     if (!args.empty()) arg = boost::algorithm::to_lower_copy(args.at(0));
 
-    auto LowercaseCompare = [](std::string Name1, std::string Name2) -> bool {
-        std::string NameLower = boost::algorithm::to_lower_copy(Name1);
-        return StringStartsWith(NameLower, Name2);
-    };
-
     // suggest setting names
     if (command == "set" || command == "get") {
         for (const auto& [k, v] : Application::mSettings) {
             std::string key = std::string(k);
-            if (LowercaseCompare(key, arg)) {
+            if (StringStartsWithLower(key, arg)) {
                 suggestions.push_back("settings " + command + " " + key);
             }
         }

src/TServer.cpp

@@ -17,6 +17,12 @@
 
 #include "Json.h"
 
+struct VehiclePacket {
+    std::string Data;
+    int Pid;
+    int Vid;
+};
+
 static std::optional<std::pair<int, int>> GetPidVid(const std::string& str) {
     auto IDSep = str.find('-');
     std::string pid = str.substr(0, IDSep);
@@ -34,6 +40,42 @@ static std::optional<std::pair<int, int>> GetPidVid(const std::string& str) {
     return std::nullopt;
 }
 
+static std::optional<VehiclePacket> ParseVehiclePacket(const std::string& Packet, const int playerID) {
+    if (Packet.size() < 8) { //2 for code, 3<= for pidvid, 1<= for data, 2 for dividers
+        // invalid packet
+        return std::nullopt;
+    }
+    // Zp:serverVehicleID:data
+    // 0-0:data
+    std::string withoutCode = Packet.substr(3);
+
+    auto NameDataSep = withoutCode.find(':', 3);
+    if (NameDataSep == std::string::npos) {
+        // invalid packet
+        return std::nullopt;
+    }
+    if (NameDataSep + 1 > withoutCode.size()) {
+        // invalid packet
+        return std::nullopt;
+    }
+
+    std::string ServerVehicleID = withoutCode.substr(0, NameDataSep);
+
+    std::string Data = withoutCode.substr(NameDataSep + 1);
+
+    // parse veh ID
+    auto MaybePidVid = GetPidVid(ServerVehicleID);
+    if (MaybePidVid) {
+        int PID, VID;
+        std::tie(PID, VID) = MaybePidVid.value();
+
+        if (PID == playerID) {
+            return { {Data, PID, VID} }; //std::vector<char>(Data.begin(), Data.end())
+        }
+    }
+    return std::nullopt;
+}
+
 TEST_CASE("GetPidVid") {
     SUBCASE("Valid singledigit") {
         const auto MaybePidVid = GetPidVid("0-1");
@@ -77,6 +119,39 @@ TEST_CASE("GetPidVid") {
     }
 }
 
+TEST_CASE("ParseVehiclePacket") {
+    SUBCASE("Valid packet") {
+        const auto valid = ParseVehiclePacket("Zp:0-0:{jsonstring}", 0);
+        CHECK(valid.has_value());
+    }
+    SUBCASE("Valid packet 2") {
+        const auto packet = ParseVehiclePacket("Zp:12-3:{jsonstring}", 12);
+        CHECK(packet.has_value());
+        CHECK_EQ(packet.value().Pid, 12);
+        CHECK_EQ(packet.value().Vid, 3);
+    }
+    SUBCASE("Missing packet") {
+        const auto valid = ParseVehiclePacket("", 0);
+        CHECK(!valid.has_value());
+    }
+    SUBCASE("Missing ServerVehicleID") {
+        const auto valid = ParseVehiclePacket("Zp:{jsonstring}", 0);
+        CHECK(!valid.has_value());
+    }
+    SUBCASE("Missing data") {
+        const auto valid = ParseVehiclePacket("Zp:0-0:", 0);
+        CHECK(!valid.has_value());
+    }
+    SUBCASE("Incorrect Pid") {
+        const auto valid = ParseVehiclePacket("Zp:0-0:{jsonstring}", 1);
+        CHECK(!valid.has_value());
+    }
+    SUBCASE("Incorrect Pid 2") {
+        const auto valid = ParseVehiclePacket("Zp:12-0:{jsonstring}", 13);
+        CHECK(!valid.has_value());
+    }
+}
+
 TServer::TServer(const std::vector<std::string_view>& Arguments) {
     beammp_infof("BeamMP Server v{} ({})", Application::ServerVersionString(), BEAMMP_GIT_HASH);
     Application::SetSubsystemStatus("Server", Application::Status::Starting);
@@ -152,7 +227,7 @@ void TServer::GlobalParser(const std::weak_ptr<TClient>& Client, std::vector<uin
 
     // V to Y
     if (Code <= 89 && Code >= 86) {
-        if (HandleVehicleUpdate(*LockedClient, StringPacket)){
+        if (HandleVehicleUpdate(StringPacket, LockedClient->GetID())){
             Network.SendToAll(LockedClient.get(), Packet, false, false);
         } else {
             beammp_debugf("Invalid vehicle update packet received from '{}' ({}), ignoring it", LockedClient->GetName(), LockedClient->GetID());
@@ -438,71 +513,19 @@ void TServer::InsertClient(const std::shared_ptr<TClient>& NewClient) {
     (void)mClients.insert(NewClient);
 }
 
-bool TServer::HandlePosition(TClient& c, const std::string& Packet) {
+bool TServer::HandlePosition(TClient& c, const std::string& PacketStr) {
-    if (Packet.size() < 3) {
+    auto MaybePacket = ParseVehiclePacket(PacketStr, c.GetID());
-        // invalid packet
-        return false;
-    }
-    // Zp:serverVehicleID:data
-    // Zp:0:data
-    std::string withoutCode = Packet.substr(3);
-    auto NameDataSep = withoutCode.find(':', 2);
-    if (NameDataSep == std::string::npos || NameDataSep < 2) {
-        // invalid packet
-        return false;
-    }
-    // FIXME: ensure that -2 does what it should... it seems weird.
-    std::string ServerVehicleID = withoutCode.substr(2, NameDataSep - 2);
-    if (NameDataSep + 1 > withoutCode.size()) {
-        // invalid packet
-        return false;
-    }
-    std::string Data = withoutCode.substr(NameDataSep + 1);
 
-    // parse veh ID
+    if (MaybePacket) {
-    auto MaybePidVid = GetPidVid(ServerVehicleID);
+        auto packet = MaybePacket.value();
-    if (MaybePidVid) {
+        c.SetCarPosition(packet.Vid, packet.Data);
-        int PID = -1;
+        return true;
-        int VID = -1;
-        std::tie(PID, VID) = MaybePidVid.value();
-        if (PID == c.GetID()) {
-            c.SetCarPosition(VID, Data);
-            return true;
-        }
     }
     return false;
 }
 
-bool TServer::HandleVehicleUpdate(TClient& c, const std::string& Packet) {
+bool TServer::HandleVehicleUpdate(const std::string& PacketStr, const int playerID) {
-    if (Packet.size() < 3) {
+    auto MaybePacket = ParseVehiclePacket(PacketStr, playerID);
-        // invalid packet
-        return false;
-    }
-    // (Vi/We/Yl):serverVehicleID:data
-    // (Vi/We/Yl):serverVehicleID:data
-    std::string withoutCode = Packet.substr(3);
-    auto NameDataSep = withoutCode.find(':', 2);
-    if (NameDataSep == std::string::npos || NameDataSep < 2) {
-        // invalid packet
-        return false;
-    }
-    // FIXME: ensure that -2 does what it should... it seems weird.
-    std::string ServerVehicleID = withoutCode.substr(2, NameDataSep - 2);
-    if (NameDataSep + 1 > withoutCode.size()) {
-        // invalid packet
-        return false;
-    }
-    std::string Data = withoutCode.substr(NameDataSep + 1);
 
-    // parse veh ID
+    return MaybePacket.has_value();
-    auto MaybePidVid = GetPidVid(ServerVehicleID);
-    if (MaybePidVid) {
-        int PID = -1;
-        int VID = -1;
-        std::tie(PID, VID) = MaybePidVid.value();
-        if (PID == c.GetID()) {
-            return true;
-        }
-    }
-    return false;
 }