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.

Sign up for GitHub

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

Jump to bottom

Custom config #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Custom config #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion vita3k/app/include/app/functions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ enum class AppRunType {
};

void init_paths(Root &root_paths);
bool init(EmuEnvState &state, Config &cfg, const Root &root_paths);
bool init(EmuEnvState &state, const Root &root_paths);
bool late_init(EmuEnvState &state);
void destroy(EmuEnvState &emuenv, ImGui_State *imgui);
void update_viewport(EmuEnvState &state);
Expand Down
2 changes: 1 addition & 1 deletion vita3k/app/src/app.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ void set_window_title(EmuEnvState &emuenv) {
const std::string title_to_set = fmt::format("{} | {} ({}) | {} | {} FPS ({} ms) | {}x{}{} | {}",
window_title,
emuenv.current_app_title, emuenv.io.title_id,
emuenv.cfg.backend_renderer,
emuenv.cfg.current_config.backend_renderer,
emuenv.fps, emuenv.ms_per_frame,
x, y, af, emuenv.cfg.current_config.screen_filter);

Expand Down
6 changes: 2 additions & 4 deletions vita3k/app/src/app_init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,9 +229,7 @@ void init_paths(Root &root_paths) {
fs::create_directories(root_paths.get_log_path() / "texturelog");
}

bool init(EmuEnvState &state, Config &cfg, const Root &root_paths) {
state.cfg = std::move(cfg);

bool init(EmuEnvState &state, const Root &root_paths) {
Copy link

Choose a reason for hiding this comment

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

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.

state.base_path = root_paths.get_base_path();
state.default_path = root_paths.get_pref_path();
state.log_path = root_paths.get_log_path();
Expand Down Expand Up @@ -268,7 +266,7 @@ bool init(EmuEnvState &state, Config &cfg, const Root &root_paths) {

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") {
Copy link

Choose a reason for hiding this comment

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

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.

Suggested change
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;

#ifndef __APPLE__
state.backend_renderer = renderer::Backend::OpenGL;
#else
Expand Down
1 change: 1 addition & 0 deletions vita3k/config/include/config/state.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ struct Config : YamlLoader {
int audio_volume = 100;
bool ngs_enable = true;
bool pstv_mode = false;
std::string backend_renderer = "OpenGL";
Copy link

Choose a reason for hiding this comment

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

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.

bool high_accuracy = false;
float resolution_multiplier = 1.0f;
bool disable_surface_sync = false;
Expand Down
2 changes: 1 addition & 1 deletion vita3k/gui/include/gui/functions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link

Choose a reason for hiding this comment

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

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 to set_config do not include the custom parameter.
  • In settings_dialog.cpp, one call to set_config does not include the custom parameter.

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);

  • settings_dialog.cpp

    • Line: set_config(emuenv, emuenv.io.app_path);
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

void set_shaders_compiled_display(GuiState &gui, EmuEnvState &emuenv);
void update_app(GuiState &gui, EmuEnvState &emuenv, const std::string &app_path);
void update_last_time_app_used(GuiState &gui, EmuEnvState &emuenv, const std::string &app);
Expand Down
49 changes: 39 additions & 10 deletions vita3k/gui/src/settings_dialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
#include <string>
#include <util/fs.h>
#include <util/log.h>
#include <util/string_utils.h>

#include <SDL.h>

Expand Down Expand Up @@ -138,7 +139,7 @@ static CPUBackend config_cpu_backend;
* @return false A custom config for the application has not been found or a custom config has been found
* but it's corrupted or invalid.
*/
static bool get_custom_config(GuiState &gui, EmuEnvState &emuenv, const std::string &app_path) {
static bool get_custom_config(EmuEnvState &emuenv, const std::string &app_path) {
const auto CUSTOM_CONFIG_PATH{ emuenv.config_path / "config" / fmt::format("config_{}.xml", app_path) };

if (fs::exists(CUSTOM_CONFIG_PATH)) {
Expand Down Expand Up @@ -166,6 +167,7 @@ static bool get_custom_config(GuiState &gui, EmuEnvState &emuenv, const std::str
// Load GPU Config
if (!config_child.child("gpu").empty()) {
const auto gpu_child = config_child.child("gpu");
config.backend_renderer = gpu_child.attribute("backend-renderer").as_string();
config.high_accuracy = gpu_child.attribute("high-accuracy").as_bool();
config.resolution_multiplier = gpu_child.attribute("resolution-multiplier").as_float();
config.disable_surface_sync = gpu_child.attribute("disable-surface-sync").as_bool();
Expand Down Expand Up @@ -231,11 +233,12 @@ static std::vector<std::string> list_user_lang;
void init_config(GuiState &gui, EmuEnvState &emuenv, const std::string &app_path) {
// If no app-specific config file is being used for the initialized application,
// set up `config` with the values set in the global emulator configuration
if (!get_custom_config(gui, emuenv, app_path)) {
if (!get_custom_config(emuenv, app_path)) {
config.cpu_backend = emuenv.cfg.cpu_backend;
config.cpu_opt = emuenv.cfg.cpu_opt;
config.modules_mode = emuenv.cfg.modules_mode;
config.lle_modules = emuenv.cfg.lle_modules;
config.backend_renderer = emuenv.cfg.backend_renderer;
config.high_accuracy = emuenv.cfg.high_accuracy;
config.resolution_multiplier = emuenv.cfg.resolution_multiplier;
config.disable_surface_sync = emuenv.cfg.disable_surface_sync;
Expand Down Expand Up @@ -276,6 +279,14 @@ void init_config(GuiState &gui, EmuEnvState &emuenv, const std::string &app_path
current_aniso_filter_log = static_cast<int>(log2f(static_cast<float>(config.anisotropic_filtering)));
max_aniso_filter_log = static_cast<int>(log2f(static_cast<float>(emuenv.renderer->get_max_anisotropic_filtering())));
audio_backend_idx = (emuenv.cfg.audio_backend == "SDL") ? 0 : 1;
#ifndef __APPLE__
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
Comment on lines +283 to +289
Copy link

Choose a reason for hiding this comment

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

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:

  1. Centralize Backend Selection Logic: Abstract the platform-specific differences into a separate function or configuration structure to centralize the backend selection logic.
  2. 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.cpp
  • vita3k/app/src/app_init.cpp
  • vita3k/main.cpp
  • vita3k/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.cpp
  • vita3k/renderer/src/vulkan/renderer.cpp
  • vita3k/renderer/src/vulkan/scene.cpp
  • vita3k/mem/src/mem.cpp
  • vita3k/gui/src/vita3k_update.cpp

Recommendations:

  1. 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.
  2. 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

emuenv.app_path = app_path;
emuenv.display.imgui_render = true;
}
Expand Down Expand Up @@ -319,6 +330,7 @@ static void save_config(GuiState &gui, EmuEnvState &emuenv) {

// GPU
auto gpu_child = config_child.append_child("gpu");
gpu_child.append_attribute("backend-renderer") = config.backend_renderer.c_str();
gpu_child.append_attribute("high-accuracy") = config.high_accuracy;
gpu_child.append_attribute("resolution-multiplier") = config.resolution_multiplier;
gpu_child.append_attribute("disable-surface-sync") = config.disable_surface_sync;
Expand Down Expand Up @@ -357,6 +369,7 @@ static void save_config(GuiState &gui, EmuEnvState &emuenv) {
emuenv.cfg.modules_mode = config.modules_mode;
emuenv.cfg.lle_modules = config.lle_modules;
emuenv.cfg.high_accuracy = config.high_accuracy;
emuenv.cfg.backend_renderer = config.backend_renderer;
emuenv.cfg.resolution_multiplier = config.resolution_multiplier;
emuenv.cfg.disable_surface_sync = config.disable_surface_sync;
emuenv.cfg.screen_filter = config.screen_filter;
Expand All @@ -383,7 +396,7 @@ static void save_config(GuiState &gui, EmuEnvState &emuenv) {
}

std::string get_cpu_backend(GuiState &gui, EmuEnvState &emuenv, const std::string &app_path) {
if (!get_custom_config(gui, emuenv, app_path))
if (!get_custom_config(emuenv, app_path))
return emuenv.cfg.cpu_backend;

return config.cpu_backend;
Expand All @@ -410,17 +423,19 @@ static void set_vsync_state(const bool &state) {
* @param emuenv State of the emulated PlayStation Vita environment
* @param app_path Path to the app or game to get the custom config for
*/
void set_config(GuiState &gui, EmuEnvState &emuenv, const std::string &app_path) {
void set_config(EmuEnvState &emuenv, const std::string &app_path, bool custom) {
// If a config file is in use, call `get_custom_config()` and set the config
// parameters with the values stored in the app-specific custom config file
if (get_custom_config(gui, emuenv, app_path))
if (custom && get_custom_config(emuenv, app_path)) {
emuenv.cfg.current_config = config;
else {
LOG_INFO("Using custom config for app: {}", app_path);
} else {
// Else inherit the values from the global emulator config
emuenv.cfg.current_config.cpu_backend = emuenv.cfg.cpu_backend;
emuenv.cfg.current_config.cpu_opt = emuenv.cfg.cpu_opt;
emuenv.cfg.current_config.modules_mode = emuenv.cfg.modules_mode;
emuenv.cfg.current_config.lle_modules = emuenv.cfg.lle_modules;
emuenv.cfg.current_config.backend_renderer = emuenv.cfg.backend_renderer;
emuenv.cfg.current_config.high_accuracy = emuenv.cfg.high_accuracy;
emuenv.cfg.current_config.resolution_multiplier = emuenv.cfg.resolution_multiplier;
emuenv.cfg.current_config.disable_surface_sync = emuenv.cfg.disable_surface_sync;
Expand All @@ -439,6 +454,19 @@ void set_config(GuiState &gui, EmuEnvState &emuenv, const std::string &app_path)
emuenv.cfg.current_config.psn_signed_in = emuenv.cfg.psn_signed_in;
}

// can happen when launching directly into a game, the renderer is not even initialized yet
if (!emuenv.renderer)
return;

#ifndef __APPLE__
if (string_utils::toupper(emuenv.cfg.current_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

// If backend render or resolution multiplier is changed when app run, reboot emu and app
if (!emuenv.io.title_id.empty() && ((emuenv.renderer->current_backend != emuenv.backend_renderer) || (emuenv.renderer->res_multiplier != emuenv.cfg.current_config.resolution_multiplier))) {
emuenv.load_exec = true;
Expand Down Expand Up @@ -583,7 +611,7 @@ void draw_settings_dialog(GuiState &gui, EmuEnvState &emuenv) {
#endif
static const char *LIST_BACKEND_RENDERER[] = { "OpenGL", "Vulkan" };
if (ImGui::Combo(lang.gpu["backend_renderer"].c_str(), reinterpret_cast<int *>(&emuenv.backend_renderer), LIST_BACKEND_RENDERER, IM_ARRAYSIZE(LIST_BACKEND_RENDERER)))
emuenv.cfg.backend_renderer = LIST_BACKEND_RENDERER[int(emuenv.backend_renderer)];
config.backend_renderer = LIST_BACKEND_RENDERER[int(emuenv.backend_renderer)];
SetTooltipEx(lang.gpu["select_backend_renderer"].c_str());
#ifdef __APPLE__
ImGui::EndDisabled();
Expand All @@ -593,7 +621,8 @@ void draw_settings_dialog(GuiState &gui, EmuEnvState &emuenv) {

const bool is_vulkan = (emuenv.backend_renderer == renderer::Backend::Vulkan);
const bool is_ingame = !emuenv.io.title_id.empty();
if (is_vulkan) {
const bool is_renderer_changed = (emuenv.backend_renderer != emuenv.renderer->current_backend);
if (is_vulkan && !is_renderer_changed) {
const std::vector<std::string> gpu_list_str = emuenv.renderer->get_gpu_list();
// must convert to a vector of char*
std::vector<const char *> gpu_list;
Expand All @@ -612,7 +641,7 @@ void draw_settings_dialog(GuiState &gui, EmuEnvState &emuenv) {

if (is_ingame)
ImGui::EndDisabled();
} else {
} else if (!is_vulkan) {
ImGui::Checkbox(lang.gpu["v_sync"].c_str(), &config.v_sync);
SetTooltipEx(lang.gpu["v_sync_description"].c_str());
ImGui::SameLine();
Expand Down Expand Up @@ -1228,7 +1257,7 @@ void draw_settings_dialog(GuiState &gui, EmuEnvState &emuenv) {
if (ImGui::Button(is_apply ? (is_reboot ? lang.main_window["save_reboot"].c_str() : lang.main_window["save_apply"].c_str()) : lang.main_window["save"].c_str(), BUTTON_SIZE)) {
save_config(gui, emuenv);
if (is_apply)
set_config(gui, emuenv, emuenv.io.app_path);
set_config(emuenv, emuenv.io.app_path);
}
SetTooltipEx(lang.main_window["keep_changes"].c_str());

Expand Down
19 changes: 13 additions & 6 deletions vita3k/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{};
Copy link

Choose a reason for hiding this comment

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

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.

Config &cfg = emuenv.cfg;

const auto config_err = config::init_config(cfg, argc, argv, root_paths);

// to make sure config.current_config is properly initialized...
gui::set_config(emuenv, "", false);

fs::create_directories(cfg.get_pref_path());

if (config_err != Success) {
Expand Down Expand Up @@ -197,10 +201,13 @@ int main(int argc, char *argv[]) {
LOG_INFO("Available ram memory: {} MiB", SDL_GetSystemRAM());

app::AppRunType run_type = app::AppRunType::Unknown;
if (cfg.run_app_path)
if (cfg.run_app_path) {
run_type = app::AppRunType::Extracted;
emuenv.config_path = root_paths.get_config_path();
gui::set_config(emuenv, *cfg.run_app_path);
}

if (!app::init(emuenv, cfg, root_paths)) {
if (!app::init(emuenv, root_paths)) {
app::error_dialog("Emulated environment initialization failed.", emuenv.window.get());
return 1;
}
Expand Down Expand Up @@ -319,15 +326,15 @@ int main(int argc, char *argv[]) {
}
}

gui::set_config(emuenv, emuenv.io.app_path);

// When backend render is changed before boot app, reboot emu in new backend render and run app
if (emuenv.renderer->current_backend != emuenv.backend_renderer) {
emuenv.load_app_path = emuenv.io.app_path;
run_execv(argv, emuenv);
return Success;
}

gui::set_config(gui, emuenv, emuenv.io.app_path);

const auto APP_INDEX = gui::get_app_index(gui, emuenv.io.app_path);
emuenv.app_info.app_version = APP_INDEX->app_ver;
emuenv.app_info.app_category = APP_INDEX->category;
Expand Down
Loading