add extension to write to cba_settings.sqf file #1333

commy2 · 2020-05-08T17:24:23Z

When merged this pull request will:

title

It would be super convenient if settings could be written by the game to the settings files directly. So I wanted to have an extension for that. Pardon my poor C++, I have no idea what's going on, lol

READ -> reads setting value & force lvl
WRITE -> overwrites setting with given value & force lvl
CLEAR -> empties cba_settings.sqf
PARSE -> gives a parseSimpleArray'able super array of all settings, value, force lvl triplets. Since extension callback is limited by like 17k chars, this comes in batches of 10k. The idea is to append the 10k batches in SQF before using PSA.

Currently only works for the main game userconfig, not for missions (need to figure out the system path).

dedmen · 2020-05-08T18:39:58Z

extensions/cba_settings/cba_settings.cpp

+    while (file.good()) {
+        getline(file, line);
+        if (file.good() || line.length() != 0) // Don't explode file by appending duplicate additional newlines.
+            result = result + trim(line) + "\n";


Suggested change

result = result + trim(line) + "\n";

result.append(trim(line) + "\n");

dedmen · 2020-05-08T18:40:34Z

extensions/cba_settings/cba_settings.cpp

+}
+
+string load(int strlen) {
+    string result = "";


reserve.

result.reserve(std::filesystem::size(SETTINGS_FILE));

Otherwise you resize/reallocate/copy almost every time you append a line.

Cool, didn't know filestream::size is a thing. Though, is it reasonable to allocate ~10k characters (bytes?) at once?

Well more reasonable than first allocating 16, then deleting and reallocating 32, then deleting and reallocating 64, then deleting and reallocating 128, then deleting and reallocating 512, then deleting and reallocating 2048, then deleting and reallocating 8192, then...

dedmen · 2020-05-08T18:43:44Z

extensions/cba_settings/cba_settings.cpp

+    return "true";
+}
+
+string load(int strlen) {


strlen is the name of a function, please don't use that as variable name.

Haha. Oops.

dedmen · 2020-05-08T18:45:02Z

extensions/cba_settings/cba_settings.cpp

+        line = trim_left(line);
+
+        // Ignore commented out lines.
+        if (line.substr(0, 2) == "//")


This copies the string every time. Would be WAY more efficient to use a string_view here, as that is all that you need. For trim_left too.

Also why trim left and right seperately? why not immediately trim both sides?

why not immediately trim both sides?

force force setting = 123;

is something one could write. If I process this, I need multiple trims, but not on both sides all the time.

dedmen · 2020-05-08T18:47:09Z

extensions/cba_settings/trim.h

+        return str.substr(0, last + 1);
+}
+
+string trim(string str) {


Example for a more efficient trim that doesn't copy the string
https://github.com/dedmen/armake/blob/cpp/src/utils.cpp#L240

dedmen · 2020-05-08T18:48:02Z

extensions/cba_settings/cba_settings.cpp

+    if (argsCnt >= 3)
+        force = min(stoi(args[2]), 2);
+
+    if (_stricmp(function, "version") == 0) {


why even check the whole string?
c
l
p
r
a

the first character would be sufficient for what you need

True I guess, but what does it matter? This is not something that needs to be particularly fast.

Doesn't really matter, just my "every nanosecond counts" bullshit.

I understand.

extensions/cba_settings/cba_settings.cpp

dedmen · 2020-05-08T18:56:12Z

extensions/cba_settings/cba_settings.cpp

+    string result = "false";
+
+    ofstream temp;
+    temp.open(string(TEMP_FILE).c_str(), ios::out);


you are reloading, reparsing, rewriting the whole file for every setting property write?
You could just parse it once, and store it in memory. Your extension won't get unloaded

store it in memory

How?

Comment below, just a global variable. Array or map or some container

dedmen · 2020-05-08T18:57:41Z

extensions/cba_settings/cba_settings.cpp

+
+        // Count force tags.
+        int force = 0;
+        while (line.substr(0, 6) == "force ") {


you are copying the substring on every iteration here. If you use the string_view trim, it will only move a pointer around

dedmen · 2020-05-08T18:59:42Z

I would

struct settingEntry {
uint8_t force;
std::string settingValue;
}

std::unordered_map<std::string, settingEntry> settings;

on parse then clear map and parse entries into it.
on update you just update the entry, and write out all others. No need to reparse file.

write(setValue) is settings[...] = ... + write
append is settings[...] = ... + write
clear is settings.clear() + write
read is return settings[...]
then you have one parse function that parses file and fills the map
and one function that writes the file from map contents that is called by write/append/clear

also then no need for append vs write, as duplicate entries don't make sense anyway?

Since extension callback is limited by like 17k chars,

is it? I thought callbacks were unlimited

commy2 · 2020-05-08T19:28:08Z

is it? I thought callbacks were unlimited

It only gave me that long a string (17 something k characters) when I tried how large a settings file I can get.

Co-authored-by: Dedmen Miller <[email protected]>

commy2 · 2020-05-08T19:35:24Z

I would
unordered_map

But what if I edit the file while the game is running?

dedmen · 2020-05-08T22:39:41Z

But what if I edit the file while the game is running?

store file change date of last parse/write.
Then when reading check file change date, if it was edited you need to reload. If not then not.

TheMagnetar

Just some comments.

TheMagnetar · 2020-05-10T19:54:47Z

extensions/cba_settings/cba_settings.cpp

+strncpy_s(output, outputSize, result.c_str(), _TRUNCATE);\
+return (val)
+
+#define SYNTAX_ERROR_WRONG_PARAMS_SIZE 101


since dedmen is suggesting c++17 features (string_view, filesystem) it is also advisable to use for numerical data the following:

Suggested change

#define SYNTAX_ERROR_WRONG_PARAMS_SIZE 101

static constexpr int32_t SYNTAX_ERROR_WRONG_PARAMS_SIZE = 101;

What's the benefit over

const int SYNTAX_ERROR_WRONG_PARAMS_SIZE = 101;

?

constexpr is evaluated at compile time

which won't make any difference here as the end result.

TheMagnetar · 2020-05-10T19:55:59Z

extensions/cba_settings/cba_settings.cpp

+#define DELIM ":"
+#define MAX_CALLBACK_LENGTH 10000
+
+using namespace std;


This is not recommended in general and it is rather dangerous.

TheMagnetar · 2020-05-10T19:59:44Z

extensions/cba_settings/cba_settings.cpp

+
+    file.close();
+
+    if (strlen * MAX_CALLBACK_LENGTH > result.length())


Not writing { } after if/while etc is not recommended at all. Consider the following

if (strlen * MAX_CALLBACK_LENGTH > result.length()) do_stuf() do_other_stuff

and then one decides to comment do_stuff

if (strlen * MAX_CALLBACK_LENGTH > result.length()) // do_stuf() do_other_stuff

resulting in do_other_stuff being inside the if context breaking a lot of stuff

TheMagnetar · 2020-05-10T20:04:14Z

extensions/cba_settings/cba_settings.cpp

+    return result;
+}
+
+string append(string setting, string value, int force) {


Passing by reference is advisable with classes. It avoids copying the whole class. The same can be applied to the other functions.

Suggested change

string append(string setting, string value, int force) {

string append(const string &setting, const string &value, int force) {

Cool. Didn't know it copies, and that this is how one prevents it.

TheMagnetar · 2020-05-10T20:05:00Z

extensions/cba_settings/cba_settings.cpp

+
+    string line = "";
+    while (force-- > 0)
+        line = line + "force ";


Suggested change

line = line + "force ";

line.append("force ");

add extension to write to cba_settings.sqf file

aabbc55

commy2 added Feature WIP labels May 8, 2020

tabs to spaces

d3c653a

dedmen reviewed May 8, 2020

View reviewed changes

extensions/cba_settings/cba_settings.cpp Outdated Show resolved Hide resolved

dedmen reviewed May 8, 2020

View reviewed changes

Update extensions/cba_settings/cba_settings.cpp

c616412

Co-authored-by: Dedmen Miller <[email protected]>

TheMagnetar reviewed May 10, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add extension to write to cba_settings.sqf file #1333

add extension to write to cba_settings.sqf file #1333

commy2 commented May 8, 2020

dedmen May 8, 2020

dedmen May 8, 2020

commy2 May 8, 2020 •

edited

Loading

dedmen May 8, 2020

dedmen May 8, 2020

commy2 May 8, 2020 •

edited

Loading

dedmen May 8, 2020

dedmen May 8, 2020

commy2 May 8, 2020

dedmen May 8, 2020

dedmen May 8, 2020

commy2 May 8, 2020

dedmen May 12, 2020

commy2 May 12, 2020

dedmen May 8, 2020

commy2 May 8, 2020 •

edited

Loading

dedmen May 8, 2020

dedmen May 8, 2020

dedmen commented May 8, 2020 •

edited

Loading

commy2 commented May 8, 2020

commy2 commented May 8, 2020

dedmen commented May 8, 2020 •

edited

Loading

TheMagnetar left a comment

TheMagnetar May 10, 2020 •

edited

Loading

commy2 May 10, 2020

TheMagnetar May 10, 2020

dedmen May 12, 2020

TheMagnetar May 10, 2020

TheMagnetar May 10, 2020 •

edited

Loading

TheMagnetar May 10, 2020

commy2 May 10, 2020

TheMagnetar May 10, 2020

	result = result + trim(line) + "\n";
	result.append(trim(line) + "\n");

	#define SYNTAX_ERROR_WRONG_PARAMS_SIZE 101
	static constexpr int32_t SYNTAX_ERROR_WRONG_PARAMS_SIZE = 101;


		file.close();

		if (strlen * MAX_CALLBACK_LENGTH > result.length())

	string append(string setting, string value, int force) {
	string append(const string &setting, const string &value, int force) {

add extension to write to cba_settings.sqf file #1333

Are you sure you want to change the base?

add extension to write to cba_settings.sqf file #1333

Conversation

commy2 commented May 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

commy2 May 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

commy2 May 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

commy2 May 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dedmen commented May 8, 2020 • edited Loading

commy2 commented May 8, 2020

commy2 commented May 8, 2020

dedmen commented May 8, 2020 • edited Loading

TheMagnetar left a comment

Choose a reason for hiding this comment

TheMagnetar May 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheMagnetar May 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

commy2 May 8, 2020 •

edited

Loading

commy2 May 8, 2020 •

edited

Loading

commy2 May 8, 2020 •

edited

Loading

dedmen commented May 8, 2020 •

edited

Loading

dedmen commented May 8, 2020 •

edited

Loading

TheMagnetar May 10, 2020 •

edited

Loading

TheMagnetar May 10, 2020 •

edited

Loading