feat: Implement multiplayer spawn system with position sync#10
Open
AvinadavCoh wants to merge 21 commits intoguilhermeljs:mainfrom
Open
feat: Implement multiplayer spawn system with position sync#10AvinadavCoh wants to merge 21 commits intoguilhermeljs:mainfrom
AvinadavCoh wants to merge 21 commits intoguilhermeljs:mainfrom
Conversation
Fixed critical bug where HandleClose was incorrectly invoking OnClientConnected instead of OnClientDisconnected. This would cause: - Improper cleanup of disconnected clients - Potential memory leaks from unclosed connections - Incorrect lobby state management - Events fired to wrong handlers on disconnect The HandleClose method should trigger disconnect events to properly clean up resources and notify other systems when a client leaves. Relates to issue guilhermeljs#5 (Integration)
Added null checks for GetPlayer() calls in three server handlers to prevent NullReferenceException when processing packets from unknown or disconnected clients: - PlayerMovePacketHandler: Check player exists before accessing UUID - PlayerRotatePacketHandler: Check player exists before accessing UUID - SelectCharacterPacketHandler: Early return if player not found, preventing null reference when creating response packet These checks improve robustness during network edge cases like: - Clients disconnecting mid-operation - Race conditions during lobby state changes - Malformed or delayed packets from stale connections Relates to issue guilhermeljs#5 (Integration) and guilhermeljs#6 (Performance)
Major Features: - Player spawn synchronization working across host and clients - Movement and rotation sync (packets ID 7, 8) - XP gain and level up sync with Harmony patches - Filtered logging system to handle packet spam Networking: - GameLoadedPacket now includes position/rotation data - Client sends spawn position to host on game load - Host spawns all players with correct positions (500ms delay for packet arrival) - Fixed Vector3.ToString() AccessViolationException in Il2Cpp logging - Added position fallback if client data not received Debug Tools: - DebugLogger: Spam-filtered logging (movement/rotation 1/sec, always log spawn/connection) - Log files: MultibonkLogs/Multibonk_[timestamp].log - Debug commands: F6 (XP), F7 (level up), F8 (status), F9 (drop), F10/F11 (enemy) Infrastructure Ready (awaiting game methods): - Enemy sync (health updates with 10% threshold, death packets) - Item drop sync (dropped/picked up packets) - Comprehensive event handlers and packet structure Multiplayer Tested: - Connection via Radmin VPN working (26.x.x.x IPs) - Host authority model functional - Client-server spawn confirmed working - Movement packets flowing correctly Known Issues: - Enemies not synced yet (need game methods via dnSpy) - Items not synced yet (need game methods via dnSpy)
There was a problem hiding this comment.
Pull Request Overview
This PR implements XP synchronization, item drop sync, and enemy health/death synchronization for the Multibonk multiplayer mod. Key infrastructure includes network packet handlers, event systems, debug commands, and comprehensive documentation for future implementation.
Key changes:
- Added XP gain and level-up network synchronization with packet handlers and event broadcasting
- Implemented enemy health/death sync with threshold-based optimization to reduce bandwidth
- Added item drop and pickup packet infrastructure (patches pending game method discovery)
Reviewed Changes
Copilot reviewed 37 out of 39 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| XP_SYNC_IMPLEMENTATION.md | Complete implementation guide for XP sync feature |
| Release/INSTALL.txt | Updated installation instructions with new features and debug commands |
| LobbyContext.cs | Added spawn position/rotation tracking for players |
| ServerProtocol.cs | Fixed bug: OnClientDisconnected was calling OnClientConnected |
| SelectCharacterPacketHandler.cs | Cleaned up null checks and removed extra whitespace |
| PlayerRotatePacketHandler.cs | Fixed variable shadowing by renaming loop variable |
| PlayerMovePacketHandler.cs | Fixed variable shadowing by renaming loop variable |
| GameLoadedPacketHandler.cs | Enhanced to store and use client spawn positions |
| SpawnPlayerPacketHandler.cs | Added debug logging for spawn operations |
| PlayerXpGainedPacketHandler.cs | New handler for XP gain notifications |
| PlayerLevelUpPacketHandler.cs | New handler for level-up notifications |
| ItemPickedUpPacketHandler.cs | New handler for item pickup sync |
| ItemDroppedPacketHandler.cs | New handler for item drop sync |
| EnemyHealthUpdatePacketHandler.cs | New handler with bandwidth optimization notes |
| EnemyDeathPacketHandler.cs | New handler for enemy death notifications |
| PacketId.cs | Added 6 new packet IDs for features |
| PlayerXpGainedPacket.cs | Packet definition for XP sync |
| PlayerLevelUpPacket.cs | Packet definition for level-up sync |
| ItemPickedUpPacket.cs | Packet definition for item pickup |
| ItemDroppedPacket.cs | Packet definition for item drops with position |
| GameLoadedPacket.cs | Enhanced to include spawn position/rotation |
| EnemyHealthUpdatePacket.cs | Packet with detailed optimization documentation |
| EnemyDeathPacket.cs | Packet for enemy death notifications |
| Multibonk.csproj | Updated library paths and downgraded dependency version |
| Multibonk.cs | Added null checks and registered new handlers |
| PlayerXpPatches.cs | Harmony patches for XP/level events |
| ItemDropPatches.cs | Template for item drop patches (commented) |
| EnemySyncPatches.cs | Template for enemy sync patches (commented) |
| PlayerXpEventHandler.cs | Broadcasts XP/level events to clients |
| ItemDropEventHandler.cs | Broadcasts item drop events |
| GameLoadedEventHandler.cs | Completely refactored spawn logic |
| EnemySyncEventHandler.cs | Threshold-based enemy health sync |
| GameFunctions.cs | Enhanced spawn function with better logging |
| GameEvents.cs | Added new event signatures for sync features |
| DebugCommands.cs | F-key debug commands for testing |
| DebugLogger.cs | Enhanced logging with file output and spam filtering |
| ENEMY_SYNC_OPTIMIZATION_GUIDE.md | Comprehensive optimization documentation |
| .gitignore | Added csproj to gitignore |
Comments suppressed due to low confidence (1)
Multibonk/Multibonk.csproj:1
- Adding *.csproj to .gitignore will prevent the project file itself from being tracked. This is problematic as the .csproj file is essential for building the project and should be version controlled. Consider using a more specific pattern if you need to ignore user-specific settings, or remove this line entirely.
<Project Sdk="Microsoft.NET.Sdk">
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+14
to
+16
| Position = new Vector3(msg.ReadFloat(), msg.ReadFloat(), msg.ReadFloat()); | ||
| Rotation = new Quaternion(msg.ReadFloat(), msg.ReadFloat(), msg.ReadFloat(), msg.ReadFloat()); | ||
| } |
There was a problem hiding this comment.
[nitpick] Consider extracting position and rotation reading into helper methods to avoid code duplication and improve readability. For example: 'Position = ReadVector3(msg);' and 'Rotation = ReadQuaternion(msg);'
Suggested change
| Position = new Vector3(msg.ReadFloat(), msg.ReadFloat(), msg.ReadFloat()); | |
| Rotation = new Quaternion(msg.ReadFloat(), msg.ReadFloat(), msg.ReadFloat(), msg.ReadFloat()); | |
| } | |
| Position = ReadVector3(msg); | |
| Rotation = ReadQuaternion(msg); | |
| } | |
| private static Vector3 ReadVector3(IncomingMessage msg) | |
| { | |
| return new Vector3(msg.ReadFloat(), msg.ReadFloat(), msg.ReadFloat()); | |
| } | |
| private static Quaternion ReadQuaternion(IncomingMessage msg) | |
| { | |
| return new Quaternion(msg.ReadFloat(), msg.ReadFloat(), msg.ReadFloat(), msg.ReadFloat()); | |
| } |
| var spawnPos = client.SpawnPosition; | ||
| var spawnRot = client.SpawnRotation; | ||
|
|
||
| if (spawnPos == Vector3.zero) |
There was a problem hiding this comment.
Using Vector3.zero as a sentinel value for 'position not set' is fragile since (0,0,0) could be a valid spawn position in some games. Consider using a nullable Vector3 wrapper or a separate boolean flag to track whether the position has been set.
Comment on lines
+100
to
+105
| float lastHealth = _lastKnownHealth[enemyId]; | ||
| float healthLost = lastHealth - currentHealth; | ||
| float percentLost = healthLost / maxHealth; | ||
|
|
||
| // Only broadcast if significant change (>10% of max health) | ||
| if (percentLost >= HEALTH_SYNC_THRESHOLD) |
There was a problem hiding this comment.
The threshold check doesn't handle health increases (healing). If currentHealth > lastHealth, percentLost will be negative and never trigger the threshold. Consider using Math.Abs(percentLost) or handle increases separately.
Comment on lines
104
to
123
| foreach (var otherPlayer in allPlayers) | ||
| { | ||
| if (otherPlayer.UUID == client.UUID) | ||
| continue; // Don't send spawn packet for themselves | ||
|
|
||
| var otherPos = otherPlayer.SpawnPosition; | ||
| var otherRot = otherPlayer.SpawnRotation; | ||
|
|
||
| // Use fallback if position not received | ||
| if (otherPos == Vector3.zero && MyPlayer.Instance != null) | ||
| { | ||
| otherPos = MyPlayer.Instance.transform.position; | ||
| otherRot = MyPlayer.Instance.transform.rotation; | ||
| } | ||
|
|
||
| var otherCharacter = Enum.Parse<ECharacter>(otherPlayer.SelectedCharacter); | ||
| var spawnPacket = new SendSpawnPlayerPacket(otherCharacter, otherPlayer.UUID, otherPos, otherRot); | ||
| client.Connection.EnqueuePacket(spawnPacket); | ||
| DebugLogger.LogSpawn($"Sent spawn packet to {client.Name} for player {otherPlayer.Name} at ({otherPos.x}, {otherPos.y}, {otherPos.z})"); | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
Comment on lines
+22
to
+28
| foreach (var player in lobbyContext.GetPlayers()) | ||
| { | ||
| if (player.Connection != null) | ||
| { | ||
| player.Connection.EnqueuePacket(packet); | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| } | ||
|
|
||
| // Track test boss health for F10 debug command | ||
| private static Dictionary<string, float> _testBossHealth = new Dictionary<string, float>(); |
There was a problem hiding this comment.
Field '_testBossHealth' can be 'readonly'.
Suggested change
| private static Dictionary<string, float> _testBossHealth = new Dictionary<string, float>(); | |
| private static readonly Dictionary<string, float> _testBossHealth = new Dictionary<string, float>(); |
| DebugLogger.LogSpawn("Finished spawning all players"); | ||
| } | ||
| catch (Exception ex) | ||
| { |
There was a problem hiding this comment.
Generic catch clause.
Suggested change
| { | |
| { | |
| // Rethrow critical exceptions | |
| if (ex is OutOfMemoryException || ex is StackOverflowException || ex is System.Threading.ThreadAbortException) | |
| throw; |
| GameDispatcher.Enqueue(() => SpawnAllPlayers(lobbyContext)); | ||
| } | ||
| catch (Exception ex) | ||
| { |
There was a problem hiding this comment.
Generic catch clause.
Suggested change
| { | |
| { | |
| if (ex is OutOfMemoryException || ex is StackOverflowException || ex is System.Threading.ThreadAbortException) | |
| throw; |
Comment on lines
+26
to
+29
| catch (System.Exception ex) | ||
| { | ||
| MelonLogger.Error($"Error in AfterAddXp patch: {ex}"); | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
Comment on lines
+50
to
+53
| catch (System.Exception ex) | ||
| { | ||
| MelonLogger.Error($"Error in AfterOnLevelUp patch: {ex}"); | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
- Added MapRevealEventHandler to handle broadcasting map tile reveals from host to clients. - Created BossSyncPatches for boss spawning, health, and death synchronization. - Implemented EnemySyncPatches for enemy health and death event synchronization. - Introduced MinimapSyncPatches for future minimap reveal synchronization. - Updated Multibonk.cs to register new event handlers and packet handlers. - Added EnemySpawnPacket and MapRevealPacket for network communication. - Developed client-side handlers for enemy spawn and map reveal packets. - Enhanced LobbyContext to track multiplayer state. - Updated README to reflect current synchronization features and status.
…in functionality - Added SteamFriendsReflection to locate Steamworks.SteamFriends API and ActivateGameOverlay method. - Created SteamTunnelService to manage Steam overlay availability and queue join endpoints. - Introduced SteamTunnelCallbackBinder to handle OnGameRichPresenceJoinRequested events and parse connect strings. - Enhanced UIManager to refresh Steam tunnel status and handle overlay requests. - Updated ConnectionWindow, HostLobbyWindow, and ClientLobbyWindow to include Steam overlay buttons and status messages. - Added OptionsWindow for gameplay settings, including Steam tunneling options. - Updated README.md and created STEAM_INTEGRATION.md for documentation on new features and usage.
… Steam tunnel status
…proved method detection and error handling
… creation fallback
…network connections, including debug logging
… testing, including UI integration
…de enemy spawning and synchronization
…awn handling, and add chest and shrine interaction logic
…nt handlers, packet definitions, and patches for multiplayer support
…luding network synchronization and client packet handling
…uding portal interaction and client packet processing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Multiplayer Spawn System Implementation
What's Working ✅
Technical Changes
Infrastructure Ready (awaiting game methods)
Testing
Tested successfully with 2 players via Radmin VPN. Players spawn correctly and can see each other's movements.
Files Changed
37 files changed, +2104 lines