Freature/rb 159 enhance waves #139
Conversation
|
Important Auto Review SkippedDraft detected. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe changes mainly revolve around the enhancement of the game's wave system, introducing more enemy types, and refining the JSON handling. 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: 7
Configuration used: CodeRabbit UI
Files ignored due to filter (25)
- assets/Audio/Musics/Napstablook.mp3
- assets/Audio/Musics/Papyrus.mp3
- assets/Boss/green-head-boss.png
- assets/Boss/le-jutteur-boss.png
- assets/Boss/napstablook.png
- assets/Boss/papyrus.png
- assets/Json/bullets.json
- assets/Json/enemies.json
- assets/Json/enemyData.json
- assets/Json/playerData.json
- assets/Json/terminator.json
- assets/Json/wave.json
- assets/Json/waves.json
- assets/R-TypeSheet/bee_1.png
- assets/R-TypeSheet/big purple boss.png
- assets/R-TypeSheet/blue boos.png
- assets/R-TypeSheet/classic-bullets.png
- assets/R-TypeSheet/crawler-attack.png
- assets/R-TypeSheet/daemon.png
- assets/R-TypeSheet/dragon.png
- assets/R-TypeSheet/hornet-attack.png
- assets/R-TypeSheet/jutteurs-bullet.png
- assets/R-TypeSheet/rat-attack.png
- assets/R-TypeSheet/scorpion-attack.png
- assets/R-TypeSheet/spaceship.png
Files selected for processing (22)
- src/Client/Raylib/Graphics/Graphics.cpp (3 hunks)
- src/Client/Raylib/Graphics/Graphics.hpp (3 hunks)
- src/Client/Systems/Events/EventsSystems.cpp (2 hunks)
- src/Client/Systems/Network/ClientNetwork.cpp (1 hunks)
- src/ECS/ECSCustomTypes.cpp (2 hunks)
- src/ECS/ECSCustomTypes.hpp (2 hunks)
- src/ECS/Json.cpp (2 hunks)
- src/ECS/Json.hpp (2 hunks)
- src/ECS/MessageTypes.h (1 hunks)
- src/ECS/Systems/BulletsSystems.cpp (2 hunks)
- src/ECS/Systems/CMakeLists.txt (2 hunks)
- src/ECS/Systems/Systems.cpp (2 hunks)
- src/ECS/Systems/Systems.hpp (2 hunks)
- src/ECS/Systems/WaveSystems.cpp (1 hunks)
- src/Nitwork/Nitwork.h (1 hunks)
- src/Nitwork/NitworkServer.cpp (3 hunks)
- src/Nitwork/NitworkServer.hpp (1 hunks)
- src/Server/CMakeLists.txt (1 hunks)
- src/Server/Systems/CMakeLists.txt (1 hunks)
- src/Server/Systems/WaveSystem.cpp (1 hunks)
- src/Server/Systems/WaveSystem.hpp (1 hunks)
- src/main_server.cpp (1 hunks)
Files skipped from review due to trivial changes (6)
- src/ECS/Json.cpp
- src/ECS/Systems/CMakeLists.txt
- src/ECS/Systems/Systems.hpp
- src/Nitwork/Nitwork.h
- src/Server/CMakeLists.txt
- src/Server/Systems/CMakeLists.txt
Additional comments: 41
src/ECS/MessageTypes.h (1)
- 13-22: The new enemy types have been added correctly to the
enemy_type_eenum. Ensure that these new types are handled correctly in all parts of the code where this enum is used. Also, make sure that theMAX_ENEMY_TYPEis updated whenever a new enemy type is added.src/ECS/Systems/BulletsSystems.cpp (2)
48-54: The template argument
<std::string>is specified for thegetJsonObjectByIdfunction. This change is consistent with the pull request summary and seems to be a necessary update following changes in theJsonclass. Ensure that thegetJsonObjectByIdfunction is updated to handle this template argument correctly.108-114: The template argument
<std::string>is specified for thegetJsonObjectByIdfunction. This change is consistent with the pull request summary and seems to be a necessary update following changes in theJsonclass. Ensure that thegetJsonObjectByIdfunction is updated to handle this template argument correctly.src/Client/Systems/Network/ClientNetwork.cpp (1)
- 54-61: The
handleStartWavefunction is updating thewaveIdandwaitingForNextWaveproperties of theWaveInfossingleton instance. It's also adding theinitWavesystem to theSystemManagerfor theGAMEscene. This seems to be a part of the wave management logic. Ensure that theinitWavesystem is properly defined and that it's the correct system to add at this point. Also, make sure that thewaveIdis being correctly set and that thewaitingForNextWaveflag is being correctly updated.src/ECS/Systems/Systems.cpp (2)
204-209: The
checkDestroyAfterDeathCallBackfunction is using a mutex to ensure thread safety. This is a good practice to prevent data races when multiple threads are accessing and modifying the same data. However, it's important to ensure that the lock is released in all code paths to prevent deadlocks. In this case, thestd::lock_guardis used which automatically releases the lock when the scope ends, which is a good practice.399-408: The
getECSSystemsfunction is combining multiple system functions into a single vector. This is a good practice for modularity and maintainability, as it allows for easy addition or removal of systems. However, it's important to ensure that all systems are compatible and can be executed in the order they are added to the vector.src/Client/Systems/Events/EventsSystems.cpp (2)
55-61: The function
getJsonObjectByIdis now templated and the template argument<std::string>is used. This indicates that the function now returns astd::string. Similarly, the functiongetDataFromJsonis also templated and the template argument<float>is used. This indicates that the function now returns afloat. Ensure that these changes are reflected throughout the codebase where these functions are used.100-106: The function
getJsonObjectByIdis now templated and the template argument<std::string>is used. This indicates that the function now returns astd::string. Similarly, the functiongetDataFromJsonis also templated and the template argument<Types::CollisionRect>is used. This indicates that the function now returns aTypes::CollisionRect. Ensure that these changes are reflected throughout the codebase where these functions are used.src/Server/Systems/WaveSystem.cpp (1)
- 90-94: Good use of lock guards in these functions for thread safety.
src/Nitwork/NitworkServer.hpp (1)
- 24-28: The function signature for
addStarWaveMessagehas been changed. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that thewaveIdparameter is being correctly used within the function.src/Client/Raylib/Graphics/Graphics.cpp (4)
589-592: The
drawTextfunction now takes thetextparameter as a const reference to a string. This is a good practice as it avoids unnecessary copying of the string.606-615: The
Textconstructor has an additional parameterkeywordof type const reference to a string. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.704-707: The
Textclass has two new member functions:getTextandgetKeyword, both returning const references to strings. This is a good practice as it provides a way to access these private members without modifying them.709-712: The
Textclass has a new member functionsetTextthat takes a const reference to a string. This is a good practice as it provides a way to modify the private member_text.src/ECS/ECSCustomTypes.cpp (3)
1-14: The inclusion of "Raylib.hpp" is conditional on the CLIENT macro. Ensure that this macro is defined correctly in the build system when building for the client.
99-104: The
prepareNextWave()function clears_remainingEnemiesand sets_waitingForNextWaveto true. Ensure that this function is called at the appropriate times in the game logic to prepare for the next wave of enemies.106-138: The
setWaitingForNextWave()function has a significant amount of logic that is conditional on the CLIENT macro. This function updates the text displayed to the user and manages entities in the registry. Ensure that this function is called at the appropriate times in the game logic to update the user interface and manage entities correctly.src/Nitwork/NitworkServer.cpp (3)
11-14: The new include directive for "WaveSystem.hpp" is added. Ensure that the file exists and is in the correct location.
216-222: The
addSystemcall is changed fromSystems::initWavetoSystems::waveHandler. Ensure that thewaveHandlerfunction exists and is correctly implemented in theSystemsnamespace.- director.getSystemManager(0).addSystem(Systems::initWave); + director.getSystemManager(0).addSystem(Systems::waveHandler);
- 270-281: The
addStarWaveMessagefunction's signature is changed to take an_id_tparameter namedwaveIdinstead ofboost::asio::ip::udp::endpointandn_id_tparameters. The function implementation is also modified to use thewaveIdparameter in themsgStartWavestruct. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.- void NitworkServer::addStarWaveMessage(boost::asio::ip::udp::endpoint endpoint, n_id_t enemyNb) + void NitworkServer::addStarWaveMessage(n_id_t waveId)src/Client/Raylib/Graphics/Graphics.hpp (4)
178-184: The
Textclass has been updated with a new constructor that accepts akeywordparameter. This parameter is not used in any of thedrawmethods. If it's not used elsewhere in the class, consider removing it to avoid confusion.192-197: New getter and setter methods have been added for the
Textclass. These methods seem to follow good practices for encapsulation and data hiding.203-212: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [199-206]
A new private member variable
_keywordhas been added to theTextclass. As mentioned earlier, if this variable is not used elsewhere in the class, consider removing it.
- 209-211: New functions have been added to draw text and measure its width. These functions seem to be well-defined and follow good practices.
src/ECS/ECSCustomTypes.hpp (2)
115-206: The
WaveInfosclass is a singleton, which is generally discouraged due to its global state and potential for creating tightly coupled code. However, if it's necessary for your use case, it's implemented correctly here. The class provides methods for managing wave information, such as setting the wave ID, adding enemies, getting remaining enemies, removing the first enemy, and checking if there are remaining enemies. The class also manages a clock ID, a flag for the first enemy created, and a flag for waiting for the next wave. The class is thread-safe due to the use ofstd::lock_guardin theEnemyclass.115-226: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [208-238]
The
Enemyclass has been updated with an additional constructor that takes anenemy_id_sparameter. A static methodisEnemyAlive()is added to check if there are any alive enemies. The method retrieves all entities with theEnemycomponent and checks if the list is empty. The class is thread-safe due to the use ofstd::lock_guard.src/Server/Systems/WaveSystem.hpp (1)
- 18-35: The
Waveclass has a static mutex_mutex. Be careful when using static mutexes as they can lead to deadlocks if not handled properly. Also, consider making_clockIdprivate if it's not intended to be accessed directly from outside the class.src/ECS/Systems/WaveSystems.cpp (11)
1-6: The file header comment is good practice for providing basic information about the file.
8-14: The included libraries and headers seem to be necessary for the code in this file.
16-22: The conditional compilation for client and server-specific code is a good practice for code that needs to behave differently depending on the environment.
26-34: The
enemiesTypesmap is a good way to associate enemy types with their string identifiers.36-43: The
getEnemyIdfunction correctly checks if the enemy type exists in the map before returning its value. The error handling withstd::runtime_erroris appropriate.45-54: The
getEnemyTypefunction correctly iterates over the map to find the enemy type associated with the given string identifier. The error handling withstd::runtime_erroris appropriate.56-63: The
getEnemieDatafunction correctly retrieves the enemy data from the JSON object.67-116: The
initEnemyfunction seems to correctly initialize an enemy with the given type and position. It also correctly checks if the enemy has an ID and sets it if it does. The use of conditional compilation for client-specific code is appropriate.120-142: The
initWavefunction correctly initializes a wave of enemies. It correctly locks the registry withstd::lock_guardto prevent data races. It also correctly removes the system from the system manager after initializing the wave.144-172: The
updateWavefunction correctly handles the updating of the wave. It correctly checks if the wave has ended and prepares the next wave if it has. It also correctly handles the creation of enemies.176-179: The
getWaveSystemsfunction correctly returns a vector of functions for updating the wave.src/ECS/Json.hpp (3)
19-27: The changes to the
JsonTypeenum andpathToJsonmap are consistent with the pull request summary. Ensure that the removedDEFAULT_ENEMYandTERMINATORvalues are not used elsewhere in the codebase, and that the newENEMIESvalue and corresponding JSON file path are correctly used.60-75: The
getObjectsIdInArrayfunction is a new addition that retrieves a vector of object IDs from a specified array in the JSON data. It appears to be correctly implemented, but ensure that thearrayNamepassed to this function corresponds to an actual array in the JSON data.77-90: The
getJsonObjectByIdfunction has been updated to accept a template parameterTand return anlohmann::jsonobject. It appears to be correctly implemented, but ensure that thearrayNamepassed to this function corresponds to an actual array in the JSON data, and that theidpassed to this function corresponds to an actual ID in the specified array.
|
|
||
| lock.unlock(); | ||
| while (isRunning && Nitwork::NitworkServer::getInstance().isRunning()) { | ||
| lock.lock(); | ||
| director.getSystemManager(0).updateSystems(); | ||
| lock.unlock(); | ||
| try { | ||
| lock.lock(); | ||
| director.getSystemManager(0).updateSystems(); | ||
| lock.unlock(); | ||
| } catch (const std::exception &e) { | ||
| Logger::fatal("UpdateSystems: " + std::string(e.what())); | ||
| break; | ||
| } | ||
| } | ||
| Nitwork::NitworkServer::getInstance().stop(); | ||
| return 0; |
There was a problem hiding this comment.
The error handling added to the updateSystems() function is a good practice. However, the lock.unlock(); statement in line 70 is not guaranteed to be executed if an exception is thrown in the updateSystems() function. This could lead to a deadlock situation where the mutex remains locked. To ensure the mutex is always unlocked, even in the case of an exception, consider using std::lock_guard instead of std::unique_lock. std::lock_guard automatically unlocks the mutex when it goes out of scope, which includes when an exception is thrown.
- std::unique_lock<std::mutex> lock(director.mutex);
+ std::lock_guard<std::mutex> lock(director.mutex);Commitable suggestion (Beta)
| lock.unlock(); | |
| while (isRunning && Nitwork::NitworkServer::getInstance().isRunning()) { | |
| lock.lock(); | |
| director.getSystemManager(0).updateSystems(); | |
| lock.unlock(); | |
| try { | |
| lock.lock(); | |
| director.getSystemManager(0).updateSystems(); | |
| lock.unlock(); | |
| } catch (const std::exception &e) { | |
| Logger::fatal("UpdateSystems: " + std::string(e.what())); | |
| break; | |
| } | |
| } | |
| Nitwork::NitworkServer::getInstance().stop(); | |
| return 0; | |
| std::lock_guard<std::mutex> lock(director.mutex); | |
| while (isRunning && Nitwork::NitworkServer::getInstance().isRunning()) { | |
| try { | |
| director.getSystemManager(0).updateSystems(); | |
| } catch (const std::exception &e) { | |
| Logger::fatal("UpdateSystems: " + std::string(e.what())); | |
| break; | |
| } | |
| } | |
| Nitwork::NitworkServer::getInstance().stop(); | |
| return 0; |
| Wave::Wave() : _waveIndex(0), _msBeforeNextWave(0), _isGameEnded(false), _isTimeBetweenWaves(false) | ||
| { | ||
| try { | ||
| _wavesId = Json::getInstance().getObjectsIdInArray<std::size_t>(JsonType::WAVE, "waves"); | ||
| } catch (const std::exception &e) { | ||
| Logger::fatal("WaveInit: " + std::string(e.what())); | ||
| } | ||
| if (_wavesId.empty()) { | ||
| Logger::error("WaveInit: No waves found"); | ||
| _isGameEnded = true; | ||
| return; | ||
| } | ||
| _clockId = Registry::getInstance().getClock().create(); | ||
| startNextWave(); | ||
| } |
There was a problem hiding this comment.
The constructor of Wave class is doing a lot of work. Consider breaking it down into smaller, more manageable functions for better readability and maintainability. Also, it's a good practice to wrap the entire constructor in a try-catch block to handle any exceptions that might be thrown during initialization.
src/Server/Systems/WaveSystem.cpp
Outdated
| // Called when the last enemy of the wave is killed and the time between waves is over | ||
| void Wave::startNextWave() | ||
| { | ||
| auto &director = Systems::SystemManagersDirector::getInstance(); | ||
| Nitwork::NitworkServer &server = Nitwork::NitworkServer::getInstance(); | ||
| std::lock_guard<std::mutex> lock(_mutex); | ||
|
|
||
| if (_waveIndex > _wavesId.size()) { | ||
| _isGameEnded = true; | ||
| return; | ||
| } | ||
| try { | ||
| std::size_t id = _wavesId.at(_waveIndex); | ||
| nlohmann::json waveData = | ||
| Json::getInstance().getJsonObjectById<std::size_t>(JsonType::WAVE, id, "waves"); | ||
| _msBeforeNextWave = Json::getInstance().getDataFromJson<std::size_t>(waveData, "msBeforeNextWave"); | ||
| Types::WaveInfos::getInstance().setWaveId(static_cast<unsigned int>(id)); | ||
| server.addStarWaveMessage(static_cast<n_id_t>(_wavesId.at(_waveIndex))); | ||
| _waveIndex++; | ||
| } catch (const std::exception &e) { | ||
| Logger::fatal("WaveInit: " + std::string(e.what())); | ||
| _isGameEnded = true; | ||
| return; | ||
| } | ||
| director.getSystemManager(0).addSystem(Systems::initWave); | ||
| } |
There was a problem hiding this comment.
The startNextWave function is using a lock guard which is a good practice for thread safety. However, the function is quite long and complex. Consider breaking it down into smaller functions for better readability and maintainability.
| bool Wave::isWaveEnded() const | ||
| { | ||
| if (Types::WaveInfos::getInstance().getWaitingForNextWave() == false | ||
| || Types::WaveInfos::getInstance().isEnemyRemaining() == true || Types::Enemy::isEnemyAlive()) { | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The isWaveEnded function is checking multiple conditions in a single if statement. This can make the code harder to read and debug. Consider breaking these conditions into separate if statements or creating helper functions for each condition.
src/Server/Systems/WaveSystem.cpp
Outdated
| void waveHandler(std::size_t /*unused*/, std::size_t /*unused*/) | ||
| { | ||
| static Wave waveHandler; | ||
| Registry ®istry = Registry::getInstance(); | ||
| std::lock_guard<std::mutex> lock(registry.mutex); | ||
|
|
||
| if (waveHandler.isGameEnded()) { | ||
| std::cout << "------------------ GAME ENDED ------------------" << std::endl; | ||
| } | ||
|
|
||
| if (waveHandler.isWaveEnded() && waveHandler.isGameEnded() == false) { | ||
| if (waveHandler.isTimeBetweenWaves() == false) { | ||
| waveHandler.setTimeBetweenWaves(true); | ||
| registry.getClock().restart(Wave::_clockId); | ||
| return; | ||
| } | ||
| if (registry.getClock().elapsedMillisecondsSince(Wave::_clockId) | ||
| >= waveHandler.getMsBeforeNextWave()) { | ||
| waveHandler.setTimeBetweenWaves(false); | ||
| waveHandler.startNextWave(); | ||
| Types::WaveInfos::getInstance().setWaitingForNextWave(false); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The waveHandler function is quite long and complex. Consider breaking it down into smaller functions for better readability and maintainability. Also, it's a good practice to wrap the entire function in a try-catch block to handle any exceptions that might be thrown during execution.
| /* | ||
| ** EPITECH PROJECT, 2023 | ||
| ** R-Bus | ||
| ** File description: | ||
| ** Wave systems serverside | ||
| */ |
There was a problem hiding this comment.
The file description is not informative. Consider providing a more detailed description of the file's purpose and functionality.
| void waveHandler(std::size_t /*unused*/, std::size_t /*unused*/); | ||
| } // namespace Systems |
There was a problem hiding this comment.
The waveHandler function has two parameters that are marked as unused. If these parameters are not needed, consider removing them to avoid confusion.
- void waveHandler(std::size_t /*unused*/, std::size_t /*unused*/);
+ void waveHandler();Commitable suggestion (Beta)
| void waveHandler(std::size_t /*unused*/, std::size_t /*unused*/); | |
| } // namespace Systems | |
| void waveHandler(); | |
| } // namespace Systems |
…Bus into freature/RB-159-enhance-waves
…Bus into freature/RB-159-enhance-waves
…Bus into freature/RB-159-enhance-waves
…Bus into freature/RB-159-enhance-waves
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
New Features:
Textclass with new functions and parameters, improving text handling in the game.WaveInfosclass for managing waves of enemies, enhancing gameplay dynamics.enemy_type_eenum, diversifying enemy encounters.Bug Fixes:
json.getJsonObjectByIdandjson.getDataFromJsonfunctions for more specific data retrieval.waveIdandwaitingForNextWaveproperties update inWaveInfos, improving wave management.Refactor:
manageBossandinitWavefunctions, simplifying the game system.addStarWaveMessagefunction's signature and implementation, improving wave initiation.Chores:
target_sourcescommand.NitworkServerclass.Tests:
updateSystems()function, improving system stability.