Custom config #16
Custom config #16
Conversation
WalkthroughThe changes streamline and refine the handling of configuration settings by focusing on reducing parameter redundancy and enhancing direct access. The major alterations involve removing unnecessary parameters and introducing direct references to configuration fields. Additionally, there are updates to function signatures and some rearrangement of initialization logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as main.cpp
participant App as app_init.cpp
participant GUI as settings_dialog.cpp
Main->>App: init(emuenv, root_paths)
App-->>Main: returns boolean
Main->>Main: if (init succeeded)
activate GUI
Main->>GUI: set_config(emuenv, "", false)
GUI-->>Main: returns
deactivate GUI
Main->>GUI: set_config(emuenv, app_path)
GUI-->>Main: returns
sequenceDiagram
participant SetConfigFunction as functions.h
participant State as state.h
participant CustomDialog as settings_dialog.cpp
SetConfigFunction->>State: Access backend_renderer
activate State
State-->>SetConfigFunction: Initial value "OpenGL"
deactivate State
SetConfigFunction->>CustomDialog: set_config(emuenv, app_path, custom)
activate CustomDialog
CustomDialog->>State: Modify backend_renderer conditionally
State-->>CustomDialog: Returns modified backend_renderer
deactivate CustomDialog
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (7)
Files skipped from review as they are similar to previous changes (7)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -101,7 +101,7 @@ void pre_run_app(GuiState &gui, EmuEnvState &emuenv, const std::string &app_path | |||
| void reset_controller_binding(EmuEnvState &emuenv); | |||
| void save_apps_cache(GuiState &gui, EmuEnvState &emuenv); | |||
| void save_user(GuiState &gui, EmuEnvState &emuenv, const std::string &user_id); | |||
| void set_config(GuiState &gui, EmuEnvState &emuenv, const std::string &app_path); | |||
| void set_config(EmuEnvState &emuenv, const std::string &app_path, bool custom = true); | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The review of the set_config function calls in main.cpp and settings_dialog.cpp reveals that not all calls include the new custom parameter. Specifically:
- In
main.cpp, two calls toset_configdo not include thecustomparameter. - In
settings_dialog.cpp, one call toset_configdoes not include thecustomparameter.
These calls need to be updated to include the custom parameter to ensure consistent and correct usage of the function.
-
main.cpp
- Line:
gui::set_config(emuenv, *cfg.run_app_path); - Line:
gui::set_config(emuenv, emuenv.io.app_path);
- Line:
-
settings_dialog.cpp
- Line:
set_config(emuenv, emuenv.io.app_path);
- Line:
Analysis chain
Addition of custom parameter allows for flexible configuration handling.
The rg output indicates that not all calls to set_config include the new custom parameter. Specifically, the calls in main.cpp and settings_dialog.cpp need to be reviewed to ensure they are correctly updated.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all function calls to `set_config` include the new `custom` parameter.
# Test: Search for the function usage. Expect: All calls include the new parameter.
rg --type cpp $'set_config'
Length of output: 1162
Script:
#!/bin/bash
# Description: Verify specific lines in `main.cpp` and `settings_dialog.cpp` for correct usage of `set_config`.
# Display lines around the calls to `set_config` in `main.cpp`.
rg --context 5 --type cpp $'set_config' vita3k/main.cpp
# Display lines around the calls to `set_config` in `settings_dialog.cpp`.
rg --context 5 --type cpp $'set_config' vita3k/gui/src/settings_dialog.cpp
Length of output: 2594
nishinji
force-pushed
the
custom_config
branch
3 times, most recently
from
911369c
to
e2abdb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (2)
vita3k/main.cpp (1)
329-329: Review the necessity of re-setting the configuration on GUI state change.The call to
gui::set_configmight be redundant if the GUI configuration is already set correctly in previous steps. Consider removing this call if it is confirmed to be unnecessary to streamline the code.vita3k/gui/src/settings_dialog.cpp (1)
Line range hint
142-170: Refactorget_custom_configandinit_configfunctions for better maintainability.The
get_custom_configandinit_configfunctions are quite long and perform multiple tasks which could be separated into smaller, more manageable functions. This would improve readability and maintainability. Consider breaking down these functions into smaller ones that handle specific parts of the configuration loading and initialization process.Also applies to: 236-241
| bool init(EmuEnvState &state, Config &cfg, const Root &root_paths) { | ||
| state.cfg = std::move(cfg); | ||
|
|
||
| bool init(EmuEnvState &state, const Root &root_paths) { |
There was a problem hiding this comment.
Refactor the initialization function to improve modularity.
Consider breaking down the init function into smaller, more focused functions. This could improve readability and maintainability, especially as the function grows in complexity with more configuration parameters and initialization steps.
| @@ -268,7 +266,7 @@ | |||
|
|
|||
| state.backend_renderer = renderer::Backend::Vulkan; | |||
|
|
|||
| if (string_utils::toupper(state.cfg.backend_renderer) == "OPENGL") { | |||
| if (string_utils::toupper(state.cfg.current_config.backend_renderer) == "OPENGL") { | |||
There was a problem hiding this comment.
Ensure consistent handling of renderer backends.
The handling of the backend_renderer setting seems inconsistent. For OpenGL, you're setting state.backend_renderer directly, but for Vulkan, you're modifying state.cfg.backend_renderer and serializing the configuration. This could lead to unexpected behavior or bugs.
- state.cfg.backend_renderer = "Vulkan";
- config::serialize_config(state.cfg, state.cfg.config_path);
+ state.backend_renderer = renderer::Backend::Vulkan;Consider aligning the handling of these settings to ensure consistency.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (string_utils::toupper(state.cfg.current_config.backend_renderer) == "OPENGL") { | |
| if (string_utils::toupper(state.cfg.current_config.backend_renderer) == "OPENGL") { | |
| state.backend_renderer = renderer::Backend::Vulkan; |
| @@ -124,10 +124,14 @@ int main(int argc, char *argv[]) { | |||
| LOG_CRITICAL("PLEASE. DO NOT RUN VITA3K AS ADMIN OR WITH ADMIN PRIVILEGES."); | |||
| } | |||
|
|
|||
| Config cfg{}; | |||
| EmuEnvState emuenv; | |||
| EmuEnvState emuenv{}; | |||
There was a problem hiding this comment.
Initialization of EmuEnvState could be streamlined.
The initialization of EmuEnvState and its configuration could be encapsulated into a constructor or a dedicated initialization method within the EmuEnvState class. This would improve the structure and readability of the code.
| if (string_utils::toupper(config.backend_renderer) == "OPENGL") | ||
| emuenv.backend_renderer = renderer::Backend::OpenGL; | ||
| else | ||
| emuenv.backend_renderer = renderer::Backend::Vulkan; | ||
| #else | ||
| emuenv.backend_renderer = renderer::Backend::Vulkan; | ||
| #endif |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results confirm that there are multiple instances where the handling of the graphics backend renderer is platform-specific, particularly for Apple. This scattered approach can indeed lead to inconsistencies and maintenance challenges.
Key Findings:
- Platform-Specific Handling: The code includes several
#ifdef __APPLE__directives to handle backend renderer selection differently for Apple. - Backend Renderer Selection: In non-Apple platforms, the backend renderer can be either OpenGL or Vulkan, while for Apple, it defaults to Vulkan.
- Multiple Files Affected: The platform-specific handling is spread across various files, including
settings_dialog.cpp,app_init.cpp, and others.
Recommendations:
- Centralize Backend Selection Logic: Abstract the platform-specific differences into a separate function or configuration structure to centralize the backend selection logic.
- Refactor and Simplify: Minimize the use of platform-specific preprocessor directives by refactoring the code to handle platform differences in a more unified manner.
Specific Locations in the Codebase:
vita3k/gui/src/settings_dialog.cppvita3k/app/src/app_init.cppvita3k/main.cppvita3k/config/src/config.cpp
Actionable Directive:
Refactor the code to centralize the handling of the graphics backend renderer. This will ensure consistency across platforms and simplify future maintenance.
Analysis chain
Ensure consistent handling of graphics backend across platforms.
The handling of the graphics backend renderer differs between Apple and other platforms. This could lead to inconsistencies and maintenance challenges. Consider abstracting the platform-specific differences into a separate function or configuration structure to centralize the backend selection logic.
Also applies to: 462-468
The search results indicate several instances where platform-specific handling is implemented, particularly for Apple. This includes various configurations and adjustments specific to Apple's platform, which might lead to inconsistencies in the graphics backend handling.
To ensure consistent handling of the graphics backend across platforms, it is essential to centralize the backend selection logic. The current implementation has scattered platform-specific conditions that can lead to maintenance challenges and potential inconsistencies.
Specific locations in the codebase that need attention:
vita3k/renderer/src/vulkan/pipeline_cache.cppvita3k/renderer/src/vulkan/renderer.cppvita3k/renderer/src/vulkan/scene.cppvita3k/mem/src/mem.cppvita3k/gui/src/vita3k_update.cpp
Recommendations:
- Centralize Platform-Specific Logic: Abstract the platform-specific differences into a separate function or configuration structure. This will help maintain consistency and make the code easier to manage.
- Review and Refactor: Go through the platform-specific conditions and ensure they are necessary. If possible, refactor the code to minimize platform-specific branches.
Next Steps:
To gather more specific information about the handling of the graphics backend, I will generate a script to search for any additional platform-specific conditions related to the graphics backend selection.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Ensure that the handling of graphics backend is consistent across all platform-specific code sections.
# Test: Search for platform-specific handling of graphics backend. Expect: Consistency in handling.
rg --type cpp -e "ifdef __APPLE__" -A 10 -B 10
Length of output: 24712
Script:
#!/bin/bash
# Description: Ensure that the handling of graphics backend is consistent across all platform-specific code sections.
# Test: Search for platform-specific handling of graphics backend. Expect: Consistency in handling.
rg --type cpp -e "backend_renderer" -A 10 -B 10
Length of output: 45645
| @@ -129,6 +129,7 @@ struct Config : YamlLoader { | |||
| int audio_volume = 100; | |||
| bool ngs_enable = true; | |||
| bool pstv_mode = false; | |||
| std::string backend_renderer = "OpenGL"; | |||
There was a problem hiding this comment.
The addition of the backend_renderer field to the Config struct is consistent with the changes in other parts of the codebase. This field should be properly documented to explain its usage and potential values.
Consider adding documentation comments for the backend_renderer field to clarify its purpose and usage within the emulator settings.
nishinji
force-pushed
the
custom_config
branch
5 times, most recently
from
b123c7f
to
6b98b77
Compare
Co-authored-by: Macdu <[email protected]>
nishinji
force-pushed
the
master
branch
2 times, most recently
from
6b55eb6
to
152c713
Compare
No description provided.