NITWORK/SERVER-NETWORK: Fix wrong implementation of tick and receiveClientEnemyDeath #117
Conversation
receiveClientEnemyDeath PATCH
WalkthroughThe changes primarily revolve around the modification of time-related constants and parameters in the Nitwork namespace. The 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: 3
Files selected for processing (2)
- src/Nitwork/Nitwork.h (1 hunks)
- src/Server/Systems/Network/ServerNetwork.cpp (1 hunks)
Additional comments (Suppressed): 2
src/Nitwork/Nitwork.h (1)
- 15-15: The change in the value of
TICKS_PER_SECONDfrom60to1/256(or256if the above issue is addressed) is a significant change. Ensure that this change is intentional and that it does not negatively impact any code that relies on theTICKS_PER_SECONDmacro.src/Server/Systems/Network/ServerNetwork.cpp (1)
- 49-71: The refactoring of the
handleEnemyDeathfunction to use a loop instead ofstd::find_ifis a good performance optimization. However, the new code assumes that the enemy IDs are the same as the array indices, which might not always be the case. If the IDs and indices are not guaranteed to be the same, this could lead to incorrect behavior. Please verify this assumption.- for (auto &id : ids) { - if (arrEnemies[id].getConstId().id == msgEnemyDeath.enemyId.id) { + for (size_t i = 0; i < arrEnemies.size(); ++i) { + if (arrEnemies[i].getConstId().id == msgEnemyDeath.enemyId.id) {
romainpanno
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (5)
- src/Nitwork/Nitwork.h (1 hunks)
- src/Nitwork/NitworkClient.hpp (1 hunks)
- src/Nitwork/NitworkServer.hpp (1 hunks)
- src/main_client.cpp (1 hunks)
- src/main_server.cpp (1 hunks)
Files skipped from review due to trivial changes (2)
- src/Nitwork/Nitwork.h
- src/Nitwork/NitworkServer.hpp
Additional comments (Suppressed): 3
src/Nitwork/NitworkClient.hpp (1)
- 29-29: The
tickparameter's default value has been updated fromTICKS_PER_SECONDtoTICK. Ensure that this change is reflected in all calls to this function throughout the codebase and that the newTICKvalue is appropriate for the intended functionality.src/main_client.cpp (1)
- 49-50: The
startClientfunction now takesTICKas the last argument instead ofTICKS_PER_SECOND. Ensure that this change is intentional and that theTICKvalue is correctly calculated and used throughout the codebase.src/main_server.cpp (1)
- 56-56: The
startServerfunction now takes two additional arguments:DEFAULT_THREAD_NBandTICK. Ensure that these new parameters are correctly initialized and used throughout the codebase. Also, verify that the changes do not break any existing functionality or cause any side effects.- if (!Nitwork::NitworkServer::getInstance().startServer(std::stoi(av[1]), std::stoi(av[2]))) { + if (!Nitwork::NitworkServer::getInstance().startServer(std::stoi(av[1]), std::stoi(av[2]), DEFAULT_THREAD_NB, TICK)) {
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (3)
- src/Nitwork/NitworkClient.hpp (1 hunks)
- src/Nitwork/NitworkServer.hpp (1 hunks)
- src/main_server.cpp (1 hunks)
Files skipped from review due to trivial changes (1)
- src/Nitwork/NitworkClient.hpp
Additional comments (Suppressed): 2
src/main_server.cpp (1)
- 56-57: The
startServerfunction now accepts two additional arguments:DEFAULT_THREAD_NBandTICK. Ensure that these new parameters are correctly defined and initialized before this function call. Also, verify that the function signature ofstartServerhas been updated to accept these additional parameters without breaking existing functionality.src/Nitwork/NitworkServer.hpp (1)
- 24-24: The
startServerfunction signature has been updated to useTICKinstead ofTICKS_PER_SECONDas the default value for thetickparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the newTICKvalue does not negatively impact the server's performance or functionality.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- src/Nitwork/Nitwork.h (1 hunks)
Additional comments (Suppressed): 1
src/Nitwork/Nitwork.h (1)
- 16-16: The
TICKS_PER_SECONDconstant has been updated from 60 to 128. This change will affect the rate at which the game server updates, potentially improving the responsiveness of the game. Ensure that this change does not introduce performance issues, such as increased CPU usage or network traffic.
src/Nitwork/Nitwork.h
Outdated
| #define ONE_SECOND 1000 | ||
| #define TICKS_PER_SECOND 128 | ||
| #define TICK (ONE_SECOND / (TICKS_PER_SECOND * ONE_SECOND)) |
There was a problem hiding this comment.
The TICK constant has been introduced, calculated based on TICKS_PER_SECOND. However, the current calculation seems incorrect as it divides ONE_SECOND by the square of TICKS_PER_SECOND. This would result in a much smaller value than expected. The correct calculation should be ONE_SECOND / TICKS_PER_SECOND.
- #define TICK (ONE_SECOND / (TICKS_PER_SECOND * ONE_SECOND))
+ #define TICK (ONE_SECOND / TICKS_PER_SECOND)There was a problem hiding this comment.
@coderabbitai its because one second is in milliseconds, and ticks per second is 128 so i need to convert it in miliseconds by doing * 1000 is it correct ?
There was a problem hiding this comment.
@romainpanno You're correct that ONE_SECOND is defined as 1000, representing one second in milliseconds. However, the current calculation for TICK is still incorrect.
The purpose of TICK is to represent the duration of one tick in milliseconds. If TICKS_PER_SECOND is 128, then there are 128 ticks in one second. Therefore, the duration of one tick should be ONE_SECOND / TICKS_PER_SECOND, which gives the number of milliseconds per tick.
The current calculation ONE_SECOND / (TICKS_PER_SECOND * ONE_SECOND) is equivalent to 1 / TICKS_PER_SECOND, which would give a very small value in seconds, not milliseconds.
So, the correct calculation should indeed be ONE_SECOND / TICKS_PER_SECOND. This will give the duration of one tick in milliseconds, which seems to be what you're aiming for. Here's the corrected line:
- #define TICK (ONE_SECOND / (TICKS_PER_SECOND * ONE_SECOND))
+ #define TICK (ONE_SECOND / TICKS_PER_SECOND)This will ensure that TICK correctly represents the duration of one tick in milliseconds.
There was a problem hiding this comment.
@romainpanno Great! If you have any more questions or need further clarification, feel free to ask. Happy coding!
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (6)
- src/Nitwork/ANitwork.cpp (1 hunks)
- src/Nitwork/Nitwork.h (1 hunks)
- src/Nitwork/NitworkClient.hpp (1 hunks)
- src/Nitwork/NitworkServer.hpp (1 hunks)
- src/main_client.cpp (1 hunks)
- src/main_server.cpp (1 hunks)
Additional comments (Suppressed): 6
src/main_client.cpp (1)
- 50-50: The
startClientfunction call has been updated to useTICKSinstead ofTICKS_PER_SECOND. Ensure that theTICKSmacro is correctly defined and that this change does not affect the expected behavior of thestartClientfunction.src/main_server.cpp (1)
- 56-57: The
startServerfunction now takes two additional parameters:DEFAULT_THREAD_NBandTICKS. Ensure that these parameters are correctly defined and initialized before this function call. Also, verify that the function signature ofstartServerhas been updated to accept these additional parameters and that it handles them correctly.src/Nitwork/NitworkServer.hpp (1)
- 24-24: The default value of the
tickparameter in thestartServerfunction has been changed fromTICKS_PER_SECONDtoTICKS. Ensure that this change is reflected in all calls to this function throughout the codebase. Also, verify that the newTICKSvalue is correctly calculated and suitable for the game's tick mechanism.src/Nitwork/NitworkClient.hpp (1)
- 24-26: The function signature for
startClienthas been changed. Thetickparameter's default value has been updated fromTICKS_PER_SECONDtoTICKS. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the newTICKSvalue is correctly calculated and used in the context of this function.src/Nitwork/ANitwork.cpp (1)
- 60-66: The new hunk introduces a macro
TICKS_PER_MILLISECONDto calculate the sleep duration for the clock thread. Ensure that this macro is correctly defined and that it provides the expected value for the sleep duration. Also, consider usingstd::lock_guardorstd::unique_lockfor managing the mutex to ensure that it gets unlocked even if an exception is thrown.- _tickMutex.lock(); - _tickConvVar.notify_all(); - _tickMutex.unlock(); + std::lock_guard<std::mutex> lock(_tickMutex); + _tickConvVar.notify_all();src/Nitwork/Nitwork.h (1)
- 14-17: The new definitions of
TICKS,TICKS_PER_SECOND, andTICKS_PER_MILLISECONDseem to change the previous behavior. Ensure that these changes are propagated throughout the codebase and that they do not introduce any unexpected behavior, especially in time-sensitive operations. Also, theTICKS_PER_SECONDandTICKS_PER_MILLISECONDmacros now require an argumentt, which was not the case before. Make sure that all calls to these macros have been updated accordingly.
| #define TICKS 128 | ||
| #define TICKS_PER_SECOND(t) (t / TICKS) | ||
| #define TICKS_PER_MILLISECOND(t) (TICKS_PER_SECOND(t) / 1000) |
There was a problem hiding this comment.
The TICKS_PER_SECOND and TICKS_PER_MILLISECOND macros are now dependent on the TICKS macro. This introduces a coupling between these macros. If TICKS is changed in the future, it will affect the other two macros. Consider if this coupling is desired or if it would be better to keep these values independent.
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