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

Add Game_Config + CmdlineParser + video options - Config 2 of N #2108

Merged
merged 14 commits into from Aug 16, 2020

Conversation

fmatthew5876
Copy link
Contributor

@fmatthew5876 fmatthew5876 commented Apr 4, 2020

The PR provides the following

  • A set of objects for manipulating configuration parameters, including the ability to disable them or lock them to specific values for platforms where it makes sense to do so.
  • A Game_Config class for storing EasyRPG configuration
  • Beginning support for config file parsing video options via --config -c
  • A new CmdlineParser class for command line arguments

@fmatthew5876 fmatthew5876 changed the title Add Game_Config class and use it for video options Add Game_Config class and use it for video options - Config 2 of N Apr 4, 2020
@fmatthew5876 fmatthew5876 added Has PR Dependencies This PR depends on another PR Video labels Apr 5, 2020
@fmatthew5876 fmatthew5876 force-pushed the game_config branch 2 times, most recently from 09b046d to b68b592 Compare May 5, 2020 19:38
@fmatthew5876 fmatthew5876 removed the Has PR Dependencies This PR depends on another PR label May 23, 2020
@fmatthew5876 fmatthew5876 force-pushed the game_config branch 2 times, most recently from 234e6ba to 88874ca Compare May 23, 2020 07:03
@fmatthew5876 fmatthew5876 marked this pull request as ready for review May 23, 2020 07:53
@fmatthew5876 fmatthew5876 changed the title Add Game_Config class and use it for video options - Config 2 of N Add Game_Config + CmdlineParser + video options - Config 2 of N May 23, 2020
@fmatthew5876
Copy link
Contributor Author

I need to do another round of testing, but this is ready for review now.

@carstene1ns
Copy link
Member

Question: Why do we need the new command line parser? Seems like inventing the wheel when there are ready to use alternatives, e.g. CLI11, Lyra, cxxopts.

@fmatthew5876
Copy link
Contributor Author

We need a command line parser so it's easier to specify options and try to minimize the amount of work for exposing a config parameter between commandline / config file / scene settings.

Whether we use a third party one or write our own doesn't much matter to me. Command line parsing is a simple exercise. Writing one seemed like less work than evaluating all these other options and then getting them to compile on all our platforms.

This one has an unusual interface in that it parses and advances to the next in the same call. So it doesn't fit a traditional loop. This however is the most efficient way to do it and doesn't do any unnecessary extra parsing.

Whatever we choose. It needs to support unicode and case (in)sensitivity the way we use them. It also needs to be able to support the legacy RPG_RT command line options seamlessly combined with more modern ones.

@Ghabry Ghabry added this to the 0.6.3 milestone Jun 29, 2020
Copy link
Member

@Ghabry Ghabry left a comment

Choose a reason for hiding this comment

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

Can you rebase this? Not all PR builders work right now.
For cmdline_parser.h and config_parser.h the indent is still off: We don't indent public:/private:. They must move 1 to the left together with the rest.
loadFromConfig and loadFromArgs need a capital L.

The parser seems fairly solid and this entire PR is a good step in the correct direction. The flexibility is needed because we must support the legacy command line arguments. The configuration system also looks well designed (compared to that abomination in libretro ;)).

Because of its cmd+configfile handling I even consider this as a usable replacement for boost_programoptions.

src/game_config.cpp Outdated Show resolved Hide resolved
src/config_param.h Outdated Show resolved Hide resolved
src/game_config.cpp Outdated Show resolved Hide resolved
@fdelapena fdelapena merged commit e1f1472 into EasyRPG:master Aug 16, 2020
@fdelapena fdelapena added Settings All about customizable features, storing and applying them Enhancement labels Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Settings All about customizable features, storing and applying them Video
Development

Successfully merging this pull request may close these issues.

None yet

4 participants