Fix/rb 141 fix last release problem

[guillaumeAbel]

• Please check if the PR fulfills these requirements

• The commit message follows our guidelines
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)
• Norm of the code has been respected

• In what state is your pull request?

• Ready to merge / Waiting a reviwer to see my work.
• Work In Progress (WIP) / My work is not finish but i want daily reviews. (Draft)
• CI Review / I want to see what the CI thinks of my work.
• Need feedback / Just to have feedback on what i produced. (May not be merged)

• What kind of change does this PR introduce? (You can choose multiple)

• Bug fix
• Feature request
• New / Updated documentation
• Testing CI ( Make the pull request in draft mode)

• 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

Refactor:

• Enhanced thread safety across various functions to ensure smooth and reliable performance.
• Streamlined the process of updating system managers, improving the software's efficiency.
• Improved error handling in the run() function, increasing the software's robustness.

New Feature:

• Added new template functions to the Json class for more flexible data retrieval from JSON objects.

Bug Fix:

• Fixed potential data race issues in the SystemManager class by removing the usage of a mutex lock.
• Adjusted the initialization and usage of the SystemManagersDirector class to ensure proper synchronization and prevent data races.

Note: These changes primarily improve the software's performance and reliability. They may not result in noticeable changes for end-users but are crucial for maintaining a high-quality user experience.

[coderabbitai]

Walkthrough

The changes primarily focus on enhancing thread safety by introducing mutex locks in various functions across multiple files. Additionally, there's a refactoring of some functions for better maintainability and readability. The changes also include the introduction of new template functions for JSON data retrieval and modifications in function signatures and calls.

Changes

