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

Allowing Config class to resolve isFullscreen flag from CLI arguments #1349

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

robyn-dressler
Copy link
Contributor

Addresses feature request #1147

Changes:

  • Move CLI argument parsing to Config class
  • Added -f and --fullscreen flags that override isFullscreen in the config
  • Added patchFile member variable to config (but excluding from TOML file, since it's game specific)
  • Added second overload to Emulator::Run

I have unfortunately not been able to properly test fullscreen in my environment due to #1348, but I think the new CLI argument works.

src/emulator.cpp Outdated
@@ -256,6 +256,14 @@ void Emulator::Run(const std::filesystem::path& file) {
std::exit(0);
}

void Emulator::Run(int& argc, char* argv[]) {

Choose a reason for hiding this comment

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

Why is argc a reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I don't know. I'll pass by value instead

@LNDF
Copy link
Contributor

LNDF commented Oct 12, 2024

It might be better to use an arg parsing library like Boost program_options

@qurious-pixel
Copy link
Contributor

linux-sdl appimage works when entering path ./Shadps4-sdl.AppImage /path/to/eboot.bin -f or --fullscreen.
Used to ./Shadps4-sdl.AppImage --fullscreen /path/to/eboot.bin order convention, but this fails with case-sensitive error message Eboot.bin file not found. Whereas removing the switch, it boots into the game.

@robyn-dressler
Copy link
Contributor Author

@LNDF I really would like to use a library for this, but I was hesitant to introduce a new dependency to common, because of this comment in the style documentation:

Don't ever introduce new external dependencies into Core

If that doesn't apply here, then I could rework this to use the Boost library. That would also probably resolve the ordering issue @qurious-pixel mentioned.

@LNDF
Copy link
Contributor

LNDF commented Oct 12, 2024

Boost is already a dependency. You could try to open a PR in https://github.com/shadps4-emu/ext-boost adding boost/program_options.hpp using bcp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants