Experimental Networking API revamp #179
Conversation
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
WalkthroughReplaces inline lambda-based networking registrations with a fluent MessageRouter API, converts many message/RPC mutators to constructor-based initialization, adds Server/Client ReceiverHandler classes and wiring, removes legacy RPC macros, and includes small utility/header additions (e.g., safe_win32). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client App
participant Net as NetworkPeer / MessageRouter
participant Inst as Instance (Client/Server)
participant World as World Modules / ReceiverHandler
Note over Client,Net: Messages/RPCs constructed via parameterized ctors
Client->>Net: send<T>(guid, ctor-args)
Net->>Net: construct T from args
Net->>Inst: router dispatch -> bound handler (e.g., OnClientHandshake / OnSetTransform)
Inst->>World: call ReceiverHandler / World modules for spawn/update/emit
alt broadcast required
World->>Net: send/sendGameRPC helpers -> network dispatch
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.{cpp,cc,cxx,h,hpp,hxx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (6)📓 Common learnings📚 Learning: 2025-12-20T18:15:01.266ZApplied to files:
📚 Learning: 2025-12-20T18:15:01.266ZApplied to files:
📚 Learning: 2025-12-20T18:15:01.266ZApplied to files:
📚 Learning: 2025-12-29T09:38:01.239ZApplied to files:
📚 Learning: 2025-12-29T09:34:41.781ZApplied to files:
🧬 Code graph analysis (1)code/framework/src/integrations/server/scripting/builtins/entity.cpp (1)
🪛 Cppcheck (2.19.0)code/framework/src/integrations/server/scripting/builtins/entity.cpp[error] 101-101: failed to evaluate #if condition (syntaxError) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
code/framework/src/integrations/server/scripting/builtins/entity.cpp (1)
9-9: Remove#pragma oncefrom .cpp file.
#pragma onceis a header guard directive and should only be used in header files (.h,.hpp). Using it in a.cppfile is incorrect and may cause unexpected behavior or compiler warnings.🔎 Remove the pragma directive
-#pragma once - #include "entity.h"code/framework/src/integrations/server/scripting/builtins/events_lua.cpp (1)
9-9: Remove#pragma oncefrom.cppfile.
#pragma onceis a header guard and should only be used in header files (.h,.hpp). Its presence in a.cppimplementation file is incorrect and could cause confusion, though it won't affect compilation in most cases.🔎 Proposed fix
-#pragma once - #include "events_lua.h"code/framework/src/world/modules/modules_impl.cpp (1)
9-9: Remove#pragma oncefrom implementation file.This file is compiled as a translation unit (listed in CMakeLists.txt) and should not contain
#pragma once. Header guards are only appropriate for.h,.hpp, or.hxxfiles. Remove line 9.
🧹 Nitpick comments (13)
code/tests/modules/state_machine_ut.h (1)
187-196: Consider verifying a minimum success rate.The conditional tracking and explanatory comments improve the test's realism for concurrent scenarios. However, the test now only verifies that at least one request succeeded, which is a very minimal check.
In a properly functioning state machine, most concurrent requests should succeed. Consider asserting a minimum threshold to catch performance degradation or locking issues.
🔎 Suggested enhancement
requester.join(); running = false; updater.join(); - // Verify thread safety: no crashes and some transitions occurred - EQUALS(successfulRequests.load() > 0, true); + // Verify thread safety: no crashes and reasonable success rate + // With 200 total requests, expect at least 50% to succeed + EQUALS(successfulRequests.load() > 100, true); EQUALS(ProcessingState::GetCounter() > 0, true);Alternatively, verify the relationship between successful requests and state counter:
// Verify thread safety: no crashes and some transitions occurred EQUALS(successfulRequests.load() > 0, true); EQUALS(ProcessingState::GetCounter() > 0, true); + // State 2 entries should be roughly half of successful requests + EQUALS(ProcessingState::GetCounter() <= successfulRequests.load(), true);code/framework/src/networking/network_client.h (1)
95-102: Naming inconsistency:sendGameRPCvsSendGameRPC.The new method uses camelCase (
sendGameRPC) while the existing method uses PascalCase (SendGameRPC). Consider renaming toSendGameRPCwith a different signature or a more distinct name likeSendEntityGameRPCto maintain consistency.🔎 Suggested rename for consistency
- // Fluent send method for Game RPCs - template <typename T, typename... Args> - bool sendGameRPC(flecs::entity ent, Args&&... args) { + // Fluent send method for Game RPCs by entity + template <typename T, typename... Args> + bool SendEntityGameRPC(flecs::entity ent, Args&&... args) { T rpc(std::forward<Args>(args)...); rpc.SetServerID(ent.id()); return SendGameRPC(rpc); }code/framework/src/networking/messages/client_kick.h (1)
15-15: Unused include:<flecs/flecs.h>appears unnecessary in this file.The flecs header doesn't seem to be used in this message class. Consider removing it to reduce compilation dependencies.
🔎 Remove unused include
#include <BitStream.h> - -#include <flecs/flecs.h>code/framework/src/integrations/server/scripting/builtins/entity.cpp (2)
220-232: Redundant streamer component fetch inKick().The
Streamercomponent is fetched twice: first asst(line 221), then again asstreamer(line 224). Additionally, thenetvariable is fetched using inlinereinterpret_castinstead of theGetNetworkServer()helper defined above.🔎 Simplify redundant code
void Entity::Kick(const std::string &reason) { - const auto st = _ent.get<Framework::World::Modules::Base::Streamer>(); - if (st) { - auto net = reinterpret_cast<Framework::Networking::NetworkServer *>(Framework::CoreModules::GetNetworkPeer()); - const auto streamer = _ent.get<Framework::World::Modules::Base::Streamer>(); - if (net && streamer) { + const auto streamer = _ent.get<Framework::World::Modules::Base::Streamer>(); + if (streamer) { + if (auto net = GetNetworkServer()) { Framework::Networking::Messages::ClientKick kick(Framework::Networking::Messages::DisconnectionReason::KICKED_CUSTOM, reason); net->Send(kick, streamer->guid); net->GetPeer()->CloseConnection(SLNet::RakNetGUID(streamer->guid), true); - return; } } }
43-61: Inconsistent ordering ofWakeEntity()call.In
SetPosition(),WakeEntity()is called before the network send (line 47), but inSetRotation()it's called after (line 60). Consider consistent ordering across these methods for maintainability.code/framework/src/integrations/shared/rpc/emit_lua_event.h (1)
45-45: Add namespace closing comment for consistency.Per coding guidelines, C++ files should use the pattern
} // namespace Framework::Integrations::Shared::RPCfor namespace closings.🔎 Proposed fix
-} +} // namespace Framework::Integrations::Shared::RPCcode/framework/src/world/client.cpp (2)
117-127: Consider marking unused parameter or documenting its purpose.The
guidparameter is unused in this handler. While it may be required by the router signature, consider adding(void)guid;for clarity, as seen in similar handlers elsewhere in the codebase.🔎 Proposed fix
void ClientEngine::OnSetTransform(SLNet::RakNetGUID guid, RPC::SetTransform *msg) { + (void)guid; // Unused parameter if (!msg->Valid()) { return; }
129-139: Consider marking unused parameter or documenting its purpose.Same as
OnSetTransform- theguidparameter is unused. Adding(void)guid;would improve consistency with similar handlers incode/framework/src/integrations/client/instance.cpp.🔎 Proposed fix
void ClientEngine::OnSetFrame(SLNet::RakNetGUID guid, RPC::SetFrame *msg) { + (void)guid; // Unused parameter if (!msg->Valid()) { return; }code/framework/src/integrations/server/instance.cpp (1)
255-257: Shadowed variablenetin lambda.The lambda captures
thisand then re-declaresneton line 256, which shadows the outernetvariable from line 243. While functionally correct (both resolve to the same object), this creates unnecessary redundancy.🔎 Suggested simplification
- net->SetOnPlayerDisconnectCallback([this](SLNet::Packet *packet, uint32_t reason) { - const auto net = _networkingEngine->GetNetworkServer(); + net->SetOnPlayerDisconnectCallback([this, net](SLNet::Packet *packet, uint32_t reason) { const auto guid = packet->guid;code/framework/src/world/modules/modules_impl.cpp (2)
126-132: Redundantadd<Transform>call.
SetupClient(viaSetupDefaultsat line 17-25 in streaming.hpp) already adds theTransformcomponent. The explicitadd<Transform>on line 129 is redundant.🔎 Simplified code
const auto e = _worldEngine->CreateEntity(msg->GetServerID()); _streamingFactory->SetupClient(e, SLNet::UNASSIGNED_RAKNET_GUID.g); - e.add<World::Modules::Base::Transform>(); const auto tr = e.get_mut<World::Modules::Base::Transform>(); *tr = msg->GetTransform();
183-196: Placeholder handler noted.
OnEntitySelfUpdatevalidates the message but performs no action. Consider adding a TODO comment if functionality is planned, or document why this handler exists.code/framework/src/world/modules/base.hpp (2)
47-54: Consider markingServerReceiverHandlerasfinal.Per coding guidelines, classes should be marked
finalwhen possible. IfServerReceiverHandleris not intended to be inherited, consider adding thefinalspecifier.🔎 Optional change
- class ServerReceiverHandler { + class ServerReceiverHandler final { Engine *_worldEngine;
60-74: Consider markingClientReceiverHandlerasfinal.Similar to
ServerReceiverHandler, ifClientReceiverHandleris not intended to be inherited, consider adding thefinalspecifier per coding guidelines.🔎 Optional change
- class ClientReceiverHandler { + class ClientReceiverHandler final { ClientEngine *_worldEngine;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
code/framework/src/external/sentry/wrapper.cppcode/framework/src/integrations/client/instance.cppcode/framework/src/integrations/client/instance.hcode/framework/src/integrations/client/scripting/builtins/events_lua.hcode/framework/src/integrations/server/instance.cppcode/framework/src/integrations/server/instance.hcode/framework/src/integrations/server/scripting/builtins/entity.cppcode/framework/src/integrations/server/scripting/builtins/events_lua.cppcode/framework/src/integrations/shared/rpc/emit_lua_event.hcode/framework/src/networking/messages/client_connection_finalized.hcode/framework/src/networking/messages/client_handshake.hcode/framework/src/networking/messages/client_initialise_player.hcode/framework/src/networking/messages/client_kick.hcode/framework/src/networking/messages/client_ready_assets.hcode/framework/src/networking/messages/client_request_streamer.hcode/framework/src/networking/messages/game_sync/entity_despawn.hcode/framework/src/networking/messages/game_sync/entity_owner_update.hcode/framework/src/networking/messages/game_sync/entity_self_update.hcode/framework/src/networking/messages/game_sync/entity_spawn.hcode/framework/src/networking/messages/game_sync/entity_update.hcode/framework/src/networking/network_client.hcode/framework/src/networking/network_peer.hcode/framework/src/networking/network_server.hcode/framework/src/world/client.cppcode/framework/src/world/client.hcode/framework/src/world/engine.hcode/framework/src/world/game_rpc/set_frame.hcode/framework/src/world/game_rpc/set_transform.hcode/framework/src/world/modules/base.hppcode/framework/src/world/modules/modules_impl.cppcode/framework/src/world/server.hcode/tests/modules/state_machine_ut.h
💤 Files with no reviewable changes (1)
- code/framework/src/world/server.h
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cc,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,cc,cxx,h,hpp,hxx}: Use.clang-format(LLVM-based) for code formatting - runscripts/format_codebase.shto format
Use_prefix for private member variables with camelCase naming in C++
Use PascalCase for class names in C++ and mark classes asfinalwhen possible
Usenamespace Framework::SubModule {with closing comment} // namespace Framework::SubModulepattern in C++
UseFW_SEND_COMPONENT_RPC(rpc, ...)andFW_SEND_COMPONENT_RPC_TO(rpc, guid, ...)macros for network communication
Access core framework systems viaFramework::CoreModules::Get*()static methods for module registration
UsePlayerFactoryandStreamingFactoryfor entity creation in the ECS system
Files:
code/framework/src/integrations/shared/rpc/emit_lua_event.hcode/framework/src/networking/messages/game_sync/entity_spawn.hcode/framework/src/networking/messages/game_sync/entity_update.hcode/framework/src/networking/messages/client_ready_assets.hcode/framework/src/networking/messages/client_connection_finalized.hcode/framework/src/integrations/client/scripting/builtins/events_lua.hcode/framework/src/networking/network_client.hcode/framework/src/integrations/server/scripting/builtins/events_lua.cppcode/framework/src/networking/messages/client_handshake.hcode/framework/src/networking/network_server.hcode/tests/modules/state_machine_ut.hcode/framework/src/world/game_rpc/set_transform.hcode/framework/src/networking/messages/client_kick.hcode/framework/src/world/game_rpc/set_frame.hcode/framework/src/networking/messages/game_sync/entity_despawn.hcode/framework/src/external/sentry/wrapper.cppcode/framework/src/world/client.cppcode/framework/src/networking/messages/client_initialise_player.hcode/framework/src/integrations/server/instance.hcode/framework/src/networking/messages/game_sync/entity_self_update.hcode/framework/src/integrations/client/instance.hcode/framework/src/integrations/server/scripting/builtins/entity.cppcode/framework/src/integrations/client/instance.cppcode/framework/src/world/engine.hcode/framework/src/world/client.hcode/framework/src/networking/network_peer.hcode/framework/src/world/modules/modules_impl.cppcode/framework/src/world/modules/base.hppcode/framework/src/networking/messages/game_sync/entity_owner_update.hcode/framework/src/integrations/server/instance.cppcode/framework/src/networking/messages/client_request_streamer.h
**/*.{h,hpp,hxx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
#pragma oncefor header guards in C++ files
Files:
code/framework/src/integrations/shared/rpc/emit_lua_event.hcode/framework/src/networking/messages/game_sync/entity_spawn.hcode/framework/src/networking/messages/game_sync/entity_update.hcode/framework/src/networking/messages/client_ready_assets.hcode/framework/src/networking/messages/client_connection_finalized.hcode/framework/src/integrations/client/scripting/builtins/events_lua.hcode/framework/src/networking/network_client.hcode/framework/src/networking/messages/client_handshake.hcode/framework/src/networking/network_server.hcode/tests/modules/state_machine_ut.hcode/framework/src/world/game_rpc/set_transform.hcode/framework/src/networking/messages/client_kick.hcode/framework/src/world/game_rpc/set_frame.hcode/framework/src/networking/messages/game_sync/entity_despawn.hcode/framework/src/networking/messages/client_initialise_player.hcode/framework/src/integrations/server/instance.hcode/framework/src/networking/messages/game_sync/entity_self_update.hcode/framework/src/integrations/client/instance.hcode/framework/src/world/engine.hcode/framework/src/world/client.hcode/framework/src/networking/network_peer.hcode/framework/src/world/modules/base.hppcode/framework/src/networking/messages/game_sync/entity_owner_update.hcode/framework/src/networking/messages/client_request_streamer.h
🧠 Learnings (3)
📚 Learning: 2025-12-20T18:15:01.266Z
Learnt from: CR
Repo: MafiaHub/Framework PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T18:15:01.266Z
Learning: Applies to **/*.{cpp,cc,cxx,h,hpp,hxx} : Use `FW_SEND_COMPONENT_RPC(rpc, ...)` and `FW_SEND_COMPONENT_RPC_TO(rpc, guid, ...)` macros for network communication
Applied to files:
code/framework/src/networking/network_client.hcode/framework/src/networking/network_server.hcode/framework/src/world/client.cppcode/framework/src/integrations/client/instance.hcode/framework/src/integrations/server/scripting/builtins/entity.cppcode/framework/src/world/engine.hcode/framework/src/world/client.hcode/framework/src/networking/network_peer.h
📚 Learning: 2025-12-20T18:15:01.266Z
Learnt from: CR
Repo: MafiaHub/Framework PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T18:15:01.266Z
Learning: Applies to **/*.{cpp,cc,cxx,h,hpp,hxx} : Use `PlayerFactory` and `StreamingFactory` for entity creation in the ECS system
Applied to files:
code/framework/src/integrations/server/instance.hcode/framework/src/integrations/client/instance.hcode/framework/src/integrations/server/scripting/builtins/entity.cppcode/framework/src/world/modules/base.hpp
📚 Learning: 2025-12-20T18:15:01.266Z
Learnt from: CR
Repo: MafiaHub/Framework PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T18:15:01.266Z
Learning: Applies to **/*.{cpp,cc,cxx,h,hpp,hxx} : Access core framework systems via `Framework::CoreModules::Get*()` static methods for module registration
Applied to files:
code/framework/src/world/engine.h
🧬 Code graph analysis (10)
code/framework/src/networking/network_client.h (2)
code/framework/src/networking/rpc/game_rpc.h (1)
bool(49-57)code/framework/src/networking/rpc/rpc.h (1)
bool(32-40)
code/framework/src/networking/network_server.h (3)
code/framework/src/networking/rpc/game_rpc.h (2)
SetServerID(36-38)bool(49-57)code/framework/src/networking/network_client.h (1)
Framework(24-104)code/framework/src/networking/messages/messages.h (1)
Framework(18-103)
code/framework/src/world/game_rpc/set_transform.h (1)
code/framework/src/world/modules/base.hpp (2)
tr(94-96)tr(94-94)
code/framework/src/world/client.cpp (1)
code/framework/src/integrations/client/instance.cpp (2)
OnSetTransform(397-409)OnSetTransform(397-397)
code/framework/src/integrations/server/instance.h (2)
code/framework/src/integrations/server/instance.cpp (10)
InitNetworkingMessages(242-276)InitNetworkingMessages(242-242)OnClientHandshake(278-338)OnClientHandshake(278-278)OnClientRequestStreamer(340-362)OnClientRequestStreamer(340-340)OnClientInitPlayer(364-369)OnClientInitPlayer(364-364)OnEmitLuaEvent(371-391)OnEmitLuaEvent(371-371)code/framework/src/world/modules/base.hpp (2)
ServerReceiverHandler(51-51)ServerReceiverHandler(51-51)
code/framework/src/integrations/client/instance.h (6)
code/framework/src/world/client.h (2)
Framework(19-22)Framework(24-60)code/framework/src/world/server.h (1)
Framework(22-57)code/framework/src/networking/messages/messages.h (1)
Framework(18-103)code/framework/src/world/modules/base.hpp (7)
ClientReceiverHandler(65-67)guid(53-53)guid(69-69)guid(70-70)guid(71-71)guid(72-72)guid(73-73)code/framework/src/world/client.cpp (2)
OnSetTransform(117-127)OnSetTransform(117-117)code/framework/src/integrations/server/instance.cpp (2)
OnEmitLuaEvent(371-391)OnEmitLuaEvent(371-371)
code/framework/src/world/client.h (4)
code/framework/src/world/game_rpc/set_transform.h (1)
Framework(14-37)code/framework/src/world/client.cpp (6)
InitRPCs(111-115)InitRPCs(111-111)OnSetTransform(117-127)OnSetTransform(117-117)OnSetFrame(129-139)OnSetFrame(129-129)code/framework/src/core_modules.h (1)
NetworkPeer(80-82)code/framework/src/integrations/client/instance.cpp (2)
OnSetTransform(397-409)OnSetTransform(397-397)
code/framework/src/networking/network_peer.h (1)
code/framework/src/networking/network_peer.cpp (2)
NetworkPeer(18-33)NetworkPeer(35-37)
code/framework/src/world/modules/modules_impl.cpp (2)
code/framework/src/world/modules/base.hpp (10)
tr(94-96)tr(94-94)guid(53-53)guid(69-69)guid(70-70)guid(71-71)guid(72-72)guid(73-73)streamable(236-236)streamable(237-237)code/framework/src/world/types/streaming.hpp (6)
e(18-26)e(18-18)e(29-37)e(29-29)e(39-44)e(39-39)
code/framework/src/networking/messages/client_request_streamer.h (1)
code/framework/src/utils/hardware_id.h (1)
string(25-49)
🔇 Additional comments (43)
code/framework/src/external/sentry/wrapper.cpp (1)
9-9: Verify necessity and ordering of the new include.The include
<utils/safe_win32.h>appears before the associated headerwrapper.h(line 11), which violates typical C++ include ordering conventions (associated header should be first). Additionally, no visible usage of symbols from this header is found in the file—it may be an unused include or indirectly required through other headers.Please verify:
- Whether
<utils/safe_win32.h>is actually used in this file or indirectly required through included headers- If it is needed, reorder includes to place
wrapper.hfirst (standard C++ convention)- Whether this include relates to the networking API revamp in this PR or is a separate change
code/tests/modules/state_machine_ut.h (1)
173-173: Good addition for concurrent test tracking.The atomic counter appropriately tracks successful state transition requests in the concurrent scenario, ensuring thread-safe increments from the requester thread.
code/framework/src/networking/messages/game_sync/entity_self_update.h (1)
18-33: LGTM!The addition of the default constructor aligns with the broader API refactoring pattern in this PR. The class follows coding guidelines: marked
final, uses#pragma once, and follows proper namespace conventions.code/framework/src/networking/messages/game_sync/entity_owner_update.h (1)
22-25: LGTM!The parameterized constructor cleanly replaces the
FromParameterspattern. The default constructor correctly initializes_ownerto the unassigned GUID, andValid()properly validates this condition.code/framework/src/networking/messages/client_connection_finalized.h (1)
24-28: LGTM!The constructors are well-implemented with proper initializer lists. The default-constructed state correctly fails validation, which is appropriate for messages that will be populated via deserialization.
code/framework/src/networking/messages/client_kick.h (1)
24-28: LGTM!The parameterized constructor cleanly replaces
FromParameters. The initialization properly convertsstd::stringtoSLNet::RakString, andValid()correctly enforces that custom kicks require a reason.code/framework/src/networking/messages/client_request_streamer.h (1)
24-30: LGTM!The parameterized constructor cleanly initializes all fields with proper
std::stringtoRakStringconversion. The optionalplayerHardwareIdparameter with empty default is appropriate for backward compatibility.code/framework/src/integrations/server/scripting/builtins/entity.cpp (1)
23-31: Helper functions for accessing global singletons.The anonymous namespace helpers centralize access to network/engine singletons. The
reinterpret_castusage is acceptable here given the known type hierarchy and thatCoreModules::GetNetworkPeer()returns the appropriate derived type on server builds. Based on learnings, accessing core systems viaFramework::CoreModules::Get*()static methods is the recommended pattern.code/framework/src/world/engine.h (1)
14-15: Macro migration is complete.The removal of
FW_SEND_COMPONENT_RPCandFW_SEND_COMPONENT_RPC_TOmacros has been successfully migrated throughout the codebase. No remaining usages of these macros were found, indicating that all call sites have been transitioned to the new router-based API.code/framework/src/networking/network_peer.h (3)
24-97: Well-designed fluent router API.The binder pattern provides a clean, type-safe registration mechanism. The lifetime requirements are properly documented in the comments. The separation of
MessageBinder,RPCBinder, andGameRPCBinderaligns well with the existingRegisterMessage,RegisterRPC, andRegisterGameRPCmethods.
231-251: Clean fluent send API with proper perfect forwarding.The templated
sendandsendRPCmethods correctly usestd::forwardfor argument forwarding and delegate to the existingSend/SendRPCimplementations. This provides a more ergonomic API while maintaining consistency with the underlying behavior.
256-280: LGTM - Binder implementations correctly delegate to existing registration methods.The implementations properly capture the instance pointer and method pointer by value. The lifetime contract (instance must outlive NetworkPeer) is documented in the class comments.
code/framework/src/networking/messages/game_sync/entity_despawn.h (1)
20-21: LGTM - Explicit default constructor supports router-based instantiation.The explicit default constructor aligns with the broader refactor and enables the fluent API to instantiate message types during registration.
code/framework/src/networking/messages/client_initialise_player.h (1)
20-21: LGTM - Consistent with the constructor-based pattern across message types.code/framework/src/integrations/client/scripting/builtins/events_lua.h (1)
28-28: LGTM - Clean transition to parameterized constructor.The change from
FromParametersto direct constructor initialization is consistent with the PR's goal of simplifying RPC creation.code/framework/src/networking/network_server.h (1)
55-77: Well-structured fluent API for server-side Game RPCs.The three variants (
sendGameRPC,sendGameRPCTo,sendGameRPCExcept) provide clear semantics for broadcast, targeted, and exclusion patterns. Perfect forwarding is correctly applied, and the server ID is properly set from the entity before dispatch.code/framework/src/integrations/server/scripting/builtins/events_lua.cpp (1)
26-27: LGTM - Constructor-based RPC creation is clean and correct.code/framework/src/networking/messages/client_ready_assets.h (1)
80-83: LGTM - Constructor pattern correctly initializes RakString from std::string.The parameterized constructor properly converts
std::stringtoSLNet::RakStringviac_str(). The default constructor supports deserialization.code/framework/src/world/game_rpc/set_transform.h (1)
20-23: LGTM - Constructor-based initialization replaces FromParameters cleanly.The parameterized constructor correctly copies the transform. This aligns with the broader refactor to constructor-based RPC/message initialization.
code/framework/src/world/game_rpc/set_frame.h (1)
20-23: LGTM! Constructor-based initialization improves type safety.The refactoring from
FromParametersto explicit constructors strengthens initialization semantics and makes the API clearer. The initializer list is correctly used.code/framework/src/networking/messages/game_sync/entity_update.h (1)
24-28: LGTM! Consistent constructor pattern.The parameterized constructor correctly initializes both members using an initializer list in the proper order.
code/framework/src/integrations/shared/rpc/emit_lua_event.h (1)
22-26: LGTM! Constructor correctly handles string conversion.The parameterized constructor properly converts
std::stringtoSLNet::RakStringviac_str().code/framework/src/networking/messages/game_sync/entity_spawn.h (1)
23-26: LGTM! Clean constructor implementation.The parameterized constructor follows the established pattern and correctly initializes the transform member.
code/framework/src/networking/messages/client_handshake.h (1)
24-30: LGTM! All handshake parameters properly initialized.The parameterized constructor correctly converts all four string parameters to
SLNet::RakStringand maintains proper member initialization order.code/framework/src/world/client.h (2)
18-22: LGTM! Clean forward declaration pattern.The forward declarations properly scope RPC types and include the namespace closing comment per coding guidelines.
35-39: LGTM! Handler method declarations follow proper conventions.The change from
constto non-const forInitRPCsis appropriate since router registration may modify internal state. Handler method signatures are consistent with the router-based pattern.code/framework/src/world/client.cpp (1)
111-115: LGTM! Router-based RPC registration is clean.The fluent router API properly registers handlers using member function pointers.
code/framework/src/integrations/server/instance.h (3)
36-45: LGTM! Well-organized forward declarations.The forward declarations are properly scoped within their respective namespaces and include closing comments per coding guidelines.
118-131: LGTM! Handler method declarations follow consistent patterns.The signature change to non-const for
InitNetworkingMessagesis appropriate, and all handler methods follow the established naming convention and signature pattern.
140-141: LGTM! Proper ownership semantics with unique_ptr.The
_serverReceiverHandlermember usesstd::unique_ptrfor clear ownership and follows the_camelCasenaming convention for private members per coding guidelines.code/framework/src/integrations/client/instance.h (3)
40-53: LGTM! Well-organized forward declarations.The forward declarations are properly grouped by namespace with correct closing comments, following the coding guidelines pattern.
133-135: Good addition of the client receiver handler.The
_clientReceiverHandlermember follows the established naming convention and usesstd::unique_ptrfor proper ownership semantics.
150-155: Consistent handler method signatures.The private network message handlers follow a consistent pattern with
(SLNet::RakNetGUID guid, MessageType *msg)signatures, matching the router API expectations.code/framework/src/integrations/server/instance.cpp (3)
278-338: Well-structured handshake validation.The handshake handler properly validates all required fields (game name, framework version, client version, game version) with appropriate error logging and kick messages for each failure case.
340-362: Good use of entity factories.The handler correctly uses
StreamingFactoryandPlayerFactoryfor entity creation, following the coding guidelines for ECS entity creation.
371-391: Consistent Lua event handling with proper error handling.The exception handling for JSON parsing is appropriate, and the implementation matches the expected pattern for invoking global events through the resource manager.
code/framework/src/world/modules/modules_impl.cpp (2)
93-115: Solid entity update validation.The server-side update handler properly validates ownership and generation ID, preventing unauthorized updates and out-of-order packet processing.
198-212: Clean router-based receiver setup.The
SetupServerReceiversandSetupClientReceiversfunctions provide a clean abstraction for wiring message handlers via the fluent router API.code/framework/src/integrations/client/instance.cpp (3)
340-358: Clean resource list handling.The handler correctly stores the resource list in
_pendingServerResourcesto survive scripting module resets, then triggers the asset download flow.
360-379: Proper connection finalization flow.The handler correctly creates the player entity using the factories, sends the init player message, and invokes the connection finalized callback with the entity and tick rate.
288-308: Well-organized router registrations.The message, GameRPC, and RPC handlers are clearly grouped with descriptive comments, making the registration flow easy to follow.
code/framework/src/world/modules/base.hpp (2)
22-29: Complete forward declarations for message types.All required game sync message types are properly forward-declared within the
Framework::Networking::Messagesnamespace, enabling the handler method declarations without requiring full includes.
236-239: Updated API signatures support handler-based wiring.The
SetupServerReceiversandSetupClientReceiverssignatures now accept handler pointers, enabling the router-based message dispatch pattern introduced in this PR.
|
@coderabbitai Do a full review of this PR from the first commit |
|
@Segfaultd, I'll perform a full review of this PR from the first commit. This is a substantial networking API refactoring that includes:
I'll review all the changes thoroughly and provide detailed feedback. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/framework/src/world/modules/modules_impl.cpp (1)
9-9: Remove#pragma oncefrom implementation file.
#pragma onceis a header guard and should not be used in.cppfiles. Implementation files should not have header guards as they are never included.Suggested fix
-#pragma once
♻️ Duplicate comments (1)
code/framework/src/integrations/client/instance.cpp (1)
534-534: Placeholder IDs remain - previously flagged and marked out of scope.The hardcoded
"MY_SUPER_ID_1"and"MY_SUPER_ID_2"values were flagged in a previous review and acknowledged as out of scope for this PR.
🧹 Nitpick comments (13)
code/tests/modules/state_machine_ut.h (1)
187-196: Consider strengthening the success threshold.The approach of conditionally tracking successful requests is correct for concurrent scenarios. However, the test has become very permissive—it only requires at least 1 successful request out of 200 attempts (100 iterations × 2 calls per iteration).
A properly functioning state machine should achieve a much higher success rate even under concurrent access. Consider adding a minimum threshold, such as:
// Expect at least 50% success rate under concurrent load EQUALS(successfulRequests.load() >= 100, true);This would still account for expected concurrent failures while ensuring the state machine performs reasonably well.
code/framework/src/integrations/shared/rpc/emit_lua_event.h (1)
45-45: Add namespace closing comment.Per coding guidelines, namespace closing braces should include a comment.
Proposed fix
-} +} // namespace Framework::Integrations::Shared::RPCcode/framework/src/integrations/server/scripting/builtins/events_lua.cpp (1)
9-9: Consider removing#pragma oncefrom implementation file.
#pragma onceis a header guard intended for.h/.hppfiles. While harmless in a.cppfile (since implementation files typically aren't#included), it's unconventional and may cause confusion.code/framework/src/networking/messages/game_sync/entity_update.h (1)
24-28: LGTM!The constructor pattern is consistent with other message types in this PR. Member initialization is correct.
Optional: Consider passing
Transformbyconstreference to avoid a copy:- GameSyncEntityUpdate(World::Modules::Base::Transform transform, uint64_t owner) - : _transform(transform) + GameSyncEntityUpdate(const World::Modules::Base::Transform &transform, uint64_t owner) + : _transform(transform)This is a minor optimization and may not matter if
Transformis a small POD type.code/framework/src/networking/messages/client_kick.h (1)
15-15: Remove unused#include <flecs/flecs.h>include.This include is not referenced anywhere in the file or its dependencies. Removing it reduces unnecessary compilation overhead.
code/framework/src/networking/network_client.h (1)
95-102: Consider consistent method naming and exposing GUID parameter.The new fluent helper provides convenient RPC construction, but note:
- Naming inconsistency:
sendGameRPCuses camelCase while the existing methodSendGameRPCuses PascalCase. Consider aligning with the existing convention.- Limited flexibility: The fluent method always uses the default GUID parameter from
SendGameRPC. If callers need to specify a target GUID, they must fall back to the old API.🔎 Optional refactor for naming consistency
// Fluent send method for Game RPCs template <typename T, typename... Args> -bool sendGameRPC(flecs::entity ent, Args&&... args) { +bool SendGameRPC(flecs::entity ent, Args&&... args) { T rpc(std::forward<Args>(args)...); rpc.SetServerID(ent.id()); return SendGameRPC(rpc); }code/framework/src/networking/network_server.h (1)
55-77: Consider consistent method naming.The three new fluent helpers provide convenient RPC construction and routing variants (broadcast, targeted, excluded). However, note the naming inconsistency:
sendGameRPC,sendGameRPCTo, andsendGameRPCExceptuse camelCase while the existing methodSendGameRPCuses PascalCase. Consider aligning with the existing PascalCase convention for consistency across the API surface.🔎 Optional refactor for naming consistency
// Fluent send methods for Game RPCs template <typename T, typename... Args> -bool sendGameRPC(Framework::World::ServerEngine* world, flecs::entity ent, Args&&... args) { +bool SendGameRPC(Framework::World::ServerEngine* world, flecs::entity ent, Args&&... args) { T rpc(std::forward<Args>(args)...); rpc.SetServerID(ent.id()); return SendGameRPC(world, rpc); } template <typename T, typename... Args> -bool sendGameRPCTo(Framework::World::ServerEngine* world, SLNet::RakNetGUID guid, +bool SendGameRPCTo(Framework::World::ServerEngine* world, SLNet::RakNetGUID guid, flecs::entity ent, Args&&... args) { T rpc(std::forward<Args>(args)...); rpc.SetServerID(ent.id()); return SendGameRPC(world, rpc, guid); } template <typename T, typename... Args> -bool sendGameRPCExcept(Framework::World::ServerEngine* world, SLNet::RakNetGUID exceptGuid, +bool SendGameRPCExcept(Framework::World::ServerEngine* world, SLNet::RakNetGUID exceptGuid, flecs::entity ent, Args&&... args) { T rpc(std::forward<Args>(args)...); rpc.SetServerID(ent.id()); return SendGameRPC(world, rpc, SLNet::UNASSIGNED_RAKNET_GUID, exceptGuid); }code/framework/src/world/client.cpp (2)
117-127: Mark unused parameter explicitly.The
guidparameter is unused in this handler. Consider explicitly marking it as unused for clarity.🔎 Proposed fix
void ClientEngine::OnSetTransform(SLNet::RakNetGUID guid, RPC::SetTransform *msg) { + (void)guid; // Unused parameter if (!msg->Valid()) { return; }
129-139: Mark unused parameter explicitly.The
guidparameter is unused in this handler. Consider explicitly marking it as unused for clarity.🔎 Proposed fix
void ClientEngine::OnSetFrame(SLNet::RakNetGUID guid, RPC::SetFrame *msg) { + (void)guid; // Unused parameter if (!msg->Valid()) { return; }code/framework/src/world/modules/base.hpp (2)
47-54: Consider marking handler classes asfinal.Per coding guidelines, classes should be marked as
finalwhen possible. SinceServerReceiverHandleris not intended for inheritance:Suggested change
- class ServerReceiverHandler { + class ServerReceiverHandler final {
60-74: Consider markingClientReceiverHandlerasfinal.Same consideration as
ServerReceiverHandler- mark asfinalif not intended for inheritance.Suggested change
- class ClientReceiverHandler { + class ClientReceiverHandler final {code/framework/src/world/modules/modules_impl.cpp (1)
129-131: Redundant Transform component addition.Line 129 adds a
Transformcomponent, then line 130-131 gets a mutable reference and assigns. This could be simplified usingensure<Transform>()which adds-if-missing and returns mutable reference.Suggested refactor
- e.add<World::Modules::Base::Transform>(); - const auto tr = e.get_mut<World::Modules::Base::Transform>(); - *tr = msg->GetTransform(); + auto &tr = e.ensure<World::Modules::Base::Transform>(); + tr = msg->GetTransform();code/framework/src/integrations/server/instance.cpp (1)
254-269: Consider extracting repeatednetvariable in disconnect callback.Line 256 shadows the outer
netvariable from line 243. While this works, it's slightly redundant since the outernetis already captured.Suggested simplification
net->SetOnPlayerDisconnectCallback([this](SLNet::Packet *packet, uint32_t reason) { - const auto net = _networkingEngine->GetNetworkServer(); const auto guid = packet->guid; Logging::GetLogger(FRAMEWORK_INNER_SERVER)->debug("Disconnecting peer {}, reason: {}", guid.g, reason); // ... rest of callback uses captured net from outer scope
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
code/framework/src/external/sentry/wrapper.cppcode/framework/src/integrations/client/instance.cppcode/framework/src/integrations/client/instance.hcode/framework/src/integrations/client/scripting/builtins/events_lua.hcode/framework/src/integrations/server/instance.cppcode/framework/src/integrations/server/instance.hcode/framework/src/integrations/server/scripting/builtins/entity.cppcode/framework/src/integrations/server/scripting/builtins/events_lua.cppcode/framework/src/integrations/shared/rpc/emit_lua_event.hcode/framework/src/networking/messages/client_connection_finalized.hcode/framework/src/networking/messages/client_handshake.hcode/framework/src/networking/messages/client_initialise_player.hcode/framework/src/networking/messages/client_kick.hcode/framework/src/networking/messages/client_ready_assets.hcode/framework/src/networking/messages/client_request_streamer.hcode/framework/src/networking/messages/game_sync/entity_despawn.hcode/framework/src/networking/messages/game_sync/entity_owner_update.hcode/framework/src/networking/messages/game_sync/entity_self_update.hcode/framework/src/networking/messages/game_sync/entity_spawn.hcode/framework/src/networking/messages/game_sync/entity_update.hcode/framework/src/networking/network_client.hcode/framework/src/networking/network_peer.hcode/framework/src/networking/network_server.hcode/framework/src/world/client.cppcode/framework/src/world/client.hcode/framework/src/world/engine.hcode/framework/src/world/game_rpc/set_frame.hcode/framework/src/world/game_rpc/set_transform.hcode/framework/src/world/modules/base.hppcode/framework/src/world/modules/modules_impl.cppcode/framework/src/world/server.hcode/tests/modules/state_machine_ut.hdocs/NETWORKING_API_MIGRATION_GUIDE.md
💤 Files with no reviewable changes (1)
- code/framework/src/world/server.h
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cc,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,cc,cxx,h,hpp,hxx}: Use.clang-format(LLVM-based) for code formatting - runscripts/format_codebase.shto format
Use_prefix for private member variables with camelCase naming in C++
Use PascalCase for class names in C++ and mark classes asfinalwhen possible
Usenamespace Framework::SubModule {with closing comment} // namespace Framework::SubModulepattern in C++
UseFW_SEND_COMPONENT_RPC(rpc, ...)andFW_SEND_COMPONENT_RPC_TO(rpc, guid, ...)macros for network communication
Access core framework systems viaFramework::CoreModules::Get*()static methods for module registration
UsePlayerFactoryandStreamingFactoryfor entity creation in the ECS system
Files:
code/framework/src/networking/messages/game_sync/entity_self_update.hcode/framework/src/networking/messages/client_initialise_player.hcode/tests/modules/state_machine_ut.hcode/framework/src/networking/messages/game_sync/entity_owner_update.hcode/framework/src/networking/messages/game_sync/entity_despawn.hcode/framework/src/networking/messages/game_sync/entity_update.hcode/framework/src/integrations/client/scripting/builtins/events_lua.hcode/framework/src/networking/messages/client_connection_finalized.hcode/framework/src/networking/messages/client_ready_assets.hcode/framework/src/external/sentry/wrapper.cppcode/framework/src/world/client.hcode/framework/src/world/engine.hcode/framework/src/world/client.cppcode/framework/src/networking/network_server.hcode/framework/src/networking/messages/game_sync/entity_spawn.hcode/framework/src/world/game_rpc/set_transform.hcode/framework/src/world/modules/base.hppcode/framework/src/integrations/client/instance.hcode/framework/src/integrations/server/scripting/builtins/events_lua.cppcode/framework/src/networking/messages/client_handshake.hcode/framework/src/networking/network_client.hcode/framework/src/world/game_rpc/set_frame.hcode/framework/src/integrations/server/instance.hcode/framework/src/networking/messages/client_kick.hcode/framework/src/integrations/server/instance.cppcode/framework/src/networking/network_peer.hcode/framework/src/networking/messages/client_request_streamer.hcode/framework/src/integrations/server/scripting/builtins/entity.cppcode/framework/src/world/modules/modules_impl.cppcode/framework/src/integrations/shared/rpc/emit_lua_event.hcode/framework/src/integrations/client/instance.cpp
**/*.{h,hpp,hxx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
#pragma oncefor header guards in C++ files
Files:
code/framework/src/networking/messages/game_sync/entity_self_update.hcode/framework/src/networking/messages/client_initialise_player.hcode/tests/modules/state_machine_ut.hcode/framework/src/networking/messages/game_sync/entity_owner_update.hcode/framework/src/networking/messages/game_sync/entity_despawn.hcode/framework/src/networking/messages/game_sync/entity_update.hcode/framework/src/integrations/client/scripting/builtins/events_lua.hcode/framework/src/networking/messages/client_connection_finalized.hcode/framework/src/networking/messages/client_ready_assets.hcode/framework/src/world/client.hcode/framework/src/world/engine.hcode/framework/src/networking/network_server.hcode/framework/src/networking/messages/game_sync/entity_spawn.hcode/framework/src/world/game_rpc/set_transform.hcode/framework/src/world/modules/base.hppcode/framework/src/integrations/client/instance.hcode/framework/src/networking/messages/client_handshake.hcode/framework/src/networking/network_client.hcode/framework/src/world/game_rpc/set_frame.hcode/framework/src/integrations/server/instance.hcode/framework/src/networking/messages/client_kick.hcode/framework/src/networking/network_peer.hcode/framework/src/networking/messages/client_request_streamer.hcode/framework/src/integrations/shared/rpc/emit_lua_event.h
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: MafiaHub/Framework PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T18:15:01.266Z
Learning: Applies to **/*.{cpp,cc,cxx,h,hpp,hxx} : Use `FW_SEND_COMPONENT_RPC(rpc, ...)` and `FW_SEND_COMPONENT_RPC_TO(rpc, guid, ...)` macros for network communication
📚 Learning: 2025-12-20T18:15:01.266Z
Learnt from: CR
Repo: MafiaHub/Framework PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T18:15:01.266Z
Learning: Applies to **/*.{cpp,cc,cxx,h,hpp,hxx} : Use `FW_SEND_COMPONENT_RPC(rpc, ...)` and `FW_SEND_COMPONENT_RPC_TO(rpc, guid, ...)` macros for network communication
Applied to files:
code/framework/src/world/client.hcode/framework/src/world/engine.hcode/framework/src/world/client.cppcode/framework/src/networking/network_server.hcode/framework/src/integrations/client/instance.hdocs/NETWORKING_API_MIGRATION_GUIDE.mdcode/framework/src/networking/network_client.hcode/framework/src/networking/network_peer.hcode/framework/src/integrations/server/scripting/builtins/entity.cpp
📚 Learning: 2025-12-20T18:15:01.266Z
Learnt from: CR
Repo: MafiaHub/Framework PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T18:15:01.266Z
Learning: Applies to **/*.{cpp,cc,cxx,h,hpp,hxx} : Access core framework systems via `Framework::CoreModules::Get*()` static methods for module registration
Applied to files:
code/framework/src/world/engine.h
📚 Learning: 2025-12-20T18:15:01.266Z
Learnt from: CR
Repo: MafiaHub/Framework PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T18:15:01.266Z
Learning: Applies to **/*.{cpp,cc,cxx,h,hpp,hxx} : Use `PlayerFactory` and `StreamingFactory` for entity creation in the ECS system
Applied to files:
code/framework/src/world/modules/base.hppcode/framework/src/integrations/client/instance.hcode/framework/src/integrations/server/instance.hcode/framework/src/integrations/server/scripting/builtins/entity.cpp
🧬 Code graph analysis (9)
code/framework/src/world/client.cpp (1)
code/framework/src/integrations/client/instance.cpp (2)
OnSetTransform(397-409)OnSetTransform(397-397)
code/framework/src/networking/network_server.h (2)
code/framework/src/networking/rpc/game_rpc.h (2)
SetServerID(36-38)bool(49-57)code/framework/src/networking/network_client.h (1)
Framework(24-104)
code/framework/src/world/game_rpc/set_transform.h (1)
code/framework/src/world/modules/base.hpp (2)
tr(94-96)tr(94-94)
code/framework/src/integrations/client/instance.h (4)
code/framework/src/world/client.h (2)
Framework(19-22)Framework(24-60)code/framework/src/world/modules/base.hpp (7)
ClientReceiverHandler(65-67)guid(53-53)guid(69-69)guid(70-70)guid(71-71)guid(72-72)guid(73-73)code/framework/src/integrations/client/instance.cpp (10)
OnClientReadyAssets(340-358)OnClientReadyAssets(340-340)OnClientConnectionFinalized(360-379)OnClientConnectionFinalized(360-360)OnClientKick(381-395)OnClientKick(381-381)OnSetTransform(397-409)OnSetTransform(397-397)OnEmitLuaEvent(411-430)OnEmitLuaEvent(411-411)code/framework/src/world/client.cpp (2)
OnSetTransform(117-127)OnSetTransform(117-117)
code/framework/src/networking/network_client.h (2)
code/framework/src/networking/rpc/game_rpc.h (1)
bool(49-57)code/framework/src/networking/rpc/rpc.h (1)
bool(32-40)
code/framework/src/integrations/server/instance.h (4)
code/framework/src/integrations/client/instance.h (4)
Framework(41-45)Framework(47-49)Framework(51-53)Framework(55-268)code/framework/src/networking/messages/client_handshake.h (1)
Framework(15-63)code/framework/src/integrations/server/instance.cpp (8)
OnClientHandshake(278-338)OnClientHandshake(278-278)OnClientRequestStreamer(340-362)OnClientRequestStreamer(340-340)OnClientInitPlayer(364-369)OnClientInitPlayer(364-364)OnEmitLuaEvent(371-391)OnEmitLuaEvent(371-371)code/framework/src/world/modules/base.hpp (8)
guid(53-53)guid(69-69)guid(70-70)guid(71-71)guid(72-72)guid(73-73)ServerReceiverHandler(51-51)ServerReceiverHandler(51-51)
code/framework/src/integrations/server/instance.cpp (3)
code/framework/src/world/modules/base.hpp (8)
net(238-238)net(239-239)guid(53-53)guid(69-69)guid(70-70)guid(71-71)guid(72-72)guid(73-73)code/framework/src/world/modules/modules_impl.cpp (2)
SetupServerReceivers(198-202)SetupServerReceivers(198-198)code/framework/src/world/types/streaming.hpp (6)
e(18-26)e(18-18)e(29-37)e(29-29)e(39-44)e(39-39)
code/framework/src/networking/messages/client_request_streamer.h (1)
code/framework/src/utils/hardware_id.h (1)
string(25-49)
code/framework/src/integrations/client/instance.cpp (5)
code/framework/src/world/modules/base.hpp (10)
net(238-238)net(239-239)guid(53-53)guid(69-69)guid(70-70)guid(71-71)guid(72-72)guid(73-73)tr(94-96)tr(94-94)code/framework/src/world/client.cpp (2)
OnSetTransform(117-127)OnSetTransform(117-117)code/framework/src/integrations/server/instance.cpp (2)
OnEmitLuaEvent(371-391)OnEmitLuaEvent(371-371)code/framework/src/integrations/client/instance.h (1)
_initialDownloadDone(139-139)code/framework/src/world/modules/modules_impl.cpp (2)
SetupClientReceivers(204-212)SetupClientReceivers(204-204)
🔇 Additional comments (52)
code/tests/modules/state_machine_ut.h (2)
173-173: Good addition for tracking concurrent request success.The atomic counter properly tracks successful state transition requests in a thread-safe manner, addressing the concurrent nature of the test.
205-207: Verification aligns with the weakened assertions above.The verification correctly checks that at least some successful transitions occurred, which is consistent with the conditional tracking introduced earlier. However, as noted in the previous comment, consider using a stronger threshold to better validate concurrent performance.
code/framework/src/world/engine.h (1)
14-15: LGTM!The include reordering is appropriate. The removal of
FW_SEND_COMPONENT_RPCandFW_SEND_COMPONENT_RPC_TOmacros (as noted in the AI summary) aligns with the PR's objective of migrating to a router-based API with explicit handler methods.Note: The coding guidelines mention using these macros for network communication, but this PR intentionally deprecates them as part of the API revamp. Consider updating the guidelines (e.g.,
CLAUDE.md) to reflect the new router-based pattern.code/framework/src/networking/messages/game_sync/entity_self_update.h (1)
20-21: LGTM!The default constructor addition is consistent with the API migration pattern. Since this message has no member variables and uses a no-op
Serialize, the defaulted constructor is appropriate.code/framework/src/integrations/shared/rpc/emit_lua_event.h (1)
22-26: LGTM!The constructor-based initialization pattern is well-implemented. The
std::stringtoSLNet::RakStringconversion via.c_str()is correct.code/framework/src/integrations/server/scripting/builtins/events_lua.cpp (1)
26-27: LGTM!The migration to single-step constructor initialization is cleaner and eliminates the possibility of using a partially-initialized RPC object. The
jsonPayload.dump()produces astd::stringwhich correctly matches the constructor parameter.code/framework/src/networking/messages/client_kick.h (1)
24-28: LGTM!Constructor-based initialization is correctly implemented. The default parameter for
customReasonprovides backward compatibility for simple kick scenarios.code/framework/src/networking/messages/client_request_streamer.h (1)
24-30: LGTM!The constructor-based initialization is well-implemented and consistent with the other message classes in this PR. The default value for
playerHardwareIdmaintains backward compatibility.code/framework/src/world/client.h (2)
18-22: LGTM!Forward declarations for RPC types are appropriate here to avoid including full headers in the interface file. The namespace structure
Framework::World::RPCfollows the project conventions.
35-39: LGTM!The migration from macro-based RPC sending to explicit handler methods is well-structured:
InitRPCsdroppingconstis appropriate since it now registers handlers (mutates state).- Handler signatures
(SLNet::RakNetGUID guid, RPC::Type *rpc)are consistent with the new router-based dispatch pattern.code/framework/src/networking/messages/client_handshake.h (1)
24-30: LGTM! Constructor-based initialization implemented correctly.The migration from
FromParameters()to constructor-based initialization follows best practices. The parameterized constructor properly initializes all members via the initializer list, and the default constructor is explicitly defaulted for flexibility.code/framework/src/integrations/server/scripting/builtins/entity.cpp (2)
124-124: LGTM! Parameterized RPC construction.The migration from default construction +
FromParameters()to direct parameterized construction ofEmitLuaEventis clean and follows the new API pattern correctly.
226-226: LGTM! Parameterized message construction.The
ClientKickmessage is now constructed with parameters directly, which is consistent with the API migration goals. Good improvement over the previous mutation-based approach.code/framework/src/networking/messages/client_initialise_player.h (1)
20-20: LGTM! Default constructor added for API consistency.The explicit default constructor aligns with the migration to constructor-based initialization across all message types, even though this particular message has no state to initialize.
code/framework/src/integrations/client/scripting/builtins/events_lua.h (1)
28-28: LGTM! Clean migration to parameterized RPC construction.The change from default construction followed by
FromParameters()to direct parameterized construction is consistent with the broader API migration and improves code clarity.code/framework/src/networking/messages/game_sync/entity_despawn.h (1)
20-20: LGTM! Default constructor added for consistency.Adding the explicit default constructor maintains consistency with the new API pattern across all message types.
docs/NETWORKING_API_MIGRATION_GUIDE.md (1)
1-358: Documentation is comprehensive and well-structured.The migration guide provides excellent coverage of the API changes including:
- Clear before/after examples
- Migration steps for each pattern
- Handler method signatures
- Complete server instance migration example
- Migration checklist
The structure and examples align well with the code changes observed in the PR.
code/framework/src/networking/messages/game_sync/entity_owner_update.h (1)
22-25: LGTM! Constructor-based initialization correctly implemented.The parameterized constructor properly initializes
_ownervia the initializer list, and the default constructor is provided for flexibility. This follows the consistent migration pattern across all message types.code/framework/src/world/game_rpc/set_frame.h (1)
20-23: LGTM! RPC constructor migration implemented correctly.The conversion from
FromParameters()to constructor-based initialization is clean and efficient. Using a const reference parameter for theFrameobject is the right choice for performance, and the initializer list ensures proper initialization.code/framework/src/networking/network_client.h (1)
16-16: LGTM - Include required for flecs::entity usage.The flecs.h include is necessary to support the new sendGameRPC method that accepts a flecs::entity parameter.
code/framework/src/networking/messages/client_ready_assets.h (1)
80-83: LGTM - Constructor-based initialization.The new constructors align with the migration from
FromParametersmutators to constructor-based initialization, enabling direct construction of messages with required parameters.code/framework/src/networking/messages/client_connection_finalized.h (1)
24-28: LGTM - Constructor-based initialization.The constructors properly initialize message state. Note that validation is deferred to the
Valid()method (line 40), which is consistent with the message pattern used throughout the codebase.code/framework/src/networking/messages/game_sync/entity_spawn.h (1)
23-26: LGTM - Constructor-based initialization.The constructors align with the migration pattern. The parameterized constructor takes
Transformby value, which is appropriate for the message construction pattern.code/framework/src/world/game_rpc/set_transform.h (1)
20-23: LGTM - Constructor-based initialization.The constructors properly replace the
FromParametersmutator pattern. The parameterized constructor efficiently uses a const reference to avoid unnecessary copying.code/framework/src/world/client.cpp (1)
111-115: LGTM - Router-based RPC wiring.The migration to router-based wiring with dedicated handler methods improves code organization and maintainability over inline lambda registrations.
code/framework/src/integrations/server/instance.h (5)
28-28: LGTM - Include required for ServerReceiverHandler.The include is necessary for the
ServerReceiverHandlermember declared on line 141.
36-45: LGTM - Forward declarations reduce compilation dependencies.The forward declarations for message and RPC handler types appropriately minimize header dependencies.
118-118: LGTM - Signature change enables router-based wiring.Removing the
constqualifier is necessary to support the non-const router wiring operations in the implementation, consistent with the broader migration to router-based message handling.
127-131: LGTM - Handler method declarations.The new handler methods follow a consistent pattern with the client-side handlers, accepting a
RakNetGUIDand message/RPC pointer for router-based dispatch.
140-141: LGTM - Appropriate ownership for ServerReceiverHandler.The
unique_ptrprovides clear ownership semantics for the server receiver handler, which is appropriate for a single-owner component lifecycle.code/framework/src/world/modules/base.hpp (2)
18-29: LGTM! Forward declarations and includes are well-organized.The forward declarations for message types allow the header to avoid heavy includes while still supporting pointer-based handler signatures. The namespace structure follows the project conventions.
238-239: LGTM! Updated API signatures properly accept handler instances.The new signatures correctly pass handler pointers, aligning with the router-based architecture. The handler lifetime requirement is documented in the binder class comments in
network_peer.h.code/framework/src/integrations/client/instance.h (4)
11-12: LGTM! Platform-safe include added.The
safe_win32.hinclude helps manage Windows-specific header pollution.
40-53: LGTM! Forward declarations reduce coupling.Forward declaring the message and RPC types allows the header to avoid including the full message definitions, keeping compilation dependencies minimal.
133-135: LGTM! Handler ownership is correctly managed.Using
std::unique_ptrfor_clientReceiverHandlerensures proper cleanup and clear ownership semantics. The handler will be destroyed when theInstanceis destroyed.
150-155: LGTM! Handler methods are appropriately scoped as private.These methods are internal implementation details for message dispatch and should not be part of the public API.
code/framework/src/networking/network_peer.h (4)
24-40: LGTM! Clean binder pattern with appropriate documentation.The
MessageBindertemplate provides a type-safe way to register handlers. The comment about handler instance lifetime is important and correctly documented.
72-97: LGTM! Fluent MessageRouter API is well-designed.The
MessageRouterprovides a clean, discoverable API for handler registration. The patternnet->router().on<T>().handle(...)is intuitive and type-safe.
231-251: LGTM! Variadic send helpers enable cleaner call sites.The templated
sendandsendRPCmethods with perfect forwarding allow constructing messages inline, reducing boilerplate at call sites.
256-280: The lambda capture concern is already addressed. The binder class comments explicitly state "The handler instance must outlive the NetworkPeer" (lines 28–29, 43–44, 58–59), and the actual usage in client/server instances correctly ensures this invariant: handlers are stored asstd::unique_ptrmembers alongside the NetworkPeer, giving both the same lifetime. The documented precondition is clear and the implementation follows it.code/framework/src/world/modules/modules_impl.cpp (2)
32-33: LGTM! Constructor-based initialization with fallback.The ternary handles the case where
tris null by providing a defaultTransform{}.
198-212: LGTM! Clean router-based wiring for receivers.The
SetupServerReceiversandSetupClientReceiversfunctions cleanly wire up message handlers using the new fluent router API.code/framework/src/integrations/server/instance.cpp (5)
242-253: LGTM! Clean migration to fluent router API.The
InitNetworkingMessagesnow uses the fluent router API for message and RPC registration, which is more readable and type-safe than the previous inline lambda approach.
271-275: LGTM! Handler creation with proper lifetime management.The
_serverReceiverHandleris created asunique_ptrand stored on the instance, ensuring it outlives theNetworkPeeras required by the binder contract.
278-338: LGTM! Comprehensive handshake validation.The
OnClientHandshakehandler properly validates game name, framework version, client version, and game version with appropriate kick messages for each failure case.
340-362: LGTM! Player entity creation and streamer setup.The handler correctly creates the player entity, sets up streaming/player factories, and sends the connection finalized response.
371-391: LGTM! EmitLuaEvent handler with proper error handling.The handler validates the RPC, parses JSON payload with try-catch, and invokes the global event through the resource manager.
code/framework/src/integrations/client/instance.cpp (5)
285-309: LGTM! Clean migration to fluent router API on client side.The client-side message registration mirrors the server-side pattern, providing consistency across the codebase. Message handlers, GameRPC handlers, and RPC handlers are all registered using the appropriate fluent methods.
333-337: LGTM! Client receiver handler setup with proper lifetime.The
_clientReceiverHandleris created with dependencies (_worldEngine,_streamingFactory) and stored asunique_ptr, ensuring it outlives the network peer.
340-358: LGTM! Resource list handling in OnClientReadyAssets.The handler properly stores the resource list in
_pendingServerResources(which survives scripting module reset) before triggering the download flow.
360-379: LGTM! Connection finalization with proper entity setup.The handler correctly initializes the world engine connection, creates the player entity, sets up factories, and notifies the server and mod-level callbacks.
411-430: LGTM! EmitLuaEvent handler mirrors server implementation.The client-side handler follows the same pattern as the server, with proper validation and JSON parsing with error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/framework/src/integrations/server/scripting/builtins/entity.cpp (1)
232-244: Use helper function for consistency.Line 235 performs an inline
reinterpret_castinstead of using theGetNetworkServer()helper introduced at the top of this file. For consistency and maintainability, refactor to use the helper function.🔎 Proposed fix for consistency
void Entity::Kick(const std::string &reason) { const auto st = _ent.get<Framework::World::Modules::Base::Streamer>(); if (st) { - auto net = reinterpret_cast<Framework::Networking::NetworkServer *>(Framework::CoreModules::GetNetworkPeer()); + auto net = GetNetworkServer(); const auto streamer = _ent.get<Framework::World::Modules::Base::Streamer>(); if (net && streamer) { Framework::Networking::Messages::ClientKick kick(Framework::Networking::Messages::DisconnectionReason::KICKED_CUSTOM, reason); net->Send(kick, streamer->guid); net->GetPeer()->CloseConnection(SLNet::RakNetGUID(streamer->guid), true); return; } } }Note: The constructor-based initialization of
ClientKickon line 238 correctly follows the new API pattern.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CLAUDE.mdcode/framework/src/integrations/server/scripting/builtins/entity.cpp
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,cc,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,cc,cxx,h,hpp,hxx}: Use.clang-format(LLVM-based) for code formatting - runscripts/format_codebase.shto format
Use_prefix for private member variables with camelCase naming in C++
Use PascalCase for class names in C++ and mark classes asfinalwhen possible
Usenamespace Framework::SubModule {with closing comment} // namespace Framework::SubModulepattern in C++
UseFW_SEND_COMPONENT_RPC(rpc, ...)andFW_SEND_COMPONENT_RPC_TO(rpc, guid, ...)macros for network communication
Access core framework systems viaFramework::CoreModules::Get*()static methods for module registration
UsePlayerFactoryandStreamingFactoryfor entity creation in the ECS system
Files:
code/framework/src/integrations/server/scripting/builtins/entity.cpp
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: MafiaHub/Framework PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T18:15:01.266Z
Learning: Applies to **/*.{cpp,cc,cxx,h,hpp,hxx} : Use `FW_SEND_COMPONENT_RPC(rpc, ...)` and `FW_SEND_COMPONENT_RPC_TO(rpc, guid, ...)` macros for network communication
📚 Learning: 2025-12-20T18:15:01.266Z
Learnt from: CR
Repo: MafiaHub/Framework PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T18:15:01.266Z
Learning: Applies to **/*.{cpp,cc,cxx,h,hpp,hxx} : Use `FW_SEND_COMPONENT_RPC(rpc, ...)` and `FW_SEND_COMPONENT_RPC_TO(rpc, guid, ...)` macros for network communication
Applied to files:
CLAUDE.mdcode/framework/src/integrations/server/scripting/builtins/entity.cpp
📚 Learning: 2025-12-20T18:15:01.266Z
Learnt from: CR
Repo: MafiaHub/Framework PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T18:15:01.266Z
Learning: Applies to **/*.{cpp,cc,cxx,h,hpp,hxx} : Use `PlayerFactory` and `StreamingFactory` for entity creation in the ECS system
Applied to files:
CLAUDE.mdcode/framework/src/integrations/server/scripting/builtins/entity.cpp
📚 Learning: 2025-12-20T18:15:01.266Z
Learnt from: CR
Repo: MafiaHub/Framework PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T18:15:01.266Z
Learning: Applies to **/*.{cpp,cc,cxx,h,hpp,hxx} : Access core framework systems via `Framework::CoreModules::Get*()` static methods for module registration
Applied to files:
CLAUDE.md
📚 Learning: 2025-12-29T09:38:01.239Z
Learnt from: Segfaultd
Repo: MafiaHub/Framework PR: 179
File: code/framework/src/integrations/server/scripting/builtins/entity.cpp:23-31
Timestamp: 2025-12-29T09:38:01.239Z
Learning: In the MafiaHub Framework codebase, `Framework::CoreModules::GetNetworkPeer()` is guaranteed to return a `Framework::Networking::NetworkServer*` pointer in server-side code, and `Framework::CoreModules::GetWorldEngine()` is guaranteed to return a `Framework::World::ServerEngine*` pointer in server-side code. Using `reinterpret_cast` for these downcasts is acceptable due to these architectural guarantees.
Applied to files:
CLAUDE.mdcode/framework/src/integrations/server/scripting/builtins/entity.cpp
📚 Learning: 2025-12-29T09:34:41.781Z
Learnt from: Segfaultd
Repo: MafiaHub/Framework PR: 179
File: code/framework/src/external/sentry/wrapper.cpp:9-9
Timestamp: 2025-12-29T09:34:41.781Z
Learning: Treat #include "utils/safe_win32.h" as a special-case header that sets Windows include order and macro definitions. Do not flag it as unused even if no symbols from it are directly referenced, as its presence influences include order and WinSock version compatibility. Apply this guidance to C/C++ files that rely on this header (e.g., files under code/framework/src/external/sentry) and ensure its inclusion occurs in the expected location before other Windows headers.
Applied to files:
code/framework/src/integrations/server/scripting/builtins/entity.cpp
🧬 Code graph analysis (1)
code/framework/src/integrations/server/scripting/builtins/entity.cpp (1)
code/framework/src/world/modules/base.hpp (4)
tr(94-96)tr(94-94)net(238-238)net(239-239)
🪛 Cppcheck (2.19.0)
code/framework/src/integrations/server/scripting/builtins/entity.cpp
[error] 101-101: failed to evaluate #if condition
(syntaxError)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test ubuntu-latest
- GitHub Check: Test macos-latest
- GitHub Check: Test windows-latest
🔇 Additional comments (5)
CLAUDE.md (2)
57-61: Clarify the distinction betweenSendRPCandsendRPCmethods.Lines 57 and 60-61 use different naming conventions:
SendRPC(PascalCase) vssendRPCandsendGameRPC(camelCase). The documentation should clarify whether these are:
- Intentionally different methods serving different purposes
- A naming convention that distinguishes constructor-based calls from templated helpers
- Or if this is inconsistent and should be normalized
53-73: No action needed. All method signatures, type names, and API references in the documentation are accurate and match the implementation in the codebase.code/framework/src/integrations/server/scripting/builtins/entity.cpp (3)
23-31: LGTM! Helper functions centralize casting logic.The anonymous namespace helpers effectively reduce code duplication and centralize the type casting logic. As per established learnings, the
reinterpret_castusage for these specific downcasts is architecturally guaranteed safe in server-side code.
43-107: LGTM! Consistent defensive null-checking pattern.All setter methods (
SetPosition,SetRotation,SetVelocity,SetScale,SetModelName,SetModelHash) correctly implement the defensive pattern of validating bothGetNetworkServer()andGetServerEngine()before callingsendGameRPC. This addresses the previously identified null-checking concern and maintains consistency across the codebase.
136-137: LGTM! Migrated to constructor-based initialization.The
EmitEventmethod correctly adopts the new constructor-based initialization pattern forEmitLuaEvent, replacing the previousFromParametersapproach as outlined in the PR objectives.
|
|
||
| #include <flecs/flecs.h> | ||
|
|
||
| // Forward declarations for network message handlers |
There was a problem hiding this comment.
Not sure I like the growing forward decls here. Is there an alternative to this approach?
There was a problem hiding this comment.
@zpl-zak I understand your concern about the accumulating forward declarations. Here are some alternatives to consider:
Current Approach (Forward Declarations)
Pros: Minimal compile-time dependencies, standard C++ pattern for pointer/reference types
Cons: Visual clutter in header, grows with each new message type
Alternative Approaches
1. Handler Class Pattern (Recommended)
Your codebase already uses this pattern with ClientReceiverHandler for world module messages. You could extend it to these instance-level handlers:
// Forward declare only the handler class
namespace Framework::Integrations::Client {
class ClientMessageHandler;
}
// In Instance class
private:
std::unique_ptr<ClientMessageHandler> _messageHandler;Then move OnClientReadyAssets, OnClientConnectionFinalized, OnClientKick, OnSetTransform, and OnEmitLuaEvent into ClientMessageHandler. This consolidates all forward declarations into a single handler implementation file.
2. Type-Erased Handler Interface
Use std::function or similar with opaque types, though this adds runtime overhead and loses type safety.
3. Include Full Headers
Trade compile-time for cleaner headers, but increases build dependencies significantly.
Recommendation
Given that you already have ClientReceiverHandler for world messages, extending that pattern to these Instance handlers would be consistent and would eliminate the forward declarations from the public header. The handler class would own the knowledge of message types, keeping instance.h cleaner.
Would you like me to explore what a handler class refactor would look like for these methods?
There was a problem hiding this comment.
gib YOLO prompt so we can rawdog it.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how ClientReceiverHandler is structured
echo "=== ClientReceiverHandler structure ==="
ast-grep --pattern 'class ClientReceiverHandler {
$$$
}'
echo -e "\n=== How it's instantiated in instance.cpp ==="
rg -A5 "_clientReceiverHandler" code/framework/src/integrations/client/instance.cpp | head -20
echo -e "\n=== Current Instance handler methods ==="
rg "void.*Instance::On(ClientReadyAssets|ClientConnectionFinalized|ClientKick|SetTransform|EmitLuaEvent)" code/framework/src/integrations/client/
Length of output: 1416
@zpl-zak Alright, here's your YOLO refactor to eliminate those forward declarations. Create a dedicated handler class following the ClientReceiverHandler pattern:
Step 1: Create new handler class
code/framework/src/integrations/client/networking/message_handler.h
#pragma once
#include <RakNetTypes.h>
#include <memory>
namespace Framework::Networking::Messages {
class ClientReadyAssets;
class ClientConnectionFinalized;
class ClientKick;
}
namespace Framework::World::RPC {
class SetTransform;
}
namespace Framework::Integrations::Shared::RPC {
class EmitLuaEvent;
}
namespace Framework::Integrations::Client {
class Instance;
class InstanceMessageHandler {
private:
Instance* _instance;
public:
explicit InstanceMessageHandler(Instance* instance);
void OnClientReadyAssets(SLNet::RakNetGUID guid, Framework::Networking::Messages::ClientReadyAssets* msg);
void OnClientConnectionFinalized(SLNet::RakNetGUID guid, Framework::Networking::Messages::ClientConnectionFinalized* msg);
void OnClientKick(SLNet::RakNetGUID guid, Framework::Networking::Messages::ClientKick* msg);
void OnSetTransform(SLNet::RakNetGUID guid, Framework::World::RPC::SetTransform* rpc);
void OnEmitLuaEvent(SLNet::RakNetGUID guid, Framework::Integrations::Shared::RPC::EmitLuaEvent* rpc);
};
}code/framework/src/integrations/client/networking/message_handler.cpp
#include "message_handler.h"
#include "../instance.h"
#include <networking/messages/client_ready_assets.h>
#include <networking/messages/client_connection_finalized.h>
#include <networking/messages/client_kick.h>
#include <world/game_rpc/set_transform.h>
#include <integrations/shared/rpc/emit_lua_event.h>
namespace Framework::Integrations::Client {
InstanceMessageHandler::InstanceMessageHandler(Instance* instance) : _instance(instance) {}
void InstanceMessageHandler::OnClientReadyAssets(SLNet::RakNetGUID guid, Framework::Networking::Messages::ClientReadyAssets* msg) {
// Move implementation from Instance::OnClientReadyAssets here
// Access Instance members via _instance->GetWorldEngine(), etc.
}
void InstanceMessageHandler::OnClientConnectionFinalized(SLNet::RakNetGUID guid, Framework::Networking::Messages::ClientConnectionFinalized* msg) {
// Move implementation from Instance::OnClientConnectionFinalized here
}
void InstanceMessageHandler::OnClientKick(SLNet::RakNetGUID guid, Framework::Networking::Messages::ClientKick* msg) {
// Move implementation from Instance::OnClientKick here
}
void InstanceMessageHandler::OnSetTransform(SLNet::RakNetGUID guid, Framework::World::RPC::SetTransform* rpc) {
// Move implementation from Instance::OnSetTransform here
}
void InstanceMessageHandler::OnEmitLuaEvent(SLNet::RakNetGUID guid, Framework::Integrations::Shared::RPC::EmitLuaEvent* rpc) {
// Move implementation from Instance::OnEmitLuaEvent here
}
}Step 2: Update instance.h
Remove the forward declarations and handler methods, add the handler member:
// Remove these lines:
// Forward declarations for network message handlers
namespace Framework::Networking::Messages {
class ClientReadyAssets;
class ClientConnectionFinalized;
class ClientKick;
}
// ... etc
// Add this include instead:
#include "networking/message_handler.h"
// In the Instance class private section:
private:
// ... existing members ...
std::unique_ptr<World::Modules::ClientReceiverHandler> _clientReceiverHandler;
std::unique_ptr<InstanceMessageHandler> _messageHandler; // Add this
// Remove these method declarations:
// void OnClientReadyAssets(...);
// void OnClientConnectionFinalized(...);
// void OnClientKick(...);
// void OnSetTransform(...);
// void OnEmitLuaEvent(...);Step 3: Update instance.cpp
Initialize the handler and wire it up:
void Instance::InitNetworkingMessages() {
// ... existing setup ...
// Create the message handler
_messageHandler = std::make_unique<InstanceMessageHandler>(this);
// Wire up using the handler
auto r = net->router();
r.on<Framework::Networking::Messages::ClientReadyAssets>()
.handle(_messageHandler.get(), &InstanceMessageHandler::OnClientReadyAssets);
r.on<Framework::Networking::Messages::ClientConnectionFinalized>()
.handle(_messageHandler.get(), &InstanceMessageHandler::OnClientConnectionFinalized);
r.on<Framework::Networking::Messages::ClientKick>()
.handle(_messageHandler.get(), &InstanceMessageHandler::OnClientKick);
r.onGameRPC<Framework::World::RPC::SetTransform>()
.handle(_messageHandler.get(), &InstanceMessageHandler::OnSetTransform);
r.onRPC<Shared::RPC::EmitLuaEvent>()
.handle(_messageHandler.get(), &InstanceMessageHandler::OnEmitLuaEvent);
}
// Move the implementation bodies from the Instance::On* methods
// to the InstanceMessageHandler::On* methods in message_handler.cppThat's it. All forward declarations gone from instance.h, consolidated into the handler implementation file where they belong. Pattern matches your existing ClientReceiverHandler approach.
🧬 Code Graph Analysis Results
[code/framework/src/integrations/client/networking/engine.h] (lines 13-32)
class Engine {
private:
std::unique_ptr<Framework::Networking::NetworkClient> _networkClient {};
public:
Engine();
bool Init() const;
bool Shutdown() const;
bool Connect(const std::string &, const int32_t, const std::string password = "") const;
void Update() const;
Framework::Networking::NetworkClient *GetNetworkClient() const {
return _networkClient.get();
}
};
[code/framework/src/networking/messages/client_handshake.h] (lines 15-63)
namespace Framework::Networking::Messages {
class ClientHandshake final: public IMessage {
private:
SLNet::RakString _clientVersion = "";
SLNet::RakString _fwVersion = "";
SLNet::RakString _gameVersion = "";
SLNet::RakString _gameName = "";
public:
ClientHandshake() = default;
ClientHandshake(const std::string &clientVersion, const std::string &fwVersion, const std::string &gameVersion, const std::string &gameName)
: _clientVersion(clientVersion.c_str())
, _fwVersion(fwVersion.c_str())
, _gameVersion(gameVersion.c_str())
, _gameName(gameName.c_str()) {}
uint8_t GetMessageID() const override {
return GAME_CONNECTION_HANDSHAKE;
}
void Serialize(SLNet::BitStream *bs, bool write) override {
bs->Serialize(write, _fwVersion);
bs->Serialize(write, _clientVersion);
bs->Serialize(write, _gameVersion);
bs->Serialize(write, _gameName);
}
bool Valid() const override {
return _fwVersion.GetLength() > 0 && _clientVersion.GetLength() > 0 && _gameVersion.GetLength() > 0 && _gameName.GetLength() > 0;
}
std::string GetFWVersion() const {
return _fwVersion.C_String();
}
std::string GetClientVersion() const {
return _clientVersion.C_String();
}
std::string GetGameVersion() const {
return _gameVersion.C_String();
}
std::string GetGameName() const {
return _gameName.C_String();
}
};
}
[code/framework/src/integrations/client/scripting/module.cpp] (lines 21-28)
ClientScriptingModule::ClientScriptingModule(std::shared_ptr<World::ClientEngine> world): _world(world) {
_clientEngine = std::make_shared<Framework::Scripting::ClientEngine>();
CoreModules::SetScriptingEngine(_clientEngine.get());
}
Summary by CodeRabbit
Refactoring
Bug Fixes / Stability
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.