File(s)	Summary
src/Client/SceneManager.cpp	Introduced mutex locks for thread safety and refactored the update logic into a new function. Added exception handling.
src/Client/Systems/Events/EventsSystems.cpp, src/Client/Systems/Graphic/*.cpp	Added mutex locks in various functions for thread safety.
src/Client/Systems/Network/ClientNetwork.cpp	Refactored to use SystemManagersDirector singleton. Modified function calls with updated arguments.
src/ECS/Json.hpp	Added new template functions for retrieving data from JSON objects.
src/ECS/Systems/Managers/SystemManager.cpp	Removed mutex locks, potentially introducing data race issues.
src/ECS/Systems/Systems.cpp, src/ECS/Systems/Systems.hpp	Refactored functions for better readability. Modified function signatures and calls.
src/Nitwork/NitworkServer.cpp	Added mutex locks for thread safety. Modified function calls with updated arguments.
src/main_server.cpp	Introduced mutex for thread safety when accessing SystemManagersDirector instance.

"In the land of code, where logic is king, 🤴
A rabbit hopped in, making changes in a swing. 🐇💨
Thread safety ensured, with a mutex lock, 🔒
Refactoring done, against the race, we now can block. 🏁
JSON data retrieved, with a template function call, 📞
Celebrate these changes, for they benefit us all!" 🎉🥳

---

Tips

Chat with CodeRabbit Bot (@coderabbitai)

• Mention @coderabbitai in any review comment for bot assistance.
• Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

• Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.
• Additionally, enter @coderabbitai pause to halt the bot's ongoing reviews as any issue or review comment. To
  resume reviews, enter @coderabbitai resume as any issue or review comment.

[coderabbitai]

Review Status

Actionable comments generated: 11

Details

Commits

Files that changed from the base of the PR and between a0692a4 and 7628a47.

Files selected for processing (14)

• src/Client/SceneManager.cpp (2 hunks)
• src/Client/Systems/Events/EventsSystems.cpp (3 hunks)
• src/Client/Systems/Graphic/AudioSystems.cpp (1 hunks)
• src/Client/Systems/Graphic/DeathSystems.cpp (1 hunks)
• src/Client/Systems/Graphic/ParallaxSystems.cpp (1 hunks)
• src/Client/Systems/Graphic/SpriteSystems.cpp (4 hunks)
• src/Client/Systems/Graphic/TextSystems.cpp (1 hunks)
• src/Client/Systems/Network/ClientNetwork.cpp (3 hunks)
• src/ECS/Json.hpp (1 hunks)
• src/ECS/Systems/Managers/SystemManager.cpp (3 hunks)
• src/ECS/Systems/Systems.cpp (7 hunks)
• src/ECS/Systems/Systems.hpp (1 hunks)
• src/Nitwork/NitworkServer.cpp (3 hunks)
• src/main_server.cpp (1 hunks)

Additional comments (Suppressed): 27

src/ECS/Systems/Systems.hpp (1)

• 29-29: The function signature for initPlayer has been changed. The playerType parameter has been removed. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the playerType is being determined correctly within the function itself.

src/Client/Systems/Graphic/AudioSystems.cpp (1)

• 64-64: The addition of a mutex lock here is a good practice for ensuring thread safety during the initialization of the audio system. This will prevent potential data races when multiple threads are trying to access or modify the Registry instance concurrently.

src/Client/Systems/Graphic/ParallaxSystems.cpp (1)

• 44-49: The addition of a mutex lock here is a good practice for thread safety, especially when accessing shared resources like the Registry instance. However, ensure that all other functions that access the Registry instance also use a similar locking mechanism to prevent data races.

src/Client/Systems/Events/EventsSystems.cpp (3)

• 33-39: Mutex lock has been added to ensure thread safety during concurrent access to the Registry instance. This is a good practice to prevent data races.

• 68-73: Mutex lock has been added to ensure thread safety during concurrent access to the Registry instance. This is a good practice to prevent data races.

• 110-115: Mutex lock has been added to ensure thread safety during concurrent access to the Registry instance. This is a good practice to prevent data races.

src/main_server.cpp (3)

• 56-58: Ensure that the startServer function is thread-safe and can handle concurrent calls. If not, consider adding a mutex lock around this function call.

• 66-68: The updateSystems function is called within a mutex lock, which is good for thread safety. However, ensure that this function does not take too long to execute, as it could cause other threads to be blocked for a significant amount of time.

• 70-70: Ensure that the stop function is thread-safe and can handle concurrent calls. If not, consider adding a mutex lock around this function call.

src/Client/Systems/Network/ClientNetwork.cpp (4)

• 46-55: The mutex lock has been moved from Registry::getInstance().mutex to SystemManagersDirector::getInstance().mutex. Ensure that this change does not introduce any race conditions or deadlocks, especially if other threads are also accessing the SystemManagersDirector instance.

• 64-64: The initPlayer function call has been updated to only include the playerId parameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

• 76-78: The initEnemy function now takes in the pos variable for the enemy's position. Ensure that this change does not introduce any inconsistencies in the game, especially if the enemy's position is used elsewhere in the code.

• 87-87: The initPlayer function call has been updated to only include the playerId and a boolean parameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

src/Nitwork/NitworkServer.cpp (3)

• 133-139: The addition of a mutex lock here is a good step towards ensuring thread safety. However, it's important to note that the lock is acquired and released within the same function. This could potentially lead to a race condition if another thread modifies _endpoints after the lock is released but before the function returns. Consider using a more granular locking mechanism or restructure your code to minimize the time the lock is held.

• 154-154: The function call to Systems::initPlayer has been modified. The playerType parameter has been removed from the function call. Ensure that this change is intentional and that the initPlayer function can correctly determine the playerType within the function itself.

• 177-178: The addition of a mutex lock here is a good step towards ensuring thread safety. However, it's important to note that the lock is acquired and released within the same function. This could potentially lead to a race condition if another thread modifies the SystemManager after the lock is released but before the function returns. Consider using a more granular locking mechanism or restructure your code to minimize the time the lock is held.

src/Client/Systems/Graphic/SpriteSystems.cpp (4)

• 84-89: The mutex lock is acquired before the Registry instance is retrieved. This is a good practice for thread safety as it prevents data races during concurrent access. However, it's important to ensure that the lock is released as soon as the data is no longer needed to avoid potential deadlocks.

• 98-103: Similar to the previous comment, the mutex lock is acquired before the Registry instance is retrieved. This is a good practice for thread safety as it prevents data races during concurrent access. However, it's important to ensure that the lock is released as soon as the data is no longer needed to avoid potential deadlocks.

• 196-201: The mutex lock is acquired before the Registry instance is retrieved. This is a good practice for thread safety as it prevents data races during concurrent access. However, it's important to ensure that the lock is released as soon as the data is no longer needed to avoid potential deadlocks.

• 213-218: The mutex lock is acquired before the Registry instance is retrieved. This is a good practice for thread safety as it prevents data races during concurrent access. However, it's important to ensure that the lock is released as soon as the data is no longer needed to avoid potential deadlocks.

src/ECS/Json.hpp (1)

• 69-72: The getDataByJsonType function has been updated to use a template parameter. This change allows for more flexibility in the types of data that can be retrieved from the JSON object. However, it's important to ensure that the data type T is compatible with the data stored in the JSON object. If the data type is not compatible, the nlohmann::json::get<T>() function will throw an exception.

src/ECS/Systems/Systems.cpp (6)

• 55-66: The windowCollision function now calls the checkOutsideWindow function for both ids and idsOtherPlayer. Ensure that this change is intended and that it does not introduce any unintended side effects.

• 199-203: The addEntity function call in the for loop no longer assigns the return value to a variable. This could potentially lead to unused entities if the return value is not used elsewhere. Please verify if this change is intended.

• 255-258: The manageBoss function now uses a mutex lock for thread safety. This is a good practice to prevent data races during concurrent access.

• 286-289: The initWave function now uses a mutex lock for thread safety. This is a good practice to prevent data races during concurrent access.

• 436-448: The initPlayer function no longer takes a playerType parameter and instead determines it within the function. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

• 496-498: The createMissile function no longer assigns the return value of addEntity to a variable. This could potentially lead to unused entities if the return value is not used elsewhere. Please verify if this change is intended.

[coderabbitai]

Review Status

Actionable comments generated: 1

Details

Commits

Files that changed from the base of the PR and between 7628a47 and 1984d1e.

Files selected for processing (2)

• src/Client/Systems/Network/ClientNetwork.cpp (3 hunks)
• src/ECS/Systems/Systems.cpp (7 hunks)

Additional comments (Suppressed): 9

src/Client/Systems/Network/ClientNetwork.cpp (4)

• 46-55: The mutex lock has been moved from Registry::getInstance().mutex to SystemManagersDirector::getInstance().mutex. Ensure that this change does not introduce any race conditions or deadlocks, especially if SystemManagersDirector::getInstance() and Registry::getInstance() are accessed concurrently in other parts of the code.

• 60-64: The initPlayer function call has been changed from initPlayer(JsonType::DEFAULT_PLAYER, playerInit.playerId) to initPlayer(playerInit.playerId). Ensure that this change does not affect the initialization of the player, especially if the JsonType::DEFAULT_PLAYER parameter was used to set default values for the player.

• 66-78: The mutex lock has been retained on Registry::getInstance().mutex, which is good for thread safety. However, the position initialization for the new enemy has been moved from after the initEnemy function call to before it. Ensure that this change does not affect the correct initialization of the enemy's position.

• 83-87: The initPlayer function call has been changed from initPlayer(JsonType::DEFAULT_PLAYER, newAllie.playerId, true) to initPlayer(newAllie.playerId, true). Ensure that this change does not affect the initialization of the ally, especially if the JsonType::DEFAULT_PLAYER parameter was used to set default values for the ally.

src/ECS/Systems/Systems.cpp (5)

• 55-66: The windowCollision function has been refactored to split the logic into two separate functions, checkOutsideWindow for Types::Player and Types::OtherPlayer. This improves the modularity of the code and makes it easier to understand and maintain. However, ensure that the checkOutsideWindow function is thread-safe before calling it.

• 199-215: The initEnemy function has been refactored to remove the conditional compilation directive #ifdef CLIENT that was previously used to add an entity to the Registry. This change simplifies the code and makes it more consistent. However, ensure that this change does not affect the client-specific functionality that was previously implemented.

• 272-291: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [255-284]

Mutex locks have been added to the manageBoss and initWave functions to ensure thread safety when accessing the Registry instance. This is a good practice to prevent data races when multiple threads are accessing or modifying the Registry.

• 436-449: The initPlayer function has been refactored to use the new template functions getDataByVector for retrieving deadTime and health values from the JSON configuration. This improves the code readability and maintainability. However, ensure that the correct data types are used when calling these template functions.

• 496-502: The createMissile function has been refactored to remove the conditional compilation directive #ifdef CLIENT that was previously used to add an entity to the Registry. This change simplifies the code and makes it more consistent. However, ensure that this change does not affect the client-specific functionality that was previously implemented.