Feature/rb 118 system receive broadcast position relative#106
Feature/rb 118 system receive broadcast position relative#106romainpanno merged 17 commits intodevfrom
Conversation
Add packet to send packet list PATCH
…receive-broadcast-position-relative
WalkthroughThe changes primarily focus on enhancing the networking capabilities of the system. New functionalities have been added to handle relative positions and new allies in the game. Exception handling has been improved in the network processing functions, and a new function to check if the network is running has been introduced. The code has been made more robust and efficient with these changes. Changes
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Files selected for processing (12)
- src/Client/Systems/Network/ClientNetwork.cpp (6 hunks)
- src/Client/Systems/Network/ClientNetwork.hpp (1 hunks)
- src/ECS/ECSCustomTypes.hpp (1 hunks)
- src/ECS/Systems/Systems.cpp (1 hunks)
- src/Nitwork/ANitwork.cpp (6 hunks)
- src/Nitwork/ANitwork.hpp (1 hunks)
- src/Nitwork/INitwork.hpp (1 hunks)
- src/Nitwork/Nitwork.h (1 hunks)
- src/Nitwork/NitworkClient.hpp (1 hunks)
- src/Nitwork/NitworkServer.cpp (3 hunks)
- src/Nitwork/NitworkServer.hpp (3 hunks)
- src/main_server.cpp (1 hunks)
Files skipped from review due to trivial changes (1)
- src/Nitwork/NitworkClient.hpp
Additional comments (Suppressed): 23
src/Client/Systems/Network/ClientNetwork.hpp (1)
- 16-16: A new function
receiveRelativePositionis introduced. Ensure that this function is implemented correctly and is called in the appropriate places in the codebase.src/Nitwork/INitwork.hpp (1)
- 92-92: The addition of
isRunning()function is a good practice as it allows to check the running status of the Nitwork server. Ensure that this function is implemented correctly in the derived classes and returns the accurate status.src/Nitwork/Nitwork.h (1)
- 191-194: The
player_infos_s datafield has been replaced withn_id_t playerId. Ensure that all uses of this struct in the codebase have been updated to reflect this change. Also, verify that the newplayerIdfield is being correctly populated and used where this struct is utilized.src/ECS/ECSCustomTypes.hpp (1)
- 42-46: The
operator+=overload for thePositionstruct is a good addition for convenience. It allows for easy addition of twoPositionobjects. However, it's important to ensure that this operator is used correctly throughout the codebase to avoid any unintended side effects.src/Nitwork/ANitwork.hpp (1)
- 28-31: The
isRunning()function has been added. This function is likely used to check the running status of the network server. Ensure that this function is implemented correctly and returns the expected result. Also, verify that it is used appropriately throughout the codebase.src/main_server.cpp (1)
- 62-62: The main loop now also checks if the
NitworkServeris running. This is a good addition as it ensures that the server is operational before updating the systems. However, please verify that theisRunning()function inNitworkServeris thread-safe and does not introduce race conditions.src/Nitwork/NitworkServer.hpp (3)
74-76: The new method
sendNewAllieis introduced. Ensure that it is used correctly throughout the codebase and that theplayerId,packetMsgNewAllie, andendpointparameters are properly validated before use. Also, consider renaming thebutNoOneparameter to something more descriptive to improve code readability.107-116: The new message type
POSITION_RELATIVEis introduced and handled byhandleRelativePositionMsg. Ensure that this message type is correctly implemented and handled throughout the codebase.172-181: The new message types
NEW_ALLIEandPOSITION_RELATIVE_BROADCASTare introduced and handled bysendData. Ensure that these message types are correctly implemented and handled throughout the codebase.src/Nitwork/NitworkServer.cpp (3)
111-126: The
sendNewAlliefunction has been added. It prepares a packet with thepacketNewAllie_sstructure and sends it to all clients or adds it to the send queue. The function takes aplayerId, apacketNewAllie_sstructure, an endpoint, and a booleanbutNoOneas parameters. IfbutNoOneis true, the packet is sent to all clients except the one at the provided endpoint. Otherwise, the packet is added to the send queue. The function modifies theplayerIdfield of thepacketNewAllie_sstructure before sending it.138-159: The
handleInitMsgfunction has been modified. It now sends apacketNewAllie_spacket to all clients when a new player is initialized. The function first adds the player's endpoint to the_endpointsvector and assigns the player an ID. It then prepares apacketNewAllie_spacket and sends it to all clients. The function also callsSystems::initPlayerto initialize the player in theSystemsnamespace. The function logs before and after sending thepacketNewAllie_spacket and in each iteration of the loop that sends the packet to all other players.181-186: The
handlePositionRelativeMsgfunction has been modified. It now casts themsgparameter to amsgPositionRelative_sstructure instead of aposition_relative_sstructure. The function then assigns theposfield of themsgPositionRelative_sstructure to theposvariable.src/Nitwork/ANitwork.cpp (5)
108-111: The
isRunningfunction is a good addition to theANitworkclass. It allows other parts of the code to check if the network is running before performing operations that require the network.215-219: The addition of exception handling in the action execution loop is a good practice. It prevents one faulty action from stopping the execution of subsequent actions.
226-228: The change to set
_isRunningtofalsewhen an exception is caught is a good practice. It ensures that the network stops running when a fatal error occurs.242-245: The addition of exception handling around the action execution is a good practice. It prevents a faulty action from stopping the execution of subsequent actions.
267-270: The change to set
_isRunningtofalsewhen an exception is caught is a good practice. It ensures that the network stops running when a fatal error occurs.src/Client/Systems/Network/ClientNetwork.cpp (5)
30-33: The function signature has been updated to include a comment indicating that the second parameter is unused. This is a good practice as it makes the code more readable and understandable.
46-49: The function signature has been updated to include a comment indicating that the second parameter is unused. This is a good practice as it makes the code more readable and understandable.
57-60: The function signature has been updated to include a comment indicating that the second parameter is unused. This is a good practice as it makes the code more readable and understandable.
66-69: The function signature has been updated to include a comment indicating that the second parameter is unused. This is a good practice as it makes the code more readable and understandable.
90-109: The function
receiveRelativePositionhas been updated to handle the new message typemsgPositionRelativeBroadcast_s. The logic seems to be correct, but it's important to ensure that the new message type is handled correctly in all parts of the codebase where it's used.src/ECS/Systems/Systems.cpp (1)
- 434-438: The new hunk introduces a
Types::PlayerAlliescomponent forotherPlayer. Ensure that this component is properly used and updated in the rest of the codebase. Also, verify that theallieobject is correctly initialized before being inserted into the registry.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Files selected for processing (12)
- src/Client/Systems/Network/ClientNetwork.cpp (6 hunks)
- src/Client/Systems/Network/ClientNetwork.hpp (1 hunks)
- src/ECS/ECSCustomTypes.hpp (1 hunks)
- src/ECS/Systems/Systems.cpp (1 hunks)
- src/Nitwork/ANitwork.cpp (6 hunks)
- src/Nitwork/ANitwork.hpp (1 hunks)
- src/Nitwork/INitwork.hpp (1 hunks)
- src/Nitwork/Nitwork.h (1 hunks)
- src/Nitwork/NitworkClient.hpp (1 hunks)
- src/Nitwork/NitworkServer.cpp (3 hunks)
- src/Nitwork/NitworkServer.hpp (3 hunks)
- src/main_server.cpp (1 hunks)
Files skipped from review due to trivial changes (1)
- src/Nitwork/NitworkClient.hpp
Additional comments (Suppressed): 16
src/Client/Systems/Network/ClientNetwork.hpp (1)
- 16-16: A new function
receiveRelativePositionhas been added. Ensure that this function is implemented correctly in the corresponding.cppfile and that it is used appropriately throughout the codebase. Also, verify that the function is handling the received data correctly and safely.src/Nitwork/INitwork.hpp (1)
- 92-92: The addition of
isRunning()function is a good practice as it allows to check the running status of the Nitwork server. Ensure that this function is implemented correctly in the derived classes and returns the accurate status.src/main_server.cpp (1)
- 62-62: The main loop condition has been updated to also check if the
NitworkServeris running. This is a good practice as it ensures that the server is operational before executing the loop's logic. However, ensure that theisRunning()function inNitworkServeris thread-safe and does not introduce race conditions.src/ECS/ECSCustomTypes.hpp (1)
- 42-46: The addition of the
operator+=function to thePositionstruct is a good practice as it simplifies the process of adding twoPositionobjects together. However, it's important to ensure that this operator is used correctly throughout the codebase to avoid any unintended side effects.Position &operator+=(const Position &pos) { x += pos.x; y += pos.y; return (*this); }src/Nitwork/Nitwork.h (1)
- 191-194: The
msgNewAllie_sstruct has been modified to replaceplayer_infos_s datawithn_id_t playerId. Ensure that all instances where this struct is used have been updated to reflect this change. This includes both the creation and usage of this struct. Also, verify that theplayerIdprovides enough information for the use cases whereplayer_infos_s datawas previously used.src/Nitwork/ANitwork.hpp (1)
- 28-31: The
isRunning()function has been added. This function is likely used to check the running status of the network server. Ensure that this function is implemented correctly and returns the expected result. Also, verify that it is used appropriately throughout the codebase.src/ECS/Systems/Systems.cpp (1)
- 434-438: The new hunk introduces a
PlayerAlliescomponent that is added to the registry whenotherPlayeris true. Ensure that this new component is properly handled in the rest of the codebase, especially in systems that interact with player entities. Also, consider initializingPlayerAllieswith some data if necessary.src/Nitwork/NitworkServer.hpp (3)
74-76: The new method
sendNewAlliehas been added. Ensure that this method is used correctly throughout the codebase. Also, thebutNoOneparameter name is not clear. Consider renaming it to improve code readability.107-116: A new message type
POSITION_RELATIVEhas been added to the message handling map. Ensure that thehandleRelativePositionMsgfunction correctly handles this new message type.168-181: Two new message types
NEW_ALLIEandPOSITION_RELATIVE_BROADCASThave been added to the message sending map. Ensure that thesendDatafunction correctly sends these new message types.src/Nitwork/NitworkServer.cpp (3)
111-126: The
sendNewAlliefunction has been added to send a new ally packet to all clients or all clients except one. The function takes aplayerId, apacketNewAllie_sstruct, anendpoint, and abutNoOneboolean. IfbutNoOneis true, the function sends the packet to all clients except the one at the specified endpoint. Otherwise, it sends the packet to all clients. The function uses thesendToAllClientsButNotOneandaddPacketToSendmethods to send the packets. This function seems to be well implemented and follows the existing code structure and style. However, it would be beneficial to verify that thesendToAllClientsButNotOneandaddPacketToSendmethods are correctly implemented and handle all edge cases.138-159: The
handleInitMsgfunction has been updated to handle the initialization of a new player. The function now creates apacketNewAllie_sstruct and sends it to all clients using thesendNewAlliefunction. It also initializes the player using theSystems::initPlayermethod. The function then loops through all the players and sends a new ally packet to each one. This function seems to be well implemented and follows the existing code structure and style. However, it would be beneficial to verify that theSystems::initPlayermethod is correctly implemented and handles all edge cases. Also, ensure that the loop correctly sends a new ally packet to each player and handles any exceptions that may occur.181-188: The
handlePositionRelativeMsgfunction has been updated to handle a relative position message. The function now extracts the position from the message data usingstd::any_cast. This is a change from the previous implementation, which directly cast the message to aposition_relative_sstruct. This change seems to be an improvement, as it makes the code more robust and less prone to errors. However, it would be beneficial to verify that thestd::any_castfunction is correctly implemented and handles all edge cases.src/Client/Systems/Network/ClientNetwork.cpp (1)
- 90-109: The new function
receiveRelativePositionis introduced to handle received position updates. It seems to be correctly implemented with proper error handling and thread safety. However, it's important to ensure that themsgPositionRelativeBroadcast_smessage type is correctly handled in all relevant parts of the codebase.src/Nitwork/ANitwork.cpp (2)
108-111: The
isRunningfunction is a good addition to theANitworkclass. It allows other parts of the code to check if the network is running before performing network-related operations, which can help prevent errors.238-245: The addition of exception handling when executing actions in the output thread is a good practice. It prevents the entire thread from crashing when an exception is thrown during the execution of an action.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Files selected for processing (6)
- src/Client/Systems/Network/ClientNetwork.cpp (6 hunks)
- src/ECS/ECSCustomTypes.hpp (1 hunks)
- src/Nitwork/ANitwork.cpp (6 hunks)
- src/Nitwork/ANitwork.hpp (1 hunks)
- src/Nitwork/NitworkServer.cpp (3 hunks)
- src/Nitwork/NitworkServer.hpp (3 hunks)
Additional comments (Suppressed): 15
src/Nitwork/ANitwork.hpp (1)
- 28-31: The
isRunningfunction has been added to check if the network server is running. Ensure that this function is used appropriately throughout the codebase to check the server status before performing operations that require the server to be running.src/ECS/ECSCustomTypes.hpp (1)
- 42-47: The addition of the
operator+=function to thePositionstruct is a good practice as it simplifies the process of adding twoPositionobjects together. However, it's important to ensure that this operator is used correctly throughout the codebase to avoid any unintended side effects.Position &operator+=(const Position &pos) { x += pos.x; y += pos.y; return (*this); }src/Nitwork/NitworkServer.hpp (3)
76-80: The
sendNewAlliefunction has been added. Ensure that it is being used correctly throughout the codebase. Also, thebutNoOneparameter is not clear from the context. Consider renaming it or adding a comment to explain its purpose.111-120: The handling of
POSITION_RELATIVEmessages has been added. Ensure that thehandleRelativePositionMsgfunction correctly processes the relative position messages.172-182: The handling of
NEW_ALLIEandPOSITION_RELATIVE_BROADCASTmessages has been added. Ensure that these new message types are correctly processed and sent.src/Nitwork/ANitwork.cpp (6)
108-111: The
isRunningfunction is a good addition for checking the running status of the network. It enhances the modularity and readability of the code.115-115: Ensure that the
startReceiveHandlerfunction is not called when the network is not running. This could lead to unexpected behavior or errors.215-219: Good addition of exception handling within the action execution loop. This prevents one faulty action from stopping the entire network.
224-228: The addition of setting
_isRunningto false when an exception is caught is a good practice. It ensures that the network stops running when a critical error occurs.242-245: Good addition of exception handling for action execution. This prevents a faulty action from stopping the entire output loop.
267-271: The addition of setting
_isRunningto false when an exception is caught is a good practice. It ensures that the network stops running when a critical error occurs.src/Client/Systems/Network/ClientNetwork.cpp (1)
- 91-109: The new function
receiveRelativePositionupdates the position of other players based on received messages. It's good to see that you're using a lock to ensure thread safety when accessing the sharedRegistryobject. However, it's important to verify that theoperator+=function for thePositionstruct handles edge cases correctly, such as overflow or underflow.src/Nitwork/NitworkServer.cpp (3)
- 111-130: The
sendNewAlliefunction has been added to send new ally packets to clients. It takes aplayerId, apacketNewAllie_sstruct, an endpoint, and a booleanbutNoOne. The function sets theplayerIdin the packet, creates a newPacketobject with the packet data, and sends it to all clients except one ifbutNoOneis true, or adds it to the send queue otherwise. This function seems to be well implemented and follows the existing code structure and style. However, thebutNoOneparameter name is not very descriptive and could be improved for better readability.- void NitworkServer::sendNewAllie( - n_id_t playerId, - struct packetNewAllie_s packetMsgNewAllie, - boost::asio::ip::udp::endpoint &endpoint, - bool butNoOne) + void NitworkServer::sendNewAllie( + n_id_t playerId, + struct packetNewAllie_s packetMsgNewAllie, + boost::asio::ip::udp::endpoint &endpoint, + bool excludeEndpoint)
142-163: The
handleInitMsgfunction has been updated to use the newsendNewAlliefunction. It creates a newpacketNewAllie_sstruct, initializes a new player with theinitPlayerfunction, and sends a new ally packet to all other clients. The function then loops through all player IDs and sends a new ally packet to each one, excluding the current player. This function seems to be well implemented and follows the existing code structure and style.185-192: The
handlePositionRelativeMsgfunction has been updated to extract position data from messages. It casts the message to amsgPositionRelative_sstruct and extracts the position data. It then creates a newpacketPositionRelativeBroadcast_sstruct with the position data. This function seems to be well implemented and follows the existing code structure and style.
…ve' of github.com:X-R-G-B/R-Bus into feature/RB-118-system-receive-broadcast-position-relative
…receive-broadcast-position-relative
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (3)
- src/Client/Systems/Network/ClientNetwork.cpp (6 hunks)
- src/Nitwork/ANitwork.cpp (6 hunks)
- src/Nitwork/NitworkServer.cpp (3 hunks)
Additional comments (Suppressed): 11
src/Client/Systems/Network/ClientNetwork.cpp (3)
81-88: The function
receiveNewAlliehas been updated to log the creation of a new ally and initialize the player. Ensure that theinitPlayerfunction can handle the new parameters and that the logging does not introduce performance issues.90-109: A new function
receiveRelativePositionhas been added. This function updates the position of other players based on received messages. It uses a mutex lock to ensure thread safety. However, consider using a more granular locking mechanism if possible to reduce contention.137-143: The
sendPositionRelativefunction has been updated to include logging. Ensure that the logging does not introduce performance issues, especially since this function might be called frequently.src/Nitwork/ANitwork.cpp (5)
108-111: The
isRunningfunction is a good addition for checking the running status of the network. It improves the modularity of the code.126-130: The
callReceiveHandlerfunction now starts the receive handler without logging an error message. Ensure that this change does not affect the debugging process or the understanding of the system's behavior.237-244: The addition of exception handling when executing an action in the output processing is a good practice. It prevents the entire output processing from stopping if an exception occurs in one action.
264-270: The addition of unlocking the
_outputQueueMutexbefore setting_isRunningto false and logging the error is a good practice. It prevents potential deadlocks if an exception is thrown.334-339: The log level for adding a packet to send has been changed from
infototrace. Ensure that this change aligns with the logging strategy and that important information is not lost in production logs.src/Nitwork/NitworkServer.cpp (3)
111-130: The
sendNewAlliefunction has been added. It prepares a packet with the new ally's information and sends it to all clients or all clients except one. The function uses thesendToAllClientsButNotOneandaddPacketToSendmethods for sending packets. Ensure that these methods are thread-safe if they can be accessed from multiple threads simultaneously. Also, consider renaming thebutNoOneparameter to something more descriptive, likeexcludeEndpoint, for better readability.142-160: The
handleInitMsgfunction has been updated. It now sends a new ally packet to all other clients when a new client connects. The function uses thesendNewAlliemethod for sending new ally packets. Ensure that thesendNewAlliemethod is thread-safe if it can be accessed from multiple threads simultaneously. Also, consider adding error handling for theSystems::initPlayercall.185-187: The
msgPositionRelative_sstruct is now used instead of theposition_relative_sstruct. Ensure that all references to theposition_relative_sstruct have been updated accordingly.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (3)
- src/Client/Systems/Network/ClientNetwork.cpp (6 hunks)
- src/ECS/ECSCustomTypes.hpp (1 hunks)
- src/ECS/Systems/Systems.cpp (1 hunks)
Additional comments (Suppressed): 9
src/ECS/ECSCustomTypes.hpp (1)
- 42-47: The addition of the
operator+=function to thePositionstruct is a good enhancement. It allows for easy addition of twoPositionobjects. However, it's important to ensure that this operator is used correctly throughout the codebase. Misuse could lead to unexpected behavior or bugs.src/Client/Systems/Network/ClientNetwork.cpp (7)
30-33: The function signature has been updated to include a comment for the unused parameter. This is a good practice as it makes the code more readable and understandable.
46-49: The function signature has been updated to include a comment for the unused parameter. This is a good practice as it makes the code more readable and understandable.
57-60: The function signature has been updated to include a comment for the unused parameter. This is a good practice as it makes the code more readable and understandable.
66-69: The function signature has been updated to include a comment for the unused parameter. This is a good practice as it makes the code more readable and understandable.
81-88: The function signature has been updated to include a comment for the unused parameter. This is a good practice as it makes the code more readable and understandable. Also, a log message has been added to indicate the creation of a new ally, which can be useful for debugging.
90-109: The new function
receiveRelativePositionis introduced to handle the reception of relative position messages and update the position of other players accordingly. It uses a mutex lock to ensure thread safety when accessing the sharedRegistryobject. The function also includes error handling to return early if the player is not found in the registry. This is a good practice as it prevents potential runtime errors.137-143: A log message has been added to trace the relative position being sent. This can be useful for debugging. The rest of the code remains unchanged.
src/ECS/Systems/Systems.cpp (1)
- 444-448: The new hunk introduces a
PlayerAlliescomponent forotherPlayer. Ensure that this component is properly handled in the rest of the codebase, and that its introduction doesn't break existing functionality. Also, theallieobject is created but not initialized with any data. If this is intentional, consider adding a comment to clarify.if (otherPlayer) { Types::OtherPlayer otherPlayerComp(constId); Registry::getInstance().getComponents<Types::OtherPlayer>().insertBack(otherPlayerComp); Types::PlayerAllies allie; + // Initialize allie with necessary data here, if needed Registry::getInstance().getComponents<Types::PlayerAllies>().insertBack(allie); } else { Types::Player playerComp = {constId}; Registry::getInstance().getComponents<Types::Player>().insertBack(playerComp);
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- src/Client/Systems/Network/ClientNetwork.cpp (6 hunks)
Additional comments (Suppressed): 3
src/Client/Systems/Network/ClientNetwork.cpp (3)
81-88: The
receiveNewAlliefunction has been modified to log the creation of a new ally and call theinitPlayerfunction with a different argument. Ensure that this change does not affect other parts of the code that rely on the original behavior of this function. Also, verify that theinitPlayerfunction can handle the new argument correctly.90-111: A new function
receiveRelativePositionhas been added. This function updates the position of other players based on received messages. It uses a mutex to ensure thread safety when accessing the sharedRegistryinstance. The function appears to be well-structured and follows good practices for thread safety and exception handling. However, it's important to ensure that themsgPositionRelativeBroadcast_smessage type is correctly handled in all relevant parts of the code.139-146: The
sendPositionRelativefunction has been modified to include a logging statement. This change seems to be for debugging purposes and should not affect the functionality of the function. However, ensure that the logging level is appropriate for the frequency of these messages and that it won't flood the logs in a production environment.
What is the current behavior? (link an issue based on the kind of change this pr introduce)
What is the new behavior (if this is a feature change)?
Other information:
Summary by CodeRabbit
msgNewAllie_smessage for more efficient data handling.