UnofficialCrusaderPatch / UnofficialCrusaderPatch3

Development for the dll Injection approach
GNU General Public License v3.0
12 stars 2 forks source link

Version 3.0.1 #103

Closed gynt closed 8 months ago

gynt commented 8 months ago

bump RPS to 1.5.0; add USER_CONFIG as global variable; utils.pack and utils.unpack for unpacking bytes to ints etc.

TheRedDaemon commented 8 months ago

So the log message improval is not yet contained? I also have a bit of a hard time understanding the two new utils. What is their Use Case? Also, I think pack could use table.concat instead of concating the string step by step.

TheRedDaemon commented 8 months ago

The handling of the config path seems to only log if it fails, right? Is this the normal behaviour for all CLI settings?

gynt commented 8 months ago

So the log message improval is not yet contained? I also have a bit of a hard time understanding the two new utils. What is their Use Case? Also, I think pack could use table.concat instead of concating the string step by step.

The log message improval is already on main: https://github.com/UnofficialCrusaderPatch/UnofficialCrusaderPatch3/commit/00f585277d576fa2fb1fb643903d2708ff057d9c

Use case of the two new utils is to convert byte streams to basic objects (ints, shorts, bytes) and back:

namespace.pack("<I", {100, 101, 102, 103, 1,2,3,4})
-- returns: "defg☺☻♥♦"

-- string.byte() allows to look up close:
string.byte(namespace.pack("<I", {100, 101, 102, 103, }), 1, -1)
-- returns: 100     0       0       0       101     0       0       0       102     0       0       0       103     0       0       0

-- round trip:
table.unpack(namespace.unpack("<I", namespace.pack("<I", {100, 101, 102, 103, 1,2,3,4})))
-- returns: 100     101     102     103     1       2       3       4

The handling of the config path seems to only log if it fails, right? Is this the normal behaviour for all CLI settings?

There is INFO log if it succeeds: https://github.com/UnofficialCrusaderPatch/UnofficialCrusaderPatch3/blob/bae087688740ee167b31757397fa338211238b28/content/ucp/code/fixes/gamedatapath.lua#L26

TheRedDaemon commented 8 months ago

Alright, missed the other commits. The CLI question was more regarding "There is no error thrown? Does this apply to all CLI options?". This might be ok, but I wanted to question this regardless. The only thing left would be the string concat in the new pack function. Loop concats of arbitrary length are usually better done by collecting the parts in a container and then join them (table.concat in Lua apperantly). (Does not always apply. I remember the export plugin for ghidra in python, where my research suggested that python heavily optimizes certain string concats. Did not find this about Lua, though.)

gynt commented 8 months ago

Using table.concat is a second slower (36 seconds on 1,000,000 namespace.pack()) while string concatenation is 35 seconds. But I agree it is neater and more proper.

gynt commented 8 months ago

Alright, missed the other commits. The CLI question was more regarding "There is no error thrown? Does this apply to all CLI options?". This might be ok, but I wanted to question this regardless. The only thing left would be the string concat in the new pack function. Loop concats of arbitrary length are usually better done by collecting the parts in a container and then join them (table.concat in Lua apperantly). (Does not always apply. I remember the export plugin for ghidra in python, where my research suggested that python heavily optimizes certain string concats. Did not find this about Lua, though.)

Good question! I guess we can also do log(ERROR, ). If a user is going to use this feature, then we better tell them how to do it the right way with errors (rather than warnings in a log) ?

TheRedDaemon commented 8 months ago

Sorry to bother you a bit more, but since you already benchmarked it, can you test it again by using the index (offset?) to set the table values? I am curious. Table insert has, I think, log n complexitiy, since tables used as arrays in lua do not know how many values they have, so insert has to get the length with every call. The last time I researched this, I think I read that they do not lineary count the elements but use one of these "pick an index and check if you are inside or not" algorithms, but nothing beats knowing on which index to place the value.

TheRedDaemon commented 8 months ago

For the config feature, using an error might be better, since this would be something the user wants. (And the GUI should likely use a local storage later, I think.)

gynt commented 8 months ago

Sorry to bother you a bit more, but since you already benchmarked it, can you test it again by using the index (offset?) to set the table values? I am curious. Table insert has, I think, log n complexitiy, since tables used as arrays in lua do not know how many values they have, so insert has to get the length with every call. The last time I researched this, I think I read that they do not lineary count the elements but use one of these "pick an index and check if you are inside or not" algorithms, but nothing beats knowing on which index to place the value.

31 seconds on 1,000,000 calls

TheRedDaemon commented 8 months ago

Ah, nice, and good that you saw the loop issue. I also always forget that in Lua the length is inclusive in the indexes. If I am not wrong, the log for the config path should be changed to error in case the function encounters issues (missing AOBs, folder missing, not a folder; just now saw your answer on Discord). I think it should actually abort the launch. If the user wants to "protect" their other settings, is should not launch. After that, I think the version is good to go.

gynt commented 8 months ago

Done and tested

TheRedDaemon commented 8 months ago

Awesome. I also did not notice that Lua has not exist and check path type function. Then only one question: How does it behave, if the path given points to a file?

gynt commented 8 months ago

Awesome. I also did not notice that Lua has not exist and check path type function. Then only one question: How does it behave, if the path given points to a file?

Oh good point! My brain was like, I will append a / when you call exists(), but I don't think I wrote that in the script actually!

gynt commented 8 months ago

So now I wrote it such that io.exists() will receive a string ending on a /, which will make the std::filesystem::is_regular_file fail, so only the is_directory can determine the result of true/false