Feature/rb 110 function receive new enemy#84
Conversation
WalkthroughThe changes primarily focus on enhancing the game's networking capabilities, improving the enemy initialization process, and adding extensive logging for debugging. The code now supports a network system manager, and the enemy struct has been refactored for clarity. The Changes
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 12
Files selected for processing (16)
- src/Client/SceneManager.cpp (1 hunks)
- src/Client/SceneManager.hpp (2 hunks)
- src/Client/Systems/Events/EventsSystems.cpp (1 hunks)
- src/Client/Systems/Graphic/AudioSystems.cpp (1 hunks)
- src/Client/Systems/Graphic/DeathSystems.cpp (1 hunks)
- src/Client/Systems/Graphic/ParallaxSystems.cpp (3 hunks)
- src/Client/Systems/Graphic/SpriteSystems.cpp (1 hunks)
- src/Client/Systems/Graphic/TextSystems.cpp (1 hunks)
- src/Client/Systems/Network/ClientNetwork.cpp (1 hunks)
- src/ECS/ECSCustomTypes.hpp (2 hunks)
- src/ECS/SparseArray.hpp (1 hunks)
- src/ECS/Systems/Managers/SystemManager.cpp (1 hunks)
- src/ECS/Systems/Systems.cpp (6 hunks)
- src/ECS/Systems/Systems.hpp (1 hunks)
- src/Nitwork/NitworkServer.cpp (1 hunks)
- src/Server/Systems/Network/ServerNetwork.cpp (1 hunks)
Files skipped from review due to trivial changes (2)
- src/Client/Systems/Graphic/DeathSystems.cpp
- src/Server/Systems/Network/ServerNetwork.cpp
Additional comments (Suppressed): 15
src/ECS/Systems/Systems.hpp (1)
- 18-18: The function signature for
initEnemyhas been changed to include two new parameters:bool setIdandstruct ::enemy_id_s enemyId. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, consider the implications of the default values for these parameters.src/Client/SceneManager.hpp (2)
21-21: The
SystemManagersenum has been extended to include aNETWORKvalue. Ensure that this new enum value is properly handled in all places whereSystemManagersis used.36-38: The
_scenesarray has been updated to includeSystemManagers::NETWORKin both of its vectors. Make sure that the addition of theNETWORKsystem manager is properly initialized and managed in the corresponding scenes.src/Nitwork/NitworkServer.cpp (1)
- 104-107: The change from implicit to explicit casting of
_endpoints.size() - 1ton_id_tis a good practice for type safety. However, ensure that the value of_endpoints.size() - 1will always be within the range ofn_id_tto prevent any potential overflow or underflow issues.src/Client/Systems/Network/ClientNetwork.cpp (5)
66-79: The
receiveNewEnemyfunction has been updated to include thread safety withstd::lock_guard<std::mutex>. It also now initializes the enemy with an ID and updates the position and health components in the registry. Ensure that theinitEnemyfunction and the registry'sinsertBackfunction can handle these changes correctly.69-69: The use of
std::lock_guardis a good practice for thread safety. However, ensure that theRegistry::getInstance().mutexis unlocked in all other places where it is locked to avoid deadlocks.72-72: The
initEnemyfunction now takes additional arguments. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.74-75: The
newEnemy.enemyInfos.pos.xandnewEnemy.enemyInfos.pos.yare being cast tofloat. Ensure that this casting does not lead to loss of precision or unexpected behavior.77-78: The
insertBackfunction is being used to add new components to the registry. Ensure that this function handles the addition of new components correctly and that it does not lead to any issues with the ordering or indexing of components in the registry.src/ECS/ECSCustomTypes.hpp (3)
- 97-121: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [98-141]
The
Enemystruct has been modified. TheconstIdmember variable has been made public and the_typemember variable has been renamed totypeand also made public. The constructor has been updated to initializeconstIdandtype. ThegetConstId()function now returns a copy ofconstIdinstead of a reference. Ensure that these changes do not break any existing code that uses theEnemystruct. Also, consider if makingconstIdandtypepublic is necessary, as it breaks encapsulation. If external code needs to modify these variables, it might be better to provide setter functions instead.
99-106: The constructor of the
Enemystruct is using a mutex to protect the increment of_enemyNb. This is good for thread safety, but it's important to ensure that the mutex is also used wherever_enemyNbis read or written in other parts of the code, to prevent data races.114-119: The
getConstId()function is using a mutex to protect the reading ofconstId. This is good for thread safety, but it's important to ensure that the mutex is also used whereverconstIdis read or written in other parts of the code, to prevent data races.src/ECS/Systems/Systems.cpp (3)
9-20: The new hunk introduces a new include directive for
ECSCustomTypes.hppand moves the include directive forCustomTypes.hppunder the#ifdef CLIENTpreprocessor directive. Ensure that this change does not affect the rest of the codebase, especially ifCustomTypes.hppwas used outside of the client-specific code.194-235: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [182-233]
The
initEnemyfunction has been updated to take two additional arguments:bool setIdandstruct ::enemy_id_s enemyId. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, theTypes::Enemy enemyCompis now initialized withenemyIdinstead of an empty initializer. Make sure this change is intended and correctly handled in the rest of the codebase.
- 378-411: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [358-409]
The
initPlayerfunction now includes a logging statement at the start and end of the function. As with thewindowCollisionfunction, consider changing the log level or removing these statements if they're not necessary. Also, theRegistry::getInstance().getComponents<Types::PlayerDatas>().insertBack(playerDatas);line has been moved under the#ifdef CLIENTpreprocessor directive. Ensure that this change does not affect the server-specific code.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (6)
- src/Client/Systems/Network/ClientNetwork.cpp (1 hunks)
- src/ECS/ECSCustomTypes.hpp (2 hunks)
- src/ECS/Systems/Systems.cpp (6 hunks)
- src/ECS/Systems/Systems.hpp (1 hunks)
- src/Nitwork/NitworkServer.cpp (1 hunks)
- src/Server/Systems/Network/ServerNetwork.cpp (1 hunks)
Files skipped from review due to trivial changes (2)
- src/Nitwork/NitworkServer.cpp
- src/Server/Systems/Network/ServerNetwork.cpp
Additional comments (Suppressed): 11
src/ECS/Systems/Systems.hpp (1)
- 19-19: The function signature for
initEnemyhas been changed to include two new parameters:setIdandenemyId. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, consider the implications of the default values for these parameters.src/Client/Systems/Network/ClientNetwork.cpp (3)
66-79: The
receiveNewEnemyfunction has been updated to include thread safety measures and additional initialization parameters for new enemies. The use ofstd::lock_guardensures that the registry is not accessed concurrently, preventing potential data races. TheinitEnemyfunction now takes an additional parameter for setting the enemy ID. The position and health of the new enemy are also explicitly set. Ensure that these changes do not conflict with other parts of the codebase that interact with thereceiveNewEnemyfunction or theRegistryclass.72-72: The
initEnemyfunction now takes an additional boolean parameter. Ensure that this parameter is correctly set in all calls toinitEnemythroughout the codebase.77-78: The
insertBackfunction is used to add the position and health of the new enemy to the respective component arrays in the registry. Ensure that the order of these insertions does not affect the consistency of the entity-component relationships in the ECS.src/ECS/ECSCustomTypes.hpp (1)
- 97-121: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [98-141]
The
Enemystruct has been refactored. TheconstIdandtypemember variables are now public, and the constructor has been overloaded to allow for setting theconstIdandtypeupon creation. ThegetConstId()function now returns a copy ofconstIdinstead of a reference. Ensure that these changes do not break any existing code that relies on the old structure and behavior of theEnemystruct. Also, consider makingconstIdandtypeprivate and providing getter and setter functions for them to maintain encapsulation.src/ECS/Systems/Systems.cpp (6)
9-20: The
ECSCustomTypes.hppis included instead ofCustomTypes.hppin the new hunk. If this change is intentional, ensure that all the types used fromCustomTypes.hppare available inECSCustomTypes.hppto avoid any compilation errors.178-184: The function signature of
initEnemyhas been changed to include two new parameterssetIdandenemyId. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.192-227: The
initEnemyfunction has been refactored to include theenemyIdin theenemyCompinitialization. This change seems to be part of the effort to improve the management of enemy entities. Ensure that theenemyIdis correctly set and used in the game logic.374-402: The
initPlayerfunction has been refactored to include theplayerDatasin theCLIENTconditional compilation block. This change seems to be part of the effort to improve the management of player entities. Ensure that theplayerDatasis correctly set and used in the game logic.405-419: The
createMissilefunction has been refactored to include theentityIdin theCLIENTconditional compilation block. This change seems to be part of the effort to improve the management of missile entities. Ensure that theentityIdis correctly set and used in the game logic.438-444: The
createMissilefunction has been refactored to remove thecollisionRect1and only usecollisionRect(previouslycollisionRect2). This change seems to be part of the effort to simplify the collision rectangle management for missile entities. Ensure that thecollisionRectis correctly set and used in the game logic.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- src/ECS/Systems/Systems.cpp (7 hunks)
Additional comments (Suppressed): 4
src/ECS/Systems/Systems.cpp (4)
12-12: The
ECSCustomTypes.hppheader file is included instead ofCustomTypes.hpp. Ensure that this change is intentional and that the new header file contains all the necessary definitions.192-226: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [178-226]
The
initEnemyfunction now accepts two additional parameters:setIdandenemyId. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, theenemyIdis now used to initialize theenemyCompwhich was previously default initialized. Make sure this change doesn't introduce any unintended side effects.
- 384-402: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [373-402]
The
initPlayerfunction has been modified. ThepositionandcollisionRectcomponents are now initialized after the#endifdirective, which means they will be initialized regardless of whether theCLIENTmacro is defined. Ensure that this change is intentional and doesn't introduce any unintended side effects.
- 405-419: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [406-444]
The
createMissilefunction has been modified. TheentityIdis now only defined if theCLIENTmacro is defined. Also, thecollisionRect1component is no longer initialized. Ensure that these changes are intentional and don't introduce any unintended side effects.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (2)
- src/ECS/ECSCustomTypes.hpp (2 hunks)
- src/ECS/SparseArray.hpp (1 hunks)
Additional comments (Suppressed): 5
src/ECS/SparseArray.hpp (1)
- 68-74: The
back()function is a good addition to theSparseArrayclass. It provides a way to directly access the last element of the array, which can be useful in many scenarios. However, it's important to note that this function throws a runtime error when the array is empty. Make sure to handle this exception properly wherever this function is used.src/ECS/ECSCustomTypes.hpp (4)
98-106: The constructor
Enemy(enum enemy_type_e _type = enemy_type_e::CLASSIC_ENEMY)has been modified to directly initialize thetypemember variable. TheconstIdmember variable is now directly initialized using the static_enemyNbvariable. The mutex lock ensures thread safety when accessing the static_enemyNbvariable.108-112: A new constructor
Enemy(struct enemy_id_s _constId, enum enemy_type_e _type = enemy_type_e::CLASSIC_ENEMY)has been added. This constructor initializes theconstIdandtypemember variables directly.114-117: The
getConstId()function has been modified to return theconstIdmember variable directly, without acquiring a mutex lock. This change assumes that theconstIdmember variable is not modified after initialization, and therefore does not require thread safety.137-138: The static
_enemyNbvariable and_mutexare now private. This change improves encapsulation by preventing direct access to these variables from outside theEnemystruct.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- src/ECS/Systems/Systems.cpp (7 hunks)
Additional comments (Suppressed): 6
src/ECS/Systems/Systems.cpp (6)
16-20: The
ECSCustomTypes.hppheader file is included in the new hunk but not in the old hunk. Ensure that this new inclusion does not introduce any conflicts or redefinitions. Also, verify that the file exists and is accessible from the current file's location.178-184: The function signature of
initEnemyhas been changed to include two new parameters:bool setIdandstruct ::enemy_id_s enemyId. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the new parameters are used correctly within the function.187-229: The
initEnemyfunction has been significantly refactored. The new code introduces a conditional initialization of theenemyCompvariable based on thesetIdparameter. This change seems to be part of a new feature that allows enemies to be initialized with a specific ID. Ensure that this new feature is implemented correctly and does not introduce any bugs or unexpected behavior.388-406: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [377-406]
The
initPlayerfunction has been refactored. The new code introduces a conditional initialization of theplayerCompvariable based on thesetIdparameter. This change seems to be part of a new feature that allows players to be initialized with a specific ID. Ensure that this new feature is implemented correctly and does not introduce any bugs or unexpected behavior.
410-423: The
createMissilefunction has been refactored. The new code removes the initialization of thecollisionRect1variable and changes the initialization of thecollisionRectvariable. Ensure that this change does not introduce any bugs or unexpected behavior, especially in the collision detection and resolution logic.442-448: The
createMissilefunction has been refactored. The new code removes the insertion of thecollisionRect1component and changes the insertion of thecollisionRectcomponent. Ensure that this change does not introduce any bugs or unexpected behavior, especially in the collision detection and resolution logic.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- src/ECS/Systems/Systems.cpp (7 hunks)
Additional comments (Suppressed): 6
src/ECS/Systems/Systems.cpp (6)
12-20: The
ECSCustomTypes.hppheader file has been added and theCustomTypes.hppheader file has been moved under the#ifdef CLIENTdirective. Ensure that this change does not affect the rest of the codebase and that all necessary types are still accessible where needed.178-184: The
initEnemyfunction now takes two additional parameters,setIdandenemyId. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.187-229: The
initEnemyfunction has been significantly refactored. TheTypes::SpriteDatasstructure is now used instead ofRaylib::Spritefor theenemyvariable. TheTypes::Enemystructure is now initialized conditionally based on thesetIdparameter. TheTypes::RectandTypes::AnimRectcomponents are now added to the registry only in the client build. Ensure that these changes are compatible with the rest of the codebase and that the client and server builds behave as expected.387-405: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [376-405]
The
initPlayerfunction has been refactored. TheTypes::RectandTypes::AnimRectcomponents are now added to the registry only in the client build. Ensure that this change is compatible with the rest of the codebase and that the client and server builds behave as expected.
408-422: The
createMissilefunction has been refactored. TheentityIdvariable is now only defined in the client build. ThecollisionRect1component has been removed from the registry. Ensure that these changes are compatible with the rest of the codebase and that the client and server builds behave as expected.441-447: The
createMissilefunction has been refactored. TheTypes::Rectcomponent is now added to the registry only in the client build. ThecollisionRect1andcollisionRect2components have been replaced with a singlecollisionRectcomponent. Ensure that these changes are compatible with the rest of the codebase and that the client and server builds behave as expected.
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