Open narknon opened 6 months ago
I can see why mods.txt would be better as a standardized format, but I don't see why UE4SS-settings needs to change from ini to json. If there's no actual benefit here beyond "all our stuff is json", then we're just adding potential difficulties for the end user. I know this PR doesn't modify UE4SS-settings, but when/if this happens it would be useful if whoever does it goes into detail about why the change is needed.
I can see why mods.txt would be better as a standardized format, but I don't see why UE4SS-settings needs to change from ini to json. If there's no actual benefit here beyond "all our stuff is json", then we're just adding potential difficulties for the end user. I know this PR doesn't modify UE4SS-settings, but when/if this happens it would be useful if whoever does it goes into detail about why the change is needed.
JSON schema allows for representing objects and arrays in the file (something ini files struggle with). If the current settings are only kvps then I see your point.
Have we considered if we should be making the settings file more than key value pairs? Some of the beauty of the ini format (as a settings mechanism) is that it's pretty obvious when it's being misused for things it shouldn't be.
Another thing to consider is that we lose the ability to ship comments/descriptions about settings within the settings file if we switch to json. Ini, toml, yaml, all have the ability to comment within the file but that's one of the key "weaknesses" of json as a configuration schema. (It's a strength for serialization/requests depending on who you ask since it makes the schema easier to regulate when using json as request payloads/http responses over the wire...)
TOML and yaml are both alternatives that support comments but have their own histories/quirks.
Just making sure that we're thinking about what the right tool for the right job is.
I can see why mods.txt would be better as a standardized format, but I don't see why UE4SS-settings needs to change from ini to json. If there's no actual benefit here beyond "all our stuff is json", then we're just adding potential difficulties for the end user. I know this PR doesn't modify UE4SS-settings, but when/if this happens it would be useful if whoever does it goes into detail about why the change is needed.
JSON schema allows for representing objects and arrays in the file (something ini files struggle with). If the current settings are only kvps then I see your point.
Have we considered if we should be making the settings file more than key value pairs? Some of the beauty of the ini format (as a settings mechanism) is that it's pretty obvious when it's being misused for things it shouldn't be.
Another thing to consider is that we lose the ability to ship comments/descriptions about settings within the settings file if we switch to json. Ini, toml, yaml, all have the ability to comment within the file but that's one of the key "weaknesses" of json as a configuration schema. (It's a strength for serialization/requests depending on who you ask since it makes the schema easier to regulate when using json as request payloads/http responses over the wire...)
TOML and yaml are both alternatives that support comments but have their own histories/quirks.
Just making sure that we're thinking about what the right tool for the right job is.
We can "fake" comments in JSON by having them as a field so that shouldn't be a huge problem, and I do think comments are crucial for the settings file. I like ini because it's very simple and easy to read. With JSON, you get more advanced features, but it's harder to read, there's substantial visual cluttering. While we do have some basic support for arrays (section as ordered list & section entries without values) in our ini system, that's not part of any kind of ini standard so it would potentially make it harder for anyone to programmatically parse the file, though I don't believe we use any of those features in UE4SS-settings.ini.
My opinion is that we should stay away from JSON or any other format for the settings file unless we desperately need more advanced features, and we should also take into account whether the setting we're implementing that requires more advanced features actually belongs in the settings file.
Example of how a basic ini structure would look like, and the equivalent in JSON:
[Section1]
; Description of A
A = 1
; Description of B
B = 2
; Description of C
C = 3
[Section2]
; Description of A
A = 1
; Description of B
B = 2
; Description of C
C = 3
{
"Section1": {
"Description of A": null,
"A": 1,
"Description of B": null,
"B": 2,
"Descriptio of C": null,
"C": 3
},
"Section2": {
"Description of A": null,
"A": 1,
"Description of B": null,
"B": 2,
"Descriptio of C": null,
"C": 3
}
}
I'd personally avoid doing the json comment by using the adding a new key hack. Whenever I see that mechanism being used, it screams "I'm misusing the intent of this schema". Json was designed as and still is a schema for data-interchange.
I'd personally avoid doing the json comment by using the adding a new key hack. Whenever I see that mechanism being used, it screams "I'm misusing the intent of this schema". Json was designed as and still is a schema for data-interchange.
Wouldn't that interfere with programs parsing the file with the assumption that it's standard json ? That's a concern because people do actually parse the UE4SS-settings file.
I'd personally avoid doing the json comment by using the adding a new key hack. Whenever I see that mechanism being used, it screams "I'm misusing the intent of this schema". Json was designed as and still is a schema for data-interchange.
Wouldn't that interfere with programs parsing the file with the assumption that it's standard json ? That's a concern because people do actually parse the UE4SS-settings file.
Let me clarify - i strongly advise against hacking in comment functionality into json through any means (new kvp, preprocessor, etc)
I'd personally avoid doing the json comment by using the adding a new key hack. Whenever I see that mechanism being used, it screams "I'm misusing the intent of this schema". Json was designed as and still is a schema for data-interchange.
Wouldn't that interfere with programs parsing the file with the assumption that it's standard json ? That's a concern because people do actually parse the UE4SS-settings file.
Let me clarify - i strongly advise against hacking in comment functionality into json through any means (new kvp, preprocessor, etc)
Oh okay, I must've misunderstood, good to have clarification. I generally agree.
Don't use this chart as the source of truth in terms of deciding what schema to use. I just wanted to provide some data points about file size bloat for each schema type in the context of mods.txt
. I want to ensure that we're looking to the future and taking scale into account so we can make informed decisions about the strengths/limitations of whatever schema we end up using.
[
{
"mod_name": "CheatManagerEnablerMod",
"mod_enabled": true
},
{
"mod_name": "ActorDumperMod",
"mod_enabled": false
},
]
"CheatManagerEnablerMod" = true
"ActorDumperMod" = false
CheatManagerEnablerMod: yes
ActorDumperMod: no
<mod name="CheatManagerEnablerMod" isEnabled="true"/>
<mod name="ActorDumperMod" isEnabled="false"/>
CheatManagerEnablerMod : 1
ActorDumperMod : 0
Format | Up-front bytes | Per-Entry Bytes | Comment Bytes |
---|---|---|---|
JSON | 4 | 59-60 + N | 0 |
TOML | 0 | 10-11 + N | 1+C |
YML | 0 | 6 + N | 1+C |
XML | 0 | 32-33 + N | 7 + C |
TXT | 0 | 5 + N | 3 + C |
I don't think we have to worry about file bloat for mods.txt/json. It should only ever be for enable/disable and any additional metadata would go elsewhere. Toml may be "ideal" for very simple files and yaml for complicated ones, but that is two separate libs that we'd need to pull in and that anyone interacting with our files would need to pull in, when json is a good middle ground for both.
Wrong place for general json settings discussion, but we iterated over several methods of setting up the settings file in json a long time ago, and made helpers to make commenting easy.
Our comment templates:
There is also jsonc:
JSON is originally intended to be a data interchange format, sure, but the original intention is not relevant. In my opinion (and many others' opinions) it is very readable as a config format, and our library has powerful syntax error logging.
Also of note for the settings file is that we will likely have a GUI for the settings at some point, but even if we do not have a separate program, the debugging GUI will have a tab. The only reason it doesn't already is because the current format would make it much more annoying to write changes out.
Our comment templates:
Does this support multi-line comments ? We currently have multi-line comments in the ini file and horizontal scrolling is terrible so I think multi-line comments is something we should support if possible.
Does it support a field for the default value ? Could be part of a multi-line comment. The default value provides an easy way for the user to experiment with values without having to pull up the file on github whenever they want to revert to the default. I consider it valuable information.
I believe CC made functionality for multiline, yes. This is another thing that got stuck in decision making limbo due to arguments, so it was long enough ago that I don't fully remember.
Yes, there was support for multiline. https://github.com/UE4SS-RE/RE-UE4SS/blob/main/deps/first/Constructs/include/Constructs/Annotated.hpp
I think runtime modifying of the settings is a good enough reason to change from our custom ini parser since that doesn't support modifying the file, but that brings into question backwards compatibility. We could implement it similarly to the file location changes, but we'd have to disable runtime modifying if the ini file is being used instead of the new one. But regardless, this isn't covered in this PR so unless you intend to expand it, I think we can move on from this discussion, and bring it back up when someone wants to implement changes to the settings file, or alternatively move this to an issue or discussion (with reference to here) if you think more discussion is needed.
Last time I implemented this I had it read out the old file and convert it to json, similar to what I do in this PR. I believe the in progress branch still exists as JSONSettings, but I can't recall what the context of the last push to it was.
But yeah, this can be made into an issue or separate discussion.
Last time I implemented this I had it read out the old file and convert it to json, similar to what I do in this PR. I believe the in progress branch still exists as JSONSettings, but I can't recall what the context of the last push to it was.
The problem with converting it is that it doesn't address the problem of third-party parsers expecting an ini file with the ini format.
I'm 99% sure there are no third party parsers for it.
I'm 99% sure there are no third party parsers for it.
We can't know for sure if there are, but it's generally a bad idea to break compatibility, and there is a better solution so we should use it.
I haven't even found a third party mod manager that actually uses mods.txt rather than enabled.txt, let alone the settings. I'd literally eat my shoe if there is one. But this PR favors the old mods.txt when updating the new one, though I am considering changing that now that I have done more research into whether any mod managers use mods.txt. A similar thing could happen here. Best practices shouldn't be used to the point of creating problems where there are none.
I haven't even found a third party mod manager that actually uses mods.txt rather than enabled.txt, let alone the settings. I'd literally eat my shoe if there is one. But this PR favors the old mods.txt when updating the new one, though I am considering changing that now that I have done more research into whether any mod managers use mods.txt. A similar thing could happen here. Best practices shouldn't be used to the point of creating problems where there are none.
I have definitely seen mod managers use mods.txt, but I can't recall which one unfortunately.
The conversion could be problematic, even though I do think it's backwards compatible.
There will be confusion from users that end up having both mods.txt and mods.json. They won't know which one to edit or the repercussions of modifying the json file. The json file will be overwritten with the contents of the txt file so any changes to the json file will be discarded, and this is harmful to the user.
It is backwards compatible assuming that it always converts and thus fully discards the current json to avoid appending the data multiple times in it, but it has the problem mentioned above.
EDIT: This is all assuming that glaze isn't smart about it and just overwrites the entire file instead of setting each value one by one leaving existing (and unknown) values unchanged.
I don't think we have to worry about file bloat for mods.txt/json.
I agree that it's negligible in this case. I just wanted to make sure that we understand that we're increasing the size of that file by ~10x and that we're okay with that (which it looks like we are unless anyone wants to object).
I don't think we have to worry about file bloat for mods.txt/json.
I agree that it's negligible in this case. I just wanted to make sure that we understand that we're increasing the size of that file by ~10x and that we're okay with that (which it looks like we are unless anyone wants to object).
I mean worst case (and unlikely), someone will run a couple hundred mods. How long would it take for glaze to parse a file that big ? Surely not long enough to be a problem ? It wouldn't be that big of a file. Is that the main concern ? If so, it's not a big problem even if it does end up taking a huge amount of time like a couple of seconds extra to load the game, as long as it doesn't go any further than that.
I don't think we have to worry about file bloat for mods.txt/json.
I agree that it's negligible in this case. I just wanted to make sure that we understand that we're increasing the size of that file by ~10x and that we're okay with that (which it looks like we are unless anyone wants to object).
I mean worst case (and unlikely), someone will run a couple hundred mods. How long would it take for glaze to parse a file that big ? Surely not long enough to be a problem ? It wouldn't be that big of a file. Is that the main concern ? If so, it's not a big problem even if it does end up taking a huge amount of time like a couple of seconds extra to load the game, as long as it doesn't go any further than that.
I agree that it won't be a problem for a sane amount of mods. I just wanted to make sure we are planning on getting benefits from switching schemas in order to offset the increase in file size. From what I've read on this discussion: readability, parsing libs, other mentioned benefits all outweigh the negligible size increase.
I just wanted to make sure we are getting tangible benefits and not just increasing the file size without any upsides in return. Apologies if it came off as being pedantic about size alone.
There will be confusion from users that end up having both mods.txt and mods.json. They won't know which one to edit or the repercussions of modifying the json file. The json file will be overwritten with the contents of the txt file so any changes to the json file will be discarded, and this is harmful to the user.
It is backwards compatible assuming that it always converts and thus fully discards the current json to avoid appending the data multiple times in it, but it has the problem mentioned above.
EDIT: This is all assuming that glaze isn't smart about it and just overwrites the entire file instead of setting each value one by one leaving existing (and unknown) values unchanged.
I suppose we could do favor ini/txt if not default settings, if ini default but json isn't favor json for each setting.. At this point I'm giving up on moving the convo for the ue4ss-settings from this PR since we're this deep into it.
In my opinion, the full backwards compat favoring settings.ini/mods.txt should last at most one minor release cycle post 4.0, or a couple months, whichever is longer. After that, the converter should be left in until 5.0, but the legacy files shouldn't be read if the json exists. It's a bit of a bandaid rip but with our release cycle I think that gives ample time for any mod managers that would ever make the switch to make the switch.
Glz will always read/write from the struct, so it's up to us how we want to populate the data in the struct from the existing ini/txt or jsons and which gets favored.
Can you rename legacy_enabled_mods_file
, enabled_mods_file
, and any other named references to mods.txt/json to something that doesn't clash with our concept of enabled.txt
?
I got confused by this naming at first when looking at your code.
Can you rename
legacy_enabled_mods_file
,enabled_mods_file
, and any other named references to mods.txt/json to something that doesn't clash with our concept ofenabled.txt
? I got confused by this naming at first when looking at your code.
Sure though enabled_mods_file was the original name of mods.txt, not something I changed.
Can you rename
legacy_enabled_mods_file
,enabled_mods_file
, and any other named references to mods.txt/json to something that doesn't clash with our concept ofenabled.txt
? I got confused by this naming at first when looking at your code.Sure though enabled_mods_file was the original name of mods.txt, not something I changed.
Understood. This was introduced before enabled.txt so that's why it was never changed but since we're making changes to the code now, we might as well make it less confusing.
Before I push it, does mod_list_file/legacy work?
Before I push it, does mod_list_file/legacy work?
Yes, that sounds fine.
Maintains legacy support for mods.txt and favors mods.txt for settings. mods.txt should only be favored for one minor release cycle post 4.0. Our packaging script will need to be updated, but after mods.txt and ue4sssettings are converted to json it should be able to be simplified significantly.
Requires #556