Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/rb 119 fix json #95

Merged
merged 9 commits into from
Oct 14, 2023
Merged

Fix/rb 119 fix json #95

merged 9 commits into from
Oct 14, 2023

Conversation

TTENSHII
Copy link
Collaborator

@TTENSHII TTENSHII commented Oct 14, 2023

  • 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 the way our software retrieves and handles data from JSON files, improving the reliability and efficiency of data extraction. This change is not directly visible to users but improves the overall performance and stability of the application.
  • New Feature: Added conditional execution paths for client and non-client builds, allowing for more flexible and optimized performance based on the specific build type.
  • Refactor: Streamlined the initialization of various data structures, leading to cleaner code and potentially faster execution times. This contributes to the overall robustness of the application.

@TTENSHII TTENSHII self-assigned this Oct 14, 2023
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 14, 2023

Walkthrough

The changes primarily focus on refactoring the code to use template functions for retrieving data from JSON. This includes updates to the getDataFromJson function calls to specify the template argument and direct construction of objects from JSON data. The changes also involve handling different code paths for CLIENT and non-CLIENT builds.

Changes

File Path Summary
src/Client/Systems/Graphic/ParallaxSystems.cpp Refactored to use template functions for JSON data retrieval. Updated getDataFromJson function calls to specify template argument.
src/ECS/Json.cpp Removed unused getDataFromJson function and associated Logger::error usage.
src/ECS/Json.hpp Added new template function getDataFromJson for retrieving data from JSON based on a given index.
src/ECS/Systems/Systems.cpp Updated code related to adding enemy entities. Handled different code paths for CLIENT and non-CLIENT builds. Modified data retrieval from JSON and component insertion into the registry.

"In the land of code, where logic is king, 🤴
A rabbit hopped by, making changes with a zing. 🐇💥
Templates for JSON, a clever trick, 🎩🐇
Making the code cleaner, and the performance slick. ⚡️💻
Celebrate the changes, for they are fine, 🎉
In the world of code, where everything's a design!" 🌐💡


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.

@TTENSHII TTENSHII added the bug Something isn't working label Oct 14, 2023
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 1

Commits Files that changed from the base of the PR and between 37f68da and 3bec14b.
Files ignored due to filter (1)
  • assets/Json/enemyData.json
Files selected for processing (4)
  • src/Client/Systems/Graphic/ParallaxSystems.cpp (2 hunks)
  • src/ECS/Json.cpp (2 hunks)
  • src/ECS/Json.hpp (3 hunks)
  • src/ECS/Systems/Systems.cpp (2 hunks)
Files skipped from review due to trivial changes (1)
  • src/ECS/Json.cpp
Additional comments (Suppressed): 6
src/Client/Systems/Graphic/ParallaxSystems.cpp (2)
  • 17-33: The new hunk has improved the way Types::Position, Types::Velocity, and Types::Rect objects are constructed from the JSON data. The explicit type specification in the getDataFromJson function calls improves code clarity and type safety. However, the Registry::getInstance().getComponents<Types::Rect>().insertBack((rect)); line seems to be inserting a Types::Rect object into the registry without associating it with any entity. This could lead to issues when trying to retrieve the component for a specific entity. Please verify if this is the intended behavior.
- Registry::getInstance().getComponents<Types::Rect>().insertBack((rect));
+ Registry::getInstance().getComponents<Types::Rect>().insert(id, rect);
  • 51-57: The new hunk is identical to the old hunk. No changes have been made in this section. The code checks if the "copy" field exists in the JSON data and if it's true, it creates an additional parallax entity with an offset position. This logic seems to be correct and well-implemented.
src/ECS/Systems/Systems.cpp (2)
  • 186-230: The new code has improved the type safety and clarity by explicitly specifying the data types in the function calls to getDataFromJson. However, there is a potential issue with the addEntity function call in the non-CLIENT build. In the CLIENT build, the entity id is stored in the id variable, but in the non-CLIENT build, the entity id is not stored. This could lead to inconsistencies if the entity id is needed later in the non-CLIENT build. Please verify if this is the intended behavior.
- Registry::getInstance().addEntity();
+ std::size_t id = Registry::getInstance().addEntity();
  • 386-392: The changes in this hunk are similar to the previous one, improving type safety and clarity by explicitly specifying the data types in the function calls to getDataFromJson. No issues are found in this hunk.
src/ECS/Json.hpp (2)
  • 9-12: The "Logger.hpp" header file has been added to Json.hpp. This is in line with the PR summary and is necessary for the new getDataFromJson function to log errors.

  • 36-37: The getDataFromJson function has been removed. This is in line with the PR summary and is replaced by a new templated getDataFromJson function.

src/ECS/Json.hpp Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between 3bec14b and 6a57ba3.
Files selected for processing (2)
  • src/Client/Systems/Graphic/ParallaxSystems.cpp (2 hunks)
  • src/ECS/Systems/Systems.cpp (2 hunks)
Additional comments (Suppressed): 4
src/Client/Systems/Graphic/ParallaxSystems.cpp (2)
  • 17-32: The new hunk has improved the way Position, Velocity, and Rect objects are constructed from JSON data. The explicit template arguments make the code more readable and less prone to errors. However, ensure that the getDataFromJson function can handle cases where the requested data is not present or is of a different type. Also, verify that the Types::Position, Types::Velocity, and Types::Rect classes have appropriate constructors that can handle the data returned by getDataFromJson.

  • 50-56: The logic for checking the "copy" field in JSON data and creating an additional parallax entity if necessary remains the same. The use of getDataFromJson<bool> improves readability and makes the intention of the code clearer. Ensure that all JSON data used in the codebase includes the "copy" field where necessary, and that it is always of type bool.

src/ECS/Systems/Systems.cpp (2)
  • 186-231: The new code has improved the way JSON data is retrieved and used to initialize various components of the game entities. The getDataFromJson function is now a template function, which allows for more flexible data retrieval. This change has been propagated throughout the codebase, with explicit template arguments specified in all function calls. The code is more readable and maintainable now. However, ensure that the JSON data being retrieved contains the expected types and values.

  • 387-392: The getDataFromJson function is used to retrieve animation rectangle data from the JSON object. This is a good use of the new template function, as it allows for more flexible data retrieval. However, ensure that the JSON data being retrieved contains the expected types and values.

@Saverio976 Saverio976 merged commit dd949ff into dev Oct 14, 2023
@Saverio976 Saverio976 deleted the fix/RB-119-Fix-json branch October 17, 2023 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants