Fix for drone deadlock #190
Conversation
| @@ -9,16 +9,26 @@ public static class DroneManager | |||
| public static int[] DronePriorities = new int[255]; | |||
| public static Random rnd = new Random(); | |||
| public static Dictionary<ushort, List<int>> PlayerDroneBuildingPlans = new Dictionary<ushort, List<int>>(); | |||
| public static List<int> PendingBuildRequests = new List<int>(); | |||
There was a problem hiding this comment.
I believe it is modified only from the main thread, from the GameTick() or from the Update() when packets are processed. It is purely used only on the client-side.
There was a problem hiding this comment.
I might be thinking about it too much, but there's not much clarity or convention on when something can be accessed from Host/Client/Both and what is the source of truth for each one.
There was a problem hiding this comment.
For this case of PendingBuildRequests it is used by clients to remember, for which prebuilds the request was sent. The issue was following: drone reached the prebuild, client sent request to build it, however from the drone point of view the prebuild still exist and still need to be built. So I have created this list to ignore prebuilds for which the client already sent request and it is just waiting for the response from server.
But it is true, that for the "Manager" classes, it is sometimes hard to see, what is used by clients and what by the host.
NebulaWorld/Player/DroneManager.cs
Outdated
| public static void RemoveBuildRequest(int entityId) | ||
| { | ||
| PendingBuildRequests.Remove(entityId); |
There was a problem hiding this comment.
Allow notifying caller if the entry was removed.
| public static void RemoveBuildRequest(int entityId) | |
| { | |
| PendingBuildRequests.Remove(entityId); | |
| public static bool RemoveBuildRequest(int entityId) | |
| { | |
| return PendingBuildRequests.Remove(entityId); |
There was a problem hiding this comment.
I am not sure if it would be helpful, maybe just to print a warning to logs that some exceptional case occured?
Since:
- Item is added to the array when the build request is sent to the host.
- item is removed from the array when the response from the host is received.
Co-authored-by: Marius Ungureanu <[email protected]>
…bula into drone-deadlock-fix
| planet.factory.BuildFinally(GameMain.mainPlayer, packet.PrebuildId); | ||
| if (!DroneManager.RemoveBuildRequest(-packet.PrebuildId)) | ||
| { | ||
| Log.Warn($"Build Request was not succesful removed."); |
There was a problem hiding this comment.
Typo, should be "successfully"
| { | ||
| PlanetId = planetId; | ||
| DroneId = droneId; | ||
| EntityId = entityId; | ||
| PlayerId = playerId; | ||
| Stage = stage; | ||
| Priority = priority; | ||
| EntityPos = new Float3(entityPos); |
There was a problem hiding this comment.
Should we be consistent and use .ToFloat3() instead?
This fix is for better synchronization of drones and it solves deadlocks that can occur due to the delay between host and client.
Two main situations were causing this:
Client sent a drone to build something and the drone will arrive and call "Build". However, we are not building it immediately on the client, we just sent a request to the host and wait for the reply, and then the building is built. So once the drone arrives, the "Build" is called, the request is sent and the drone is going back to the player. This causes that another drone was dispatched to the same building because, from the client's point of view, the building was not finished yet.
buildPreviewfor which client sent request to be build (another drone already reached it)Issue is with recycling the
prebuildIDs. When you placebuildPreviewit hasId = -1, when that building is constructed and you place anotherbuildPreview, it has againId = -1. So due to the delay, client sent a drone to the first build preview (id: -1) because he does not see it build yet. The host will receive info about it, but from the host point of theid: -1is for the secondbuildPreview, which the client does not see yet. In short: The client sent a drone to building A, but the host thinks that the drone is going to building B. And both buildings had the same ID.buildPreviewand we think.