Converts mods.txt to use mods.json #540

narknon · 2024-05-31T01:20:52Z

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

UE4SS · 2024-05-31T10:37:53Z

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.

bitonality · 2024-05-31T13:10:05Z

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.

UE4SS · 2024-05-31T13:29:25Z

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
    }
}

bitonality · 2024-05-31T13:45:06Z

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.

UE4SS · 2024-05-31T13:47:30Z

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.

bitonality · 2024-05-31T13:49:34Z

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)

UE4SS · 2024-05-31T13:51:57Z

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.

bitonality · 2024-05-31T15:01:54Z

Schema Comparison

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.

Json

[
   {
      "mod_name": "CheatManagerEnablerMod",
      "mod_enabled": true
   },
   {
      "mod_name": "ActorDumperMod",
      "mod_enabled": false
   },
]

Toml

"CheatManagerEnablerMod" = true
"ActorDumperMod" = false

Yaml

CheatManagerEnablerMod: yes
ActorDumperMod: no

Xml

<mod name="CheatManagerEnablerMod" isEnabled="true"/>
<mod name="ActorDumperMod" isEnabled="false"/>

Mods.txt

CheatManagerEnablerMod : 1
ActorDumperMod : 0

Napkin Math

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

Graph

narknon · 2024-05-31T15:22:59Z

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.

narknon · 2024-05-31T15:24:28Z

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.

UE4SS · 2024-05-31T15:28:29Z

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.

narknon · 2024-05-31T15:32:09Z

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.

narknon · 2024-05-31T15:35:39Z

Yes, there was support for multiline. https://github.com/UE4SS-RE/RE-UE4SS/blob/main/deps/first/Constructs/include/Constructs/Annotated.hpp

UE4SS · 2024-05-31T15:38:22Z

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.

narknon · 2024-05-31T15:48:55Z

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.

UE4SS · 2024-05-31T16:09:22Z

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.

narknon · 2024-05-31T16:10:30Z

I'm 99% sure there are no third party parsers for it.

UE4SS · 2024-05-31T16:11:33Z

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.

narknon · 2024-05-31T16:14:03Z

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.

UE4SS · 2024-05-31T16:19:36Z

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.

bitonality · 2024-05-31T16:21:22Z

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).

UE4SS · 2024-05-31T16:28:55Z

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.

bitonality · 2024-05-31T16:32:17Z

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.

narknon · 2024-05-31T17:09:27Z

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.

UE4SS · 2024-06-05T15:23:04Z

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.

narknon · 2024-06-05T15:24:22Z

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.

Sure though enabled_mods_file was the original name of mods.txt, not something I changed.

UE4SS · 2024-06-05T15:25:31Z

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.

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.

narknon · 2024-06-05T15:28:25Z

Before I push it, does mod_list_file/legacy work?

UE4SS · 2024-06-05T15:29:15Z

Before I push it, does mod_list_file/legacy work?

Yes, that sounds fine.

UE4SS

Please run clang format.
Note that I haven't yet tested this PR.

UE4SS · 2024-06-05T15:27:06Z

UE4SS/src/UE4SSProgram.cpp

+            std::string descriptive_error = glz::format_error(pe, buffer);
+
+            size_t count = descriptive_error.size() + 1;
+            wchar_t* converted_method_name = new wchar_t[count];
+
+            size_t num_of_char_converted = 0;
+            mbstowcs_s(&num_of_char_converted, converted_method_name, count, descriptive_error.data(), count);
+
+            auto converted = File::StringViewType(converted_method_name);
+
+            delete[] converted_method_name;
+
+            Output::send<LogLevel::Error>(STR("{}\n\nError parsing mods.json file, please fix the file...\n"), converted);


What is this ?
Why aren't we using to_wstring ?

UE4SS · 2024-06-05T15:31:11Z

UE4SS/src/UE4SSProgram.cpp

+    auto UE4SSProgram::convert_legacy_mods_file(StringType legacy_enabled_mods_file, std::vector<ModData>& mod_data_vector) -> void
+    {
+        Output::send(STR("Converting legacy mods.txt to mods.json...\n"));
+        std::wifstream mods_stream = File::open_file_skip_BOM(legacy_enabled_mods_file);


Should use StreamType here instead of std::wifstream.

UE4SS · 2024-06-05T15:33:07Z

UE4SS/include/UE4SSProgram.hpp

+
+            GLZ_LOCAL_META(ModData, mod_name, mod_enabled);
+        };
+        std::vector<ModData> global_mod_data_vector;


Unused variable ?
I can't find any usages in this PR.

UE4SS · 2024-06-05T15:36:39Z

UE4SS/src/UE4SSProgram.cpp

+        } catch (const std::exception& e) {
+            std::cerr << e.what() << std::endl;
+        }
+        std::wstring current_line;


Use StringType.

UE4SS · 2024-06-05T15:36:52Z

UE4SS/src/UE4SSProgram.cpp

+        try {
+            mods_stream = File::open_file_skip_BOM(legacy_mod_list_file);
+        } catch (const std::exception& e) {
+            std::cerr << e.what() << std::endl;
+        }


This is bad, don't throw here, just let the exception fall through to main like we do with all fatal exceptions.
The way you've currently set this up will cause the error to not actually be logged, it will just be ignored and probably crash as you're still continuing to operate on the stream even though there was an error with it causing the stream to be invalid.

UE4SS · 2024-06-05T15:38:55Z

UE4SS/src/UE4SSProgram.cpp

+            std::wstring mod_name = explode_by_occurrence(current_line, L':', 1);
+            std::wstring mod_enabled = explode_by_occurrence(current_line, L':', ExplodeType::FromEnd);


Use StringType, and STR('...') instead of L'...'.

UE4SS · 2024-06-05T15:46:24Z

UE4SS/src/UE4SSProgram.cpp

+
+
+        Output::send(STR("Starting mods (from mods.json load order)...\n"));
+        for (auto it = mod_data_vector.begin(); it != mod_data_vector.end(); ++it)


Any reason why we're using the more complicated iterator method of looping here instead of the simpler
for (const auto& mod_data : mod_data_vector) ?

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.

…r is different from existing json

….txt

narknon mentioned this pull request Jun 5, 2024

[BUG - Release] mods.txt parsing #555

Open

UE4SS requested changes Jun 5, 2024

View reviewed changes

narknon force-pushed the mods.json branch from 54448c8 to 9e6efaf Compare June 10, 2024 19:18

narknon and others added 4 commits June 17, 2024 15:07

Add open_file_skipping_BOM function to RC::File

a0359bf

Make mods.txt parsing skip BOM; make .json only write if moddatavecto…

81a56ce

…r is different from existing json

Rename enabled_mods_file to mod_list_file for clarity against enabled…

60a8402

….txt

narknon force-pushed the mods.json branch from 9e6efaf to 60a8402 Compare June 17, 2024 19:10

UE4SS mentioned this pull request Oct 6, 2024

Add open_file_skipping_BOM function to RC::File #556

Closed

		std::wstring mod_name = explode_by_occurrence(current_line, L':', 1);
		std::wstring mod_enabled = explode_by_occurrence(current_line, L':', ExplodeType::FromEnd);



		Output::send(STR("Starting mods (from mods.json load order)...\n"));
		for (auto it = mod_data_vector.begin(); it != mod_data_vector.end(); ++it)

Converts mods.txt to use mods.json #540

Are you sure you want to change the base?

Converts mods.txt to use mods.json #540

Conversation

narknon commented May 31, 2024 • edited Loading

UE4SS commented May 31, 2024

bitonality commented May 31, 2024

UE4SS commented May 31, 2024

bitonality commented May 31, 2024

UE4SS commented May 31, 2024

bitonality commented May 31, 2024

UE4SS commented May 31, 2024

bitonality commented May 31, 2024

Schema Comparison

Json

Toml

Yaml

Xml

Mods.txt

Napkin Math

Graph

narknon commented May 31, 2024

narknon commented May 31, 2024

UE4SS commented May 31, 2024

narknon commented May 31, 2024

narknon commented May 31, 2024

UE4SS commented May 31, 2024

narknon commented May 31, 2024

UE4SS commented May 31, 2024

narknon commented May 31, 2024

UE4SS commented May 31, 2024

narknon commented May 31, 2024

UE4SS commented May 31, 2024 • edited Loading

bitonality commented May 31, 2024

UE4SS commented May 31, 2024

bitonality commented May 31, 2024 • edited Loading

narknon commented May 31, 2024

UE4SS commented Jun 5, 2024

narknon commented Jun 5, 2024

UE4SS commented Jun 5, 2024

narknon commented Jun 5, 2024

UE4SS commented Jun 5, 2024

UE4SS left a comment

Choose a reason for hiding this comment

UE4SS Jun 5, 2024

Choose a reason for hiding this comment

UE4SS Jun 5, 2024

Choose a reason for hiding this comment

UE4SS Jun 5, 2024

Choose a reason for hiding this comment

UE4SS Jun 5, 2024

Choose a reason for hiding this comment

UE4SS Jun 5, 2024

Choose a reason for hiding this comment

UE4SS Jun 5, 2024

Choose a reason for hiding this comment

UE4SS Jun 5, 2024

Choose a reason for hiding this comment

narknon commented May 31, 2024 •

edited

Loading

UE4SS commented May 31, 2024 •

edited

Loading

bitonality commented May 31, 2024 •

edited

Loading