NITWORK: Fix packet id doesn't need to change if he's resent#100
NITWORK: Fix packet id doesn't need to change if he's resent#100Saverio976 merged 17 commits intodevfrom
Conversation
Rate Limit Exceeded@romainpanno has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 6 minutes and 25 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, convert this PR to a draft and then mark it as ready for review again to re-trigger the review. Alternatively, you can push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per repository. WalkthroughThe changes primarily focus on improving the handling of packet resending in the Nitwork networking library. This includes adding a Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (3)
- src/Nitwork/ANitwork.cpp (1 hunks)
- src/Nitwork/ANitwork.hpp (1 hunks)
- src/Nitwork/INitwork.hpp (1 hunks)
Additional comments (Suppressed): 3
src/Nitwork/ANitwork.hpp (1)
- 43-61: The new hunk introduces a check for the
isResendflag in thePacketclass. If the packet is not a resend, the packet ID and header are updated. This is a good performance optimization as it avoids unnecessary operations for resending packets. However, ensure that theisResendflag is being correctly set elsewhere in the code where packets are being resent. Also, verify that skipping the update of packet ID and header for resending packets does not introduce any issues in the packet processing logic.src/Nitwork/ANitwork.cpp (1)
- 188-193: The new hunk introduces a flag
isResendto thePacketclass and sets it totruewhen a packet is being resent. This flag is used to optimize the packet resending process by avoiding redundant updates to the packet ID and header for resend packets. However, it's important to ensure that theisResendflag is reset tofalsewhen the packet is successfully sent, otherwise, it might cause issues in subsequent resends if the same packet object is reused.packet->isResend = true; + if (sendPacket(*packet)) { + packet->isResend = false; + } addPacketToSend(*packet);src/Nitwork/INitwork.hpp (1)
- 56-63: The
Packetclass has been extended with a new boolean memberisResend. Ensure that this new member is properly initialized in all places where aPacketobject is created, and that it is correctly set totruewhen a packet is being resent. Also, verify that this change doesn't break any existing functionality that relies on thePacketclass.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- src/Nitwork/ANitwork.hpp (1 hunks)
Additional comments (Suppressed): 2
src/Nitwork/ANitwork.hpp (2)
43-58: The new hunk introduces a check for
packet.isResendbefore updating the packet ID and header. This is a logical change to prevent unnecessary updates for resend packets. However, it's important to ensure that theisResendflag is correctly set throughout the codebase, especially in the packet creation and resend logic. If not, this could lead to incorrect packet IDs and headers for resend packets.59-61: The
async_send_tofunction is called with the same arguments as before. The changes in the hunk do not affect this part of the code.
…:X-R-G-B/R-Bus into fix/Nitwork-dont-change-resent-packet-id
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (5)
- src/Nitwork/ANitwork.cpp (1 hunks)
- src/Nitwork/ANitwork.hpp (1 hunks)
- src/Nitwork/INitwork.hpp (1 hunks)
- src/Nitwork/Nitwork.h (1 hunks)
- src/Nitwork/NitworkServer.cpp (1 hunks)
Files skipped from review due to trivial changes (2)
- src/Nitwork/ANitwork.cpp
- src/Nitwork/Nitwork.h
Additional comments (Suppressed): 5
src/Nitwork/NitworkServer.cpp (1)
- 130-136: The new code introduces a check for the number of connected clients before sending the star wave message. This is a good practice to prevent unnecessary network traffic when there are not enough clients. However, ensure that this change does not affect other parts of the system that rely on the star wave message being sent regardless of the number of clients.
+ if (_endpoints.size() < MAX_CLIENTS) { + Logger::info("Not enough clients"); + return; + }src/Nitwork/ANitwork.hpp (2)
43-58: The new code introduces a check for the
isResendflag before updating the packet ID and header. This is a good optimization as it prevents unnecessary updates for resend packets. However, ensure that theisResendflag is correctly set in all places where a packet is being resent. Also, verify that the absence of ID and header updates for resend packets does not affect the functionality of the system.46-46: The use of
std::any_castcan throw abad_any_castexception if the contained value inpacket.bodyis not of typeT. Ensure that the type ofpacket.bodyis alwaysTwhen this line is executed, or add error handling for thebad_any_castexception.src/Nitwork/INitwork.hpp (2)
56-60: The
getIsResendandsetIsResendmethods have been added to thePacketclass to manage the new_isResendmember variable. This variable is used to indicate whether a packet is being resent. This is a good approach to avoid unnecessary updates for resend packets.68-68: The
_isResendmember variable is initialized tofalse. This is a good practice as it ensures that the variable is not left uninitialized, which could lead to undefined behavior.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- src/Nitwork/NitworkServer.cpp (1 hunks)
Additional comments (Suppressed): 1
src/Nitwork/NitworkServer.cpp (1)
- 133-136: The addition of a check for the number of connected clients before sending a star wave message is a good practice. It prevents unnecessary network traffic when there are not enough clients connected. However, ensure that this change does not affect the game logic or user experience negatively. For example, if a star wave message is critical for game progression, this change could potentially halt the game if there are not enough clients.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (2)
- src/Nitwork/ANitwork.hpp (1 hunks)
- src/Nitwork/INitwork.hpp (1 hunks)
Files skipped from review due to trivial changes (1)
- src/Nitwork/INitwork.hpp
Additional comments (Suppressed): 1
src/Nitwork/ANitwork.hpp (1)
- 43-58: The new code introduces a check for
packet.getIsResend()before updating the packet ID and header. This is a good optimization as it prevents unnecessary updates for resend packets. However, ensure that thegetIsResend()method is correctly implemented and that theisResendflag is properly set when a packet is being resent. Also, verify that this change does not affect the functionality of the system in any unexpected ways.
…:X-R-G-B/R-Bus into fix/Nitwork-dont-change-resent-packet-id
…:X-R-G-B/R-Bus into fix/Nitwork-dont-change-resent-packet-id
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (2)
- docs/network/rfc/RFC.md (1 hunks)
- src/Nitwork/ANitwork.hpp (1 hunks)
Additional comments (Suppressed): 4
docs/network/rfc/RFC.md (1)
- 295-299: The change in the server's response to the client's ready action is significant. Ensure that the server-side logic has been updated to only send a start wave action when all clients are ready, and that this change is compatible with the client-side logic.
src/Nitwork/ANitwork.hpp (3)
43-58: The new code introduces a check for
packet.getIsResend(). If the packet is not a resend, it updates the packet ID and header. This is a good performance optimization as it avoids unnecessary operations for resending packets. However, ensure that thegetIsResend()method is correctly implemented and theisResendflag is properly set when a packet is being resent.46-46: The use of
std::any_castcan throw astd::bad_any_castexception if the contained value inpacket.bodyis not of typeT. Ensure that the type ofpacket.bodyis alwaysTwhen this function is called, or add error handling for thestd::bad_any_castexception.48-58: The code creates a new
header_sstruct and assigns it todata.header. This is fine ifdata.headeris not used elsewhere in the code after this point. If it is, this could lead to unexpected behavior. Verify that this is the intended behavior.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (9)
- src/Nitwork/ANitwork.cpp (2 hunks)
- src/Nitwork/ANitwork.hpp (3 hunks)
- src/Nitwork/Nitwork.h (1 hunks)
- src/Nitwork/NitworkClient.cpp (1 hunks)
- src/Nitwork/NitworkClient.hpp (1 hunks)
- src/Nitwork/NitworkServer.cpp (3 hunks)
- src/Nitwork/NitworkServer.hpp (2 hunks)
- src/main_client.cpp (1 hunks)
- src/main_server.cpp (2 hunks)
Files skipped from review due to trivial changes (1)
- src/Nitwork/Nitwork.h
Additional comments (Suppressed): 16
src/Nitwork/NitworkClient.cpp (1)
- 25-28: The function signature of
startClienthas been changed. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the order of arguments passed toANitwork::startis correct.src/main_client.cpp (1)
- 64-70: The function signature of
start()has been changed tostartClient(). Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, the order of parameters has been changed. Verify that the new order of parameters is correctly used in all function calls.- .start(std::stoi(av[2]), DEFAULT_THREAD_NB, TICKS_PER_SECOND, av[1]) + .startClient(std::stoi(av[2]), av[1], DEFAULT_THREAD_NB, TICKS_PER_SECOND)src/Nitwork/ANitwork.cpp (2)
188-193: The new code introduces a
setIsResend(true)call before adding the packet to the send queue. This is a good performance optimization as it prevents unnecessary updates for resend packets. However, ensure that thesetIsResendmethod correctly sets theisResendmember variable and that this variable is checked before updating packet ID and header information.317-319: The function signature for
addPacketToSendremains unchanged, but the context shows that a functiongetEndpointSenderhas been removed. Ensure that this function is not used elsewhere in the codebase, or that its functionality has been replaced appropriately.src/Nitwork/ANitwork.hpp (4)
29-29: The
stopmethod has been moved from public to protected. Ensure that this change does not break any code that calls this method from outside the class.33-33: The
startmethod has been moved from public to protected and theoverridekeyword has been removed. Ensure that this change does not break any code that calls this method from outside the class or relies on it being an override of a base class method.48-60: The new code checks if the packet is a resend before updating the packet ID and header. This is a good performance optimization as it avoids unnecessary updates for resend packets.
79-83: The
getEndpointSendermethod has been removed. Ensure that this change does not break any code that calls this method.src/Nitwork/NitworkServer.hpp (2)
24-28: The function signature for
startServerhas been changed. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, theipparameter has been removed and a new parameternbPlayerhas been added. Verify if this change is intentional and if it doesn't break any existing functionality.87-87: A new member variable
_maxNbPlayerhas been added. This seems to be used for limiting the number of players. Ensure that this variable is being properly initialized and used where necessary.src/Nitwork/NitworkClient.hpp (1)
- 24-29: The function signature for
starthas been changed tostartClientand the order of parameters has been modified. Ensure that all calls to this function throughout the codebase have been updated to match the new signature and parameter order.- bool start( - int port, - int threadNb = DEFAULT_THREAD_NB, - int tick = TICKS_PER_SECOND, - const std::string &ip = "") final; + bool startClient( + int port, + const std::string &ip, + int threadNb = DEFAULT_THREAD_NB, + int tick = TICKS_PER_SECOND);src/Nitwork/NitworkServer.cpp (3)
25-33: The function signature for
startServerhas been changed and a new parameternbPlayerhas been added. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, the IP parameter has been removed, verify if this change doesn't affect the functionality.116-122: The maximum number of clients (
MAX_CLIENTS) has been replaced with_maxNbPlayer. This change allows for a dynamic number of maximum players, which can be set when starting the server. Ensure that_maxNbPlayeris always properly set before this function is called to prevent unexpected behavior.135-144: The server now checks if the number of connected clients is less than
_maxNbPlayerbefore sending a star wave message. If there are not enough clients, the function returns early. This change ensures that the game starts only when all clients are ready. Make sure this change doesn't affect other parts of the code that rely on the star wave message.src/main_server.cpp (2)
- 31-48: The
checkArgsfunction now checks for two arguments instead of one, and the usage message has been updated accordingly. The function now uses a vector to store the arguments and checks if each argument is a number using theisNumberfunction. It also checks if the first argument is a valid port number and if the second argument is greater than 0. This is a good approach, but the error messages could be more specific to help users understand what went wrong. For example, the function could specify which argument is invalid and what the valid range for each argument is.- Logger::error("Invalid argument"); + Logger::error("Invalid argument: " + arg + ". Both arguments must be numbers."); - Logger::error("Invalid port or playerNb"); + Logger::error("Invalid port or player number. Port must be between " + std::to_string(PORT_MIN) + " and " + std::to_string(PORT_MAX) + ". Player number must be greater than 0.");
- 61-63: The
startServerfunction now takes two arguments instead of one. The first argument is the port number, and the second argument is the number of players. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (4)
- src/Nitwork/ANitwork.hpp (3 hunks)
- src/Nitwork/NitworkClient.hpp (1 hunks)
- src/Nitwork/NitworkServer.cpp (3 hunks)
- src/Nitwork/NitworkServer.hpp (2 hunks)
Additional comments (Suppressed): 9
src/Nitwork/NitworkClient.hpp (1)
- 22-29: The
startfunction has been renamed tostartClientand the order of parameters has been changed. Theipparameter is now required and is no longer the last parameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature and parameter order.src/Nitwork/ANitwork.hpp (3)
34-34: The
startfunction has been marked asfinal, which means it cannot be overridden in derived classes. Ensure that this change does not affect any subclasses that were previously overriding this function.50-61: The
sendDatafunction has been modified to update the packet ID and header only if the packet is not a resend. This is a good approach to avoid unnecessary operations for resending packets. However, ensure that theisResendflag is properly managed in the code where packets are being sent.79-85: The
getEndpointSenderfunction has been removed. If this function was used elsewhere in the codebase, those references will need to be updated or removed.src/Nitwork/NitworkServer.hpp (2)
24-28: The function signature for
startServerhas been changed. Theipparameter has been removed and a newnbPlayerparameter has been added. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the removal of theipparameter does not affect the functionality of the server.87-87: A new member variable
_maxNbPlayerhas been introduced. This variable is used to store the maximum number of players that can connect to the server. Ensure that this variable is properly initialized and updated throughout the codebase.src/Nitwork/NitworkServer.cpp (3)
25-29: The function signature of
startServerhas been changed. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, theipparameter has been removed and replaced withnbPlayer. Verify that this change does not affect the functionality of the server.112-118: The check for the maximum number of clients has been updated to use the
_maxNbPlayervariable instead of theMAX_CLIENTSconstant. This change allows for a dynamic maximum number of players. However, ensure that_maxNbPlayeris always properly initialized before this check to avoid unexpected behavior.131-137: A new condition has been added to check if the number of connected clients is less than
_maxNbPlayer. This change seems to be in line with the new feature of having a dynamic maximum number of players. However, ensure that this condition does not interfere with the expected flow of the program.
+ fix codeRabbit requests PATCH
…:X-R-G-B/R-Bus into fix/Nitwork-dont-change-resent-packet-id
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (2)
- src/main_client.cpp (3 hunks)
- src/main_server.cpp (2 hunks)
Additional comments (Suppressed): 7
src/main_server.cpp (2)
21-24: The
isNumberfunction is a good addition to validate if a string is a number. It uses thestd::all_offunction from the STL and the::isdigitfunction to check if all characters in the string are digits.56-56: The
startServerfunction now takes an additional argument for the number of players. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.src/main_client.cpp (5)
8-10: The inclusion of the header files "Logger.hpp", "NitworkClient.hpp", and "Registry.hpp" remains unchanged. No issues found.
16-19: The
isNumberfunction is a new addition. It checks if a string is a number by checking if all characters are digits. This is a more efficient and safer way to validate numeric strings compared to the previous method.21-37: The
checkArgsfunction has been updated to use theisNumberfunction for port validation. This is a more efficient and safer way to validate the port number. The function now also checks if the IP address is empty, which is a good addition for error handling.39-42: The
mainfunction signature has been updated to useconst char **avinstead ofchar **av. This is a good practice as it prevents accidental modification of the command-line arguments.47-53: The
startmethod ofNitworkClienthas been renamed tostartClientand now takes the IP address as a separate parameter. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.
2 participants
PATCH
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
startfunction in bothNitworkClientandNitworkServerclasses for better clarity and functionality. TheNitworkClient'sstartfunction now includes an optional IP parameter, and theNitworkServer'sstartfunction includes a parameter for the maximum number of players.