-
Notifications
You must be signed in to change notification settings - Fork 112
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
Saving and loading game settings from a config #639
base: release
Are you sure you want to change the base?
Conversation
In the previous version, the Playback Delay was being set to zero after the start of a game session
created a separate fieldType for json string that needs to be mapped to an Enum
Config loader now validates integers in the config to ensure they fall within specified min and max boundaries
The value of rogue.playbackDelayPerTurn is preserved from the purging of 'rogue' fields in the initializeRogue functions. That allows to store playbackDelayPerTurn value loaded from a config directly in 'rogue' global variable instead of creating a separate one.
description of changes
This looks like a good start to me. I briefly tested all the settings this adds; they all seem to work. Error handling seems to work too; if a field is set to an invalid type or value, it resets that field to the default. However, if you make a syntax error while modifying the file manually, it resets the entire file to the default settings. For now that's fine, but it would be a problem if we ever add settings that aren't modifiable in-game. There's at least two possible solutions: when there's a syntax error, show an error and abort instead of overwriting the file, or alternatively make sure all settings that we add in the future are modifiable in-game (maybe with a new settings menu). I haven't taken a close look at the code yet so I may have more comments later. |
return NULL; | ||
} | ||
|
||
fread(buffer, 1, fileSize, jsonFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Should probably check the number of bytes written by fread
, and return an error if it's less than fileSize
switch (entries[i].type) { | ||
case INT_TYPE: | ||
if (cJSON_IsNumber(jsonField)) { | ||
short value = jsonField->valueint; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to check for overflow/underflow, since jsonField->valueint
is an int
(typically 32 bits) but value
is a short
(typically 16 bits).
|
||
case BOOLEAN_TYPE: | ||
if (cJSON_IsBool(jsonField)) { | ||
*((boolean*)entries[i].paramPointer) = jsonField->valueint; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading the documentation for cJson correctly, I believe you need to check cJSON_IsTrue(...)
instead of (...)->valueint
, which doesn't look to be set for booleans.
short mode = mapStringToEnum(modeString, entries[i].validator.enumMappings); | ||
|
||
if (mode != -1) { | ||
*((short*)entries[i].paramPointer) = mode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if all of the enum types are definitely short-sized. It would be nice to have some better type-safety here, maybe a constructor function for enum struct field definitions that explicitly only accepts short*
?
boolean displayStealthRangeMode; | ||
boolean trueColorMode; | ||
boolean wizard; | ||
boolean easyMode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional suggested naming:
stealthRangeEnabled
colorEffectsEnabled
wizardModeEnabled
easyModeEnabled
fieldValidator validator; | ||
} configField; | ||
|
||
static const char* JSON_FILENAME = "brogue_config.json"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional. How about brogue.config ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. I like it. I'm looking forward to the stealth range setting being sticky.
This pull request introduces a functionality for saving and loading the game settings from a JSON config. The feature ensures that settings will be persistent between game launches. This PR is related to the issue #384.
General description
The config handling is done within the new module 'Config.c'. The order of operations with config as follows:
Implemented settings
Additional thoughts
While working on the PR, I faced an issue with storing the replay speed value in the 'rogue' global variable. It appeared that after starting a new game its value (rogue.playbackDelaySpeed) was being set to NULL. Eventually I discovered that every 'rogue' field is set to NULL during the game initialization (function 'initializeRogue' in RogueMain.c), and I had to manually prevent a purge of a value I was interested in. It worked, but I suspect that this behavior can confuse future developers (it certainly took me some time to figure it out). Perhaps this part of code can benefit from refactoring; for example, values that need to be persistent across games could be separated into a different struct instead of being manually saved from purging.