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

Make the config file generated rather than always existing #130

Open
Hopson97 opened this issue Feb 8, 2020 · 8 comments
Open

Make the config file generated rather than always existing #130

Hopson97 opened this issue Feb 8, 2020 · 8 comments
Assignees
Labels
Feature Suggestions or PRs that add new features Good First Issue Good for newcomers GUI Stuff to do with the GUI systems In Progress

Comments

@Hopson97
Copy link
Owner

Hopson97 commented Feb 8, 2020

Suggestion Title

Config File Generation

Describe your suggestion

Right now the config file (config.obd) always exists.

However, sometimes it is accidentally added in the git commits (by me lol)

It would be better

  1. The config file could be .gitignored
  2. This means the config file would not exist sometimes
  3. This means it would be to be auto-created if it doesn't exist

Implementations ideas [optional]

Some kind of ifstream and maybe a bit of the file system api

@Hopson97 Hopson97 added Feature Suggestions or PRs that add new features Good First Issue Good for newcomers GUI Stuff to do with the GUI systems labels Feb 8, 2020
@MarkusG
Copy link

MarkusG commented Feb 8, 2020

Just to clarify, the current implementation is as follows, failing if the config file doesn't exist?

read in options from file
set option1 = filedata.option1
...
set optionN = filedata.optionN

I think the best way to solve this would be creating the config file and populating it with hard-coded defaults if it doesn't exist already. And, of course, adding it to .gitignore so you stop checking it into git 😛

if (file exists)
    read values from file
    set option1 = filedata.option1
    ...
    set optionN = filedata.optionN
else
    set option1 = defaultvalue1
    ...
    set optionN = defaultvalueN
    write default values to file

@Hopson97
Copy link
Owner Author

Hopson97 commented Feb 8, 2020

Yeah that is pretty much it 😛 It all happens in main.cpp

@MarkusG
Copy link

MarkusG commented Feb 9, 2020

Thoughts on using a standard config file format such as JSON or TOML? The config file seems to already be a bit messy as well as the code to read it, with

auto data = getObdData("config.obd");
auto clientData = data[0].data;
auto serverData = data[1].data;

poorly reflecting the structure of the config file.

The only downside would be another library reference and a rework of the current config logic.

@Hopson97
Copy link
Owner Author

Hopson97 commented Feb 9, 2020

No need, the config file is extremely simple

@Hopson97
Copy link
Owner Author

Hopson97 commented Feb 9, 2020

I can probably update the obd parser so that it reads like

auto clientData = data["CLIENT_DATA"].data;
auto serverData = data["SERVER_DATA"].data;

@MarkusG
Copy link

MarkusG commented Feb 9, 2020

The config file may be simple now, but adopting a standardized markup for it is going to be necessary in the long run. Even now, we're manually parsing some options to integers while leaving others as strings. As the config file gets more options added to it, there are going to be more types to worry about (more work we have to put into a parser), and the structure will get more complex. With a markup language, the config file gets more structure, we gain support for a variety of different types, and we don't have to worry about maintaining our own parser.

Consider the current CLIENT_DATA section

CLIENT_DATA
    cap_fps 0
    fps_limit 60
    fov 90

    fullscreen 0
    window_width 1600
    window_height 900

    skin player
    texture_pack default
    shouldShowInstructions 1

    server_ip LOCAL
end

Intuitively, cap_fps, fullscreen, and shouldShowInstructions, read as integers. Without looking at the source code, it's difficult to understand why these are represented as integers and not booleans in the config file. Also, the end directive is unnecessary if we use an established markup language. Consider an alternative TOML implementation:

[client]
cap_fps = false
fps_limit = 60
fov = 90
...
[server]
world_size = 8

cpptoml seems to be a decent library for interacting with TOML files, and its value_or() function seems like it would be very useful in reducing repetition when generating default values.

@Hopson97
Copy link
Owner Author

Hopson97 commented Feb 9, 2020

True but eventually the config will be able to be configured via menus in the game anyways, so adding a 3600 line dependency seems a bit much 😛

I can probably simplify the parser anyways.

Originally it was going to be used for loading block types and such, hence why I have random support for sections like CLIENT_DATA and such.

It can probably be something like

cap_fps 0
fps_limit 60
fov 90

fullscreen 0
window_width 1600
window_height 900

skin player
texture_pack default
shouldShowInstructions 1

server_ip LOCAL

world_size 8

Thinking about it, Lua could be used as the project has support for this anyways

ClientConfig = {
    cap_fps = false,
    fps_limit = 60,
}
--etc 

@MarkusG
Copy link

MarkusG commented Feb 9, 2020

That's a good point. I forgot that configuration would be handled by a menu in the future. I'll work on cleaning up the config with the current parser.

@MarkusG MarkusG mentioned this issue Feb 10, 2020
@Hopson97 Hopson97 added this to To do in Open Builder Mar 23, 2020
@Hopson97 Hopson97 moved this from To do to In progress in Open Builder Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Suggestions or PRs that add new features Good First Issue Good for newcomers GUI Stuff to do with the GUI systems In Progress
Projects
Open Builder
  
In progress
Development

No branches or pull requests

2 participants