OpenXcom Forum
OpenXcom Forks => OpenXcom Extended (OXCE) => Topic started by: Delian on October 21, 2024, 09:41:19 pm
-
Everyone knows that saving and loading games in OXC and OXCE gets progressively slow as the game goes on. Well, it's not much of a problem in OXC, because vanilla games aren't large and don't last that long, so the issue doesn't become super noticeable. Since this isn't an OXC issue, it doesn't make much sense to fix it there.
Anyway, the main culprit is the yaml-cpp library that the game uses. The simplest solution is to ditch that library and use a faster one. So, I've decided to take on a monumental task of converting OXCE from yaml-cpp to rapidyaml.
I'll use this thread to document some of the observations and pitfalls I've encountered during the coding.
-
Let's start with a simple parsing speed demo of a 5.8MB .rul file 4 times in debug mode
With yaml-cpp:
auto start = high_resolution_clock::now();
for (size_t i = 0; i < 4; i++)
{
auto stream = CrossPlatform::readFile("C:\\Dioxine_XPiratez\\user\\mods\\Piratez\\Ruleset\\Piratez.rul");
YAML::Node doc = YAML::Load(*stream);
}
auto stop = high_resolution_clock::now();
auto duration = duration_cast<microseconds>(stop - start);
duration: 216.2 seconds (216,277,714 microseconds)
With rapidyaml:
auto start = high_resolution_clock::now();
for (size_t i = 0; i < 4; i++)
{
size_t size;
char* data = CrossPlatform::ReadFileRaw("C:\\Dioxine_XPiratez\\user\\mods\\Piratez\\Ruleset\\Piratez.rul", &size);
ryml::Tree tree = ryml::parse_in_arena(ryml::csubstr(data, size)); //copy data to internal data buffer (arena) and parse it
free(data);
}
auto stop = high_resolution_clock::now();
auto duration = duration_cast<microseconds>(stop - start);
duration: 3.2 seconds (3,246,784 microseconds)
67 times faster, not bad.
-
I would like to speed up load & save but yaml-cpp is bit entrenched in OXC.
Loading some rules sets in theory would be easy to swap to different "provider" but there some classes
that have custom converters. Another is that rule sets some times copy yaml-cpp nodes and keep them longer
after rule loading. It could be required lot of work to keep current sematic of yaml-cpp when using faster parser.
Another question, do `ryml` load all nodes? or do it lazy? only when you request data from it?
yaml-cpp is eager in converting file into nodes in memory.
-
I would like to speed up load & save but yaml-cpp is bit entrenched in OXC.
Loading some rules sets in theory would be easy to swap to different "provider" but there some classes
that have custom converters. Another is that rule sets some times copy yaml-cpp nodes and keep them longer
after rule loading. It could be required lot of work to keep current sematic of yaml-cpp when using faster parser.
Another question, do `ryml` load all nodes? or do it lazy? only when you request data from it?
yaml-cpp is eager in converting file into nodes in memory.
>>keeping the same semantics
I've completely abandoned the thought of keeping the yaml-cpp semantics, I'm just going to replace all the code that interacts with yaml objects. There's just too many differences in the APIs. Most yaml-cpp function return a Node object, but most rapidyaml do not, and instead require passing objects as pointers through function calls.
>>custom converters
For converters, here's an example:
Before:
template<>
struct convert<OpenXcom::UnitStats>
{
static bool decode(const Node& node, OpenXcom::UnitStats& rhs)
{
if (!node.IsMap())
return false;
rhs.tu = node["tu"].as<int>(rhs.tu);
rhs.stamina = node["stamina"].as<int>(rhs.stamina);
rhs.health = node["health"].as<int>(rhs.health);
rhs.bravery = node["bravery"].as<int>(rhs.bravery);
rhs.reactions = node["reactions"].as<int>(rhs.reactions);
rhs.firing = node["firing"].as<int>(rhs.firing);
rhs.throwing = node["throwing"].as<int>(rhs.throwing);
rhs.strength = node["strength"].as<int>(rhs.strength);
rhs.psiStrength = node["psiStrength"].as<int>(rhs.psiStrength);
rhs.psiSkill = node["psiSkill"].as<int>(rhs.psiSkill);
rhs.melee = node["melee"].as<int>(rhs.melee);
rhs.mana = node["mana"].as<int>(rhs.mana);
return true;
}
static Node encode(const OpenXcom::UnitStats& rhs)
{
Node node;
node["tu"] = rhs.tu;
node["stamina"] = rhs.stamina;
node["health"] = rhs.health;
node["bravery"] = rhs.bravery;
node["reactions"] = rhs.reactions;
node["firing"] = rhs.firing;
node["throwing"] = rhs.throwing;
node["strength"] = rhs.strength;
node["psiStrength"] = rhs.psiStrength;
node["psiSkill"] = rhs.psiSkill;
node["melee"] = rhs.melee;
node["mana"] = rhs.mana;
return node;
}
};
After:
//overload for parsing UnitStats
bool read(ryml::ConstNodeRef const& n, UnitStats* val)
{
// because the order is always the same, we can safely use next_subling, which is fast
auto node = n.first_child(); node >> val->tu;
node = node.next_sibling(); node >> val->stamina;
node = node.next_sibling(); node >> val->health;
node = node.next_sibling(); node >> val->bravery;
node = node.next_sibling(); node >> val->reactions;
node = node.next_sibling(); node >> val->firing;
node = node.next_sibling(); node >> val->throwing;
node = node.next_sibling(); node >> val->strength;
node = node.next_sibling(); node >> val->psiStrength;
node = node.next_sibling(); node >> val->psiSkill;
node = node.next_sibling(); node >> val->melee;
node = node.next_sibling(); node >> val->mana;
return true;
}
void write(ryml::NodeRef* n, UnitStats const& val)
{
*n |= ryml::MAP;
n->append_child() << ryml::key("tu") << val.tu;
n->append_child() << ryml::key("stamina") << val.stamina;
n->append_child() << ryml::key("health") << val.health;
n->append_child() << ryml::key("bravery") << val.bravery;
n->append_child() << ryml::key("reactions") << val.reactions;
n->append_child() << ryml::key("firing") << val.firing;
n->append_child() << ryml::key("throwing") << val.throwing;
n->append_child() << ryml::key("strength") << val.strength;
n->append_child() << ryml::key("psiStrength") << val.psiStrength;
n->append_child() << ryml::key("psiSkill") << val.psiSkill;
n->append_child() << ryml::key("melee") << val.melee;
n->append_child() << ryml::key("mana") << val.mana;
}
>>copy yaml-cpp nodes and keep them longer
That's a problem, because nodes in rapidyaml are just pointers to tree data. You can't copy them or move them around because they don't hold any data. If you tried to copy anything, you'd have to copy all the data - the whole tree hehe. Don't worry, if it becomes a problem I'll fix it!
>do `ryml` load all nodes
The file load functions that we use already load all the data from hard drive into memory, the whole file, so the streams that we use aren't lazy-loaded. If you mean parsing the said string data, it's lazy-loaded. Converting from string to int only happens on demand, like in the above example, with the >> operator.
Btw, rapidyaml uses two different types of objects, depending if you want to just read, or if you want to write. For reading, it's the read-only node called ryml::ConstNodeRef, but for writing it's ryml::NodeRef. You can't convert them from one to another, so it makes spotting errors a lot easier. However, it's incompatible with yaml-cpp, because yaml-cpp doesn't distinct between read-only and read-write nodes.
Hmm, anyway.
If I want to add a wrapper a library, where to put it? How to name it? For now, instead of #including rapidyaml directly, I'm including #rapidyamlwrapper.h (which I put in /src) so that I can put some defines and pragmas in there, but I wonder what's the normal practice...
In the CrossPlatform.cpp I had to move most of the code from readFile to a new readFileRaw function. That's because I don't want an istream to the yaml data, I just want the data, and it's slow converting from stream back to data. So readFile calls readFileRaw and wraps it into stream. But for parsing yaml I'll just be calling readFileRaw directly.
-
Some common differences between yaml-cpp and rapidyaml:
yaml-cpp:
YAML::Node childNode = node["key"];
if (childNode) {
doStuff(childNode);
}
rapidyaml:
// ryml::ConstNodeRef childNode = node["key"]; //crashes if key doesn't exist!
ryml::ConstNodeRef childNode = node.find_child("key"); //doesn't crash if node doesn't exist, instead returns an invalid node
if (!childNode.invalid()) { //you also can't do "if (node)"
doStuff(childNode);
}
or
if (node.has_child("key")) { //this is a slower option because you search for the key twice, and searching children keys is slow in large objects - O(n)
doStuff(node["key"]);
}
-
converting
rhs.tu = node["tu"].as<int>(rhs.tu);
into
node = node.next_sibling(); node >> val->stamina;
is nonstarter. Nowhere we guarantee order or existence of nodes.
Many save objects save optional values only if they do not have default values.
Even if current code work fine, you need consider different versions (past and future).
If we make loading too fast then changing format become hard as is in binary files.
For code, "shape" its irreverent, I many times use helpers like `mod->load*` to override yaml-cpp behavior.
If you could replace all with `load(node, "name", value);` I would be for it.
Best case could be your pull request: https://github.com/MeridianOXC/OpenXcom/pull/132
instead of using `to_string` each time you want avoid slow yaml-cpp load we should add dedicated loan & save functions
that use most optimal code (like `_vec = node["list"].as<std::vector<int>>(_vec)` copy all data each time is called, I try avoid code like this)
This will improve current loading speed and allow future changes.
One place where I did things like this, was `LoadYaml.h` and `Mod.h`. Probably better is first case as more general.
I think probably best way I would see to start all this is add new layer of indirection.
Create `OpenXcom::Yaml` and it will now store `YAML::Node` but then it could be easy `#ifdef` to store `ryml::NodeRef` or `ryml::ConstNodeRef`
(we could make dedicated objects for save and load to better match ryml behavior) .
Btw there are other yaml libs, could have less performance as rapidyaml but could have closer semantic to yaml and easier to swich
-
Some common differences between yaml-cpp and rapidyaml:
yaml-cpp:
YAML::Node childNode = node["key"];
if (childNode) {
doStuff(childNode);
}
rapidyaml:
// ryml::ConstNodeRef childNode = node["key"]; //crashes if key doesn't exist!
ryml::ConstNodeRef childNode = node.find_child("key"); //doesn't crash if node doesn't exist, instead returns an invalid node
if (!childNode.invalid()) { //you also can't do "if (node)" because it's a struct, not a class
doStuff(childNode);
}
or
if (node.has_child("key")) { //this is a slower option because you search for the key twice, and searching children keys is slow in large objects - O(n)
doStuff(node["key"]);
}
I think lot better would be wrapper `OpenXcom::Yaml`.
In case of rapid it could store first, last and current nodes. each time you check for `"X"` it check if its current node, if not it loop `next_sibling()` to next one and check if its same, this loop until it reach end, when its reach end, it start from beginning unit reach previous current node.
This reduce cost to one `O(n)` and in "best" case it could have cost `O(1)` if loading order is same as saving.
For rulesets we could make "random" version that allocate flat buffer that sort all nodes based on it.
-
rapidyaml has difficulties with Enums.
You can do
node << (int)myEnumVar;
but you can't do:
node >> (int)myEnumVar;
Or maybe I'm stupid and I just don't know how lol... only thing I figured out is:
int temp;
node >> temp;
myEnumVar = (MyEnum)temp;
So for now I'm just specifying a custom read overload:
bool read(ryml::ConstNodeRef const& n, MyEnum* val){ int temp; n >> temp; *val = (MyEnum)temp; return true; }
and then "node >> myEnumVar"; works fine
rapidyaml already has a read function defined for all integral types, also for vector and map, so parsing most types is easy. But for custom types like enums, you gotta specify your own read(), and maybe write overload.
>next_sibling() is nonstarter
Sorry, this was one of the exceptional places where I used next_sibling(), I don't normally use it. Forget you saw that! I'll change it to use keys instead.
>>Many save objects save optional values only if they do not have default values.
Yeah, don't worry about it. I'm careful, which is why converting to rapidyaml is taking me so long. I've already been working on it for two weeks!
BattleUnit::load()
Before:
void BattleUnit::load(const YAML::Node &node, const Mod *mod, const ScriptGlobal *shared)
{
_id = node["id"].as<int>(_id);
_faction = (UnitFaction)node["faction"].as<int>(_faction);
_status = (UnitStatus)node["status"].as<int>(_status);
_wantsToSurrender = node["wantsToSurrender"].as<bool>(_wantsToSurrender);
_isSurrendering = node["isSurrendering"].as<bool>(_isSurrendering);
_pos = node["position"].as<Position>(_pos);
_direction = _toDirection = node["direction"].as<int>(_direction);
_directionTurret = _toDirectionTurret = node["directionTurret"].as<int>(_directionTurret);
_tu = node["tu"].as<int>(_tu);
_health = node["health"].as<int>(_health);
_mana = node["mana"].as<int>(_mana);
...
After:
void BattleUnit::load(const ryml::ConstNodeRef &node, const Mod *mod, const ScriptGlobal *shared)
{
const ryml::Tree tree = *node.tree();
std::unordered_map<ryml::csubstr, ryml::id_type> index(node.num_children()); //build and use an index to avoid [] operator's O(n) complexity
for (const ryml::ConstNodeRef& childNode : node.children())
index[childNode.key()] = childNode.id();
tree.cref(index["id"]) >> _id;
tree.cref(index["faction"]) >> _faction;
tree.cref(index["status"]) >> _status;
if (index.count("wantsToSurrender"))
tree.cref(index["wantsToSurrender"]) >> _wantsToSurrender;
if (index.count("isSurrendering"))
tree.cref(index["isSurrendering"]) >> _isSurrendering;
tree.cref(index["position"]) >> _pos;
tree.cref(index["direction"]) >> _direction;
_toDirection = _direction;
tree.cref(index["directionTurret"]) >> _directionTurret;
_toDirectionTurret = _directionTurret;
tree.cref(index["tu"]) >> _tu;
tree.cref(index["health"]) >> _health;
tree.cref(index["mana"]) >> _mana;
...
I checked out BattleUnit::save() to find out which keys are only saved if not default value, so I know which keys I have to be careful about. It's a lot of work...
>Even if current code work fine, you need consider different versions (past and future).
I'm being as careful as I can be~
The real test will be after I'm finished. I'll make sure that all the mods and saved games serialize and deserialize correctly, so they produce identical files.
>If we make loading too fast then changing format become hard as is in binary files.
Haha, I know what you mean. While I am trying to be optimize things here and there, I'm not going to sacrifice readability (ignore that next_sibling case). The code won't look worse than it is after I'm done, I promise!
>If you could replace all with `load(node, "name", value);` I would be for it.
I would be for it also, but, right now there is no abstraction layer. Creating an abstraction layer would be a lot of work, but then it would be even more work to convert all the code to use the abstraction layer. Basically, converting to abstraction layer is more work than direct conversion to rapidyaml.
>Best case could be your pull request: https://github.com/MeridianOXC/OpenXcom/pull/132
I've closed that pull request because it won't matter after I'm done with this conversion.
>that use most optimal code
Even the most optimal code with yaml-cpp is very slow, so any possible improvements are marginal at best. If you'd seen the internal code of rapidyaml, you'd know what I'm talking about. For instance, ryml::to_chars() is faster than even std::to_chars()
>I think probably best way I would see to start all this is add new layer of indirection.
I agree with you, that conversion to an common API would be the best but, that would be a lot of extra work.
Also, I'm already over half way done! In the beginning I was only trying out stuff to see how compatible the libraries were, but I got pretty absorbed into the coding. So the best I can do is finish a direct conversion before I lose the will to code.
>Btw there are other yaml libs, could have less performance as rapidyaml but could have closer semantic to yaml and easier to swich
I couldn't find any. Either they were similar to rapidyaml, or they had bad performance, or no performance data. It seems like writing a yaml library is something people did as a high school project. If the library works, you got a good grade in school!
I think lot better would be wrapper `OpenXcom::Yaml`.
In case of rapid it could store first, last and current nodes. each time you check for `"X"` it check if its current node, if not it loop `next_sibling()` to next one and check if its same, this loop until it reach end, when its reach end, it start from beginning unit reach previous current node.
This reduce cost to one `O(n)` and in "best" case it could have cost `O(1)` if loading order is same as saving.
For rulesets we could make "random" version that allocate flat buffer that sort all nodes based on it.
That's a pretty nifty algorithm. But what if we can guarantee O(1)? In the above code example I loop through children to build an index, and then all use it to get O(1). This is what rapidyaml author wrote we should do when we want fast access. I only build index for large objects tho due to overhead. In general rapidyaml has a lot of options to speed up parsing, reusing parsers, reusing trees and buffers... stuff that is specific to rapidyaml which can't be used with an abstraction layer.
-
Some more yaml-cpp differences:
rapidyaml works on the basis of "no extra luggage".
Features that would slow down parsing or serializing are disabled by default, and you only enable them if you need them. Of course, enabling them slows down parsing, which is why they're best left disabled until necessary.
One of such features is position-in-document.
In yaml-cpp:
Log(LOG_ERROR) << "Error in line " << node.Mark().line;
In rapidyaml:
? ? ?
Nodes don't keep locations of where a specific node is in a document. Which line and column certain node is at. That's parser's job. But we don't have the parser anymore, and even if we had it, it parsed the original yaml without location logging enabled to speed up the parsing. So the solutions is:
// reparse the original yaml but with locations data enabled
ryml::csubstr yaml = node.tree()->arena();
ryml::EventHandlerTree handler = {};
ryml::Parser parser(&handler, ryml::ParserOptions().locations(true));
const ryml::Tree newTree = ryml::parse_in_arena(&parser, yaml);
ryml::Location loc = parser.location(newTree, node.id());
Log(LOG_ERROR) << "Error in line " << loc.line;
The good news is that errors are exceptional behavior, and after the error the game will close anyway, so it doesn't matter if we spend a bit of time parsing the yaml to find out the offending location. Other than that, we generally don't need to know location of nodes in yaml document.
-
Some more yaml-cpp differences:
rapidyaml works on the basis of "no extra luggage".
Features that would slow down parsing or serializing are disabled by default, and you only enable them if you need them. Of course, enabling them slows down parsing, which is why they're best left disabled until necessary.
One of such features is position-in-document.
In yaml-cpp:
Log(LOG_ERROR) << "Error in line " << node.Mark().line;
In rapidyaml:
? ? ?
Nodes don't keep locations of where a specific node is in a document. Which line and column certain node is at. That's parser's job. But we don't have the parser anymore, and even if we had it, it parsed the original yaml without location logging enabled to speed up the parsing. So the solutions is:
// reparse the original yaml but with locations data enabled
ryml::csubstr yaml = node.tree()->arena();
ryml::EventHandlerTree handler = {};
ryml::Parser parser(&handler, ryml::ParserOptions().locations(true));
const ryml::Tree newTree = ryml::parse_in_arena(&parser, yaml);
ryml::Location loc = parser.location(newTree, node.id());
Log(LOG_ERROR) << "Error in line " << loc.line;
The good news is that errors are exceptional behavior, and after the error the game will close anyway, so it doesn't matter if we spend a bit of time parsing the yaml to find out the offending location. Other than that, we generally don't need to know location of nodes in yaml document.
Problem is we need know it, maybe not in save file but in rulefile is critical, without it any error reporting is pointless, consider mod like XPZ and now get errors from file that have 10000 lines, can you say what `prop:` break mod?
I agree with you, that conversion to an common API would be the best but, that would be a lot of extra work.
Also, I'm already over half way done! In the beginning I was only trying out stuff to see how compatible the libraries were, but I got pretty absorbed into the coding. So the best I can do is finish a direct conversion before I lose the will to code.Yes, this will take more time but in long run it will take less time.
You can not even try make common API, only hide most of boilerplate needed by rapid, and more impartiality hide complexity and error handling.
Best example is your code:
const ryml::Tree tree = *node.tree();
std::unordered_map<ryml::csubstr, ryml::id_type> index(node.num_children()); //build and use an index to avoid [] operator's O(n) complexity
for (const ryml::ConstNodeRef& childNode : node.children())
index[childNode.key()] = childNode.id();
tree.cref(index["id"]) >> _id;
tree.cref(index["faction"]) >> _faction;
tree.cref(index["status"]) >> _status;
if (index.count("wantsToSurrender"))
tree.cref(index["wantsToSurrender"]) >> _wantsToSurrender;
if (index.count("isSurrendering"))
tree.cref(index["isSurrendering"]) >> _isSurrendering;
tree.cref(index["position"]) >> _pos;
tree.cref(index["direction"]) >> _direction;
_toDirection = _direction;
tree.cref(index["directionTurret"]) >> _directionTurret;
_toDirectionTurret = _directionTurret;
tree.cref(index["tu"]) >> _tu;
tree.cref(index["health"]) >> _health;
tree.cref(index["mana"]) >> _mana;
when you try load save from OXC then line like `tree.cref(index["mana"])` how will behave? probably you should use `if (index.count("wantsToSurrender"))` every where to be 100% sure we do not crash here.
This make whole code mess and harder to maintain, but if you add new class `YamlReader` that hide all this complexity you could speed up lot of changes in code:
void BattleUnit::load(const ryml::ConstNodeRef &nodeRyml, const Mod *mod, const ScriptGlobal *shared)
{
YamlReader node{ nodeRyml }; //make all allocations and other complex operations
node.load("id", _id);
node.load("faction", _faction);
node.load("status", _status);
node.load("wantsToSurrender", _wantsToSurrender);
...
}
less code to type, and better error handling (like someone put `"abc"` instead of `123`).
This will require less typing, you could use regexp to mass update files or some CodePilot or some thing like this to speed up updates.
This is because you do not need to think what is required or not (this could change too in future!), yes this will cost more but
if we ditch yaml-cpp and reduce time by 99% then increase it by 5 times for error handling and comparability, then will still have net grain as it will be overall 95% reduction.
Overall I look forward to see overall results, how much better OXCE will behave after your change.
-
Ok. In what file should I put YamlReader class, and other yaml stuff? \Engine\Yaml.cpp ?
-
Yes, this is good place for it.
[ps]
I asked Meridian what his option about changing yaml parser, and he said he do not opposite it as long it will work correctly on all supported platforms.
-
I learned a bit more of C++ and I was able to find a solution to the enums problem, so now I don't need to add overload for every Enum we have, yay
// Deserialization template for enums. We have to overload from_chars() instead of read(), because read() already has a template for all types which leads to template conflict
template <typename EnumType>
typename std::enable_if<std::is_enum<EnumType>::value, bool>::type inline from_chars(ryml::csubstr buf, EnumType* v) noexcept
{
return ryml::atoi(buf, (int*)v);
}
>keeping yaml nodes longer
For the problem with copying nodes and keeping them longer, I've decided to convert those nodes back to strings. So for instance, Unit._spawnedSoldier is now std::string instead of YAML::Node. So when we want to use the spawnedSoldierTemplate, the string is parsed to nodes as necessary.
If you want, I can keep ryml::Tree (generated from the node we want to keep) instead of std::string, so performance is better because the string doesn't need to be parsed whenever the spawned soldier template is used. However, I think that those templates are rarely used, and parsing with ryml is fast, so keeping strings is safer.
>errors from file that have 10000 lines
Hmm, the largest .rul file, Piratez.rul has 230k lines so reparsing it could take a long time...
You wanna know how long it takes? 40ms in release build.
Parsing with line logging enabled takes 50ms. So in the worst case, if someone crashes because of an error in Piratez.rul, they will have to wait 50 milliseconds extra due to reparsing.
I think the reparsing is an ok solution because people normally don't crash due to errors in .rul files. Mod authors don't provide users with faulty mod files~
>YamlReader
For now, the normal code usage looks like this:
void BattleUnit::load(const ryml::ConstNodeRef &node, const Mod *mod, const ScriptGlobal *shared)
{
YAML::YamlNodeReader reader(node, true); //useIndex = true
reader.tryRead("id", _id);
reader.tryRead("faction", _faction);
reader.tryRead("status", _status);
reader.tryRead("wantsToSurrender", _wantsToSurrender);
...
std::string spawnUnitType;
reader.tryRead("spawnUnit", spawnUnitType);
if (_spawnUnit = mod->getUnit(spawnUnitType, false)) // ignore bugged types
{
reader.tryRead("respawn", _respawn);
reader.tryRead("spawnUnitFaction", _spawnUnitFaction);
}
reader.tryRead("motionPoints", _motionPoints);
reader.tryRead("customMarker", _customMarker);
reader.tryRead("alreadyRespawned", _alreadyRespawned);
reader.tryRead("activeHand", _activeHand);
reader.tryRead("preferredHandForReactions", _preferredHandForReactions);
reader.tryRead("reactionsDisabledForLeftHand", _reactionsDisabledForLeftHand);
reader.tryRead("reactionsDisabledForRightHand", _reactionsDisabledForRightHand);
if (reader.hasChild("tempUnitStatistics"))
_statistics->load(reader.getChild("tempUnitStatistics"));
reader.tryRead("murdererId", _murdererId);
...
I decided to go with read. The english word load doesn't fit well.
- load is best used when bringing data into memory from an external source
- read is best used when processing some sort of input and may or may not involve interpreting data
- parse is best used when interpreting raw data
What we're doing is, first we load file data into a string, we parse the raw data string into a node tree, and then process individual nodes, which involves reading the data from nodes, parsing it to the correct type (could also say "converting", but convert is used in C#, not in C++), and saving it into individual variables. We could use the term "parse" both for parsing into node tree and for parsing nodes, but using the same term for two things is confusing. Anyway, based on what the function is doing, the precise function name would be... FindChildNodeAndReadItsValueAndParseItIntoOutputVariable, but that would be too long hehe, so I think shortening it to "read" is the best option.
Hmm. Names like loadBool are confusing...
I made read and tryRead. tryRead fits well because it signifies that reading isn't guaranteed. The read version guarantees read, or crash. Sometimes we want to crash if a value is missing. But I also added an overload that takes a default value. If you provide a default value, then it doesn't crash.
I have a question. Which code is easier to use?
1. reader.tryRead("faction", _faction); - Simple, but not super intuitive due to passing output as a function parameter
2. _faction = reader.read("reaction", _faction); - Read guarantees reading if we provide a default value, but coding is annoying because _faction is duplicate
3. _faction = reader.tryRead("faction"); - Looks nice, doesn't it? It works the same as the 1st option. However, it requires tag dispatching.
-
Ok, look good, for `reader.tryRead("faction", _faction);` is probably best as could easy avoid allocations when `_faction` is bigger object like vectors.
Last version could need lot of "magic" to avoid overriding values if node do not exists. I not sure if its even possible as assignment operator need be class member
and we can't add it to `int` or `std::vector`.
For `read` vs `load` this is bikeshedding, I go with `load` but not as english word but more a "keyword" in OXC codebase. But you should not concern yourself with it as it take 1s to change it in whole code base, you could even make `foo` and `bar`.
For replacing `YAML::Node` with `std::string`, yes and no, we can store it as string but it should not be string, it should be dedicated struct that contains this data and probably nothing else. Consider:
std::string foo;
YamlCopy bar;
can you explain what is `foo` and what is `bar`? in first case you can't aside to say its some text, in second you see in type itself that this is yaml data.
For speed, if base time take 40ms and "full" take 40+50ms I would enable it by default. At least for rulesets but saves could be useful for Meridian as he need debug broken saves by manual edits sometimes. We already order of magnitude faster and we should not trade all "easy to use" and "code simplicity" to be even faster as loading is only small fraction of whole user playtime.
-
>you see in type itself that this is yaml data
If I understand correctly, you're saying I should define:
typedef std::string YamlString;
That's possible. Well, normally the variable name itself should be self-explanatory enough about what it is. What is better then?
std::string _spawnedSoldierYaml; or
YamlString _spawnedSoldier; ?
-
Close, I mean dummy struct like:
strcut YamlString
{
std::string yaml;
};
Because `typedef` help with readability but not prevent typos, like you try send `_spawnedSoldier` to `getSolderType(_spawnedSoldier)` and it will compile fine.
You can add some helpers to this `YamlString` to easier get data from it like:
YAML::YamlNodeReader reader(_spawnedSoldier, true); //create `ryml::ConstNodeRef` on the fly and other boilerplate to make `YamlNodeReader` wrok
-
Ok.
Next thing I'm wondering about is... should all the save and load functions pass around readers/writers instead of nodes, e.g.:
instead of
void BattleUnit::load(const ryml::ConstNodeRef& node, const Mod *mod, const ScriptGlobal *shared)
{
YAML::YamlNodeReader reader(node);
reader.useIndex(true);
reader.tryRead("id", _id);
_statistics->load(reader.getChild("tempUnitStatistics"));
...
void BattleUnit::save(ryml::NodeRef node, const ScriptGlobal *shared) const
{
node |= ryml::MAP;
YAML::YamlNodeWriter writer(node);
writer.write("id", _id);
_statistics->save(writer.writeKey("tempUnitStatistics"));
...
is it better to use
void BattleUnit::load(const YAML::YamlNodeReader reader, const Mod *mod, const ScriptGlobal *shared)
{
reader.tryRead("id", _id);
_statistics->load(reader.getChild("tempUnitStatistics"))
...
void BattleUnit::save(YAML::YamlNodeWriter writer, const ScriptGlobal *shared) const
{
writer |= ryml::MAP;
writer.write("id", _id);
_statistics->save(writer.writeKey("tempUnitStatistics"));
...
?
-
Second is better in my opinion, less boilerplate and look more similar to old code.
btw you do not use `&`? what copy sematic you assign to this new types?
-
I only started learning C++ two weeks ago, I don't know much about all the const and & stuff :(
fucking... [&&] const &function(const int* const& &name) const!
getChild finds a node with a key and wraps it into a new YamlNodeReader instance and returns it
writeKey creates a new child node with a key and wraps it into a new YamlNodeWriter instance and returns it
I don't really know what you mean with "copy semantics".
-
In our case is do this class behave as `int*` or like `std::vector<int>`, both can refer to list of integers but copy work different.
int* p = new int[]{1, 2, 3};
int* b = p;
b[0] = 42;
assert(p[0] == 42); // both change same memory localization
where for `std::vector`:
std::vector<int> p{1,2,3};
std::vector<int> b = p;
b[0] = 42;
assert(p[0] == 1); // each one have unique memory
As rapid do try avoid copy memory is possible that you have first case, know as "shallow copy" and refer as "not owing memory",
where vector have "deep copy" and "own memory".
Then if you have type with "deep copy" to avoid coping lot of memory every time you try send it to function you
use reference `&` to send object effective as pointer but with better syntax (`.` instead of `->` and `*`) and not-null guarantee.
In most cases this is used as `const T& arg` and this mean "I only want quick look on this object and I promise I do not change anything".
-
Ok, I'll use & as much as possible then.
In some cases I was having issues, for instance
_blabla.save(node.append_child("key"));
If the save function uses &node, then this would give me some lvalue error or whatever. But if I remove the &, then there's no error.
alternatively, I could use
Node child = node.append_child("key");
_blabla.save(child);
But this is more annoying to code with, because of having to create a variable every time before calling the save/load function.
This was before, when I was passing around ConstNodeRef. With YamlNodeReader there doesn't seem to be an issue with using the 1st option.
-
I not asking you to add `&`, in some cases passing as value (without any `&`) is preferred, especially if type do not allocate memory or need to share same object in multiple places.
Have you this code available in github? probably better would be look on code itself as sometimes small things could affect answer to questions like this.
-
Been working on it for the past two days, but until it's all done, the main project is not gonna compile.
I've whipped up a separate test project, which does compile. You can check it out and tell me all the things I'm doing wrong lol
Today I wasted at least 2 hours trying to figure out why I was getting linker errors. Because I put the template function implementations in the .cpp file...
-
I found out that there are a couple of "bugs" in rapidyaml, but I think they're fixable.
First is the fact that rapidyaml author didn't have the time to add any output formatting options for yaml. By default, rapidyaml will output flow style nodes delimited with comma instead of comma+space ([1,2,3] vs [1, 2, 3]), like we currently do. So, I fixed this by editing rapidyaml.hpp single header file...
In Emitter<Writer>::_do_visit_flow_sl(), line 26107, I changed:
this->Writer::_do_write(',');
to
this->Writer::_do_write(", ");
I'm not sure if it's better for us to include rapidyaml as a single header file, or as separate files but, since the library author is afk, modifying those files is basically the only option I think. Hmm. I suppose it would also be possible to do a partial yaml emit of a flow style node and use string replace, but that might cause other bugs, plus it wouldn't be efficient (because we use flow style a lot).
The second bug I already reported at https://github.com/biojppm/rapidyaml/issues/475
That one is a bit more critical, because it affects some mods (for instance XPZ), and causes parsing failure. So either I fix it in the library, or the mod authors have to run a regex [ ]+$ through their .rul files to remove the offending trailing whitespace. Since there's technically nothing wrong with trailing whitespace, I think the fixing it in the library is the correct choice.
Edit: I fixed the 2nd bug in the library as well. There were 5 places where the parser forgot to skip whitespace.
-
Some random stuff I encountered today...
- I noticed that with proper buffer management (by setting large enough initial buffers), I can easily get another 5% parsing speed increase.
- Converting base64 binTiles encoding to rapidyaml was a chore...
- I came across some funny code in SaveGame::load()
// Finish loading crafts after bases (more specifically after all crafts) are loaded, because of references between crafts (i.e. friendly escorts)
{
for (YAML::const_iterator i = doc["bases"].begin(); i != doc["bases"].end(); ++i)
{
// Bases don't have IDs and names are not unique, so need to consider lon/lat too
double lon = (*i)["lon"].as<double>(0.0);
double lat = (*i)["lat"].as<double>(0.0);
std::string baseName = "";
if (const YAML::Node &name = (*i)["name"])
{
baseName = name.as<std::string>();
}
Base *base = 0;
for (auto* xbase : _bases)
{
if (AreSame(lon, xbase->getLongitude()) && AreSame(lat, xbase->getLatitude()) && xbase->getName() == baseName)
{
base = xbase;
break;
}
}
if (base)
{
base->finishLoading(*i, this);
}
}
}
I changed it into
for (int i = 0; i < _bases.size(); ++i)
_bases[i]->finishLoading(reader["bases"][i], this);
it was using some convoluted way to compare bases, but since the _bases vector has the same order as the bases in the save file, it's much simpler to use this order to match them.
-
I found out that there are a couple of "bugs" in rapidyaml, but I think they're fixable.
First is the fact that rapidyaml author didn't have the time to add any output formatting options for yaml. By default, rapidyaml will output flow style nodes delimited with comma instead of comma+space ([1,2,3] vs [1, 2, 3]), like we currently do. So, I fixed this by editing rapidyaml.hpp single header file...
In Emitter<Writer>::_do_visit_flow_sl(), line 26107, I changed:
this->Writer::_do_write(',');
to
this->Writer::_do_write(", ");
I'm not sure if it's better for us to include rapidyaml as a single header file, or as separate files but, since the library author is afk, modifying those files is basically the only option I think. Hmm. I suppose it would also be possible to do a partial yaml emit of a flow style node and use string replace, but that might cause other bugs, plus it wouldn't be efficient (because we use flow style a lot).
The second bug I already reported at https://github.com/biojppm/rapidyaml/issues/475
That one is a bit more critical, because it affects some mods (for instance XPZ), and causes parsing failure. So either I fix it in the library, or the mod authors have to run a regex [ ]+$ through their .rul files to remove the offending trailing whitespace. Since there's technically nothing wrong with trailing whitespace, I think the fixing it in the library is the correct choice.
Edit: I fixed the 2nd bug in the library as well. There were 5 places where the parser forgot to skip whitespace.
I don't want any custom bugfixes.... unless ABSOLUTELY necessary.
1/ Output formatting I don't care about... if I understand correctly it is just a cosmetic thing, not functional, right?
2/ Trailing whitespaces... on the frontpage he writes something about known limitations (https://github.com/biojppm/rapidyaml?tab=readme-ov-file#known-limitations), there are trailing tabs mentioned there... don't know if it's the same or not... but if yes, then I'd rather ask modders to remove trailing whitespaces instead of losing 10% performance
3/ Regarding including rapidyaml, I'll need to discuss with Yankes and SupSuper, but my personal preference would be:
a/ "as is" (just copypaste source snapshot one to one into our repo)
b/ as single hpp file
c/ as static library
d/ as dynamic library -- this I really don't like, after all we've been through over the years with yamlcpp
-
1. Yes, it's just cosmetic. I'll leave that change out then.
2. The known limitations are about tabs, not spaces. The parser crashes with extra " " spaces at the end of flow-style lines. The standard says that those should simply be ignored, so it's either a bug, or a limitation.
I've tested the performance difference with and without a custom fix with the extra whitespace checking. It's <0.5% performance difference, so that shouldn't be a consideration. But I agree that, custom bugfixes are a nasty solution because they make updating libraries a nightmare. Going with the "tell mod authors to remove trailing spaces" solution, I'll have to make sure that when the game encounters such a parser error, that it's correctly communicated to the user.
3. Ok, I'll try to produce a test project that includes a 1:1 copy of rapidyaml/src/ instead of the single header file.
Anyway, another formatting difference is, sequences don't start new lines:
Before:
moduleMap:
-
-
- 1
- 1
-
- 2
- 2
-
- 3
- 3
-
-
- 1
- 1
-
- 2
- 2
-
- 3
- 3
-
-
- 1
- 1
-
- 2
- 2
-
- 3
- 3
After:
moduleMap:
- - - 1
- 1
- - 2
- 2
- - 3
- 3
- - - 1
- 1
- - 2
- 2
- - 3
- 3
- - - 1
- 1
- - 2
- 2
- - 3
- 3
Lastly, the yaml standard says that certain non-printable characters should be escaped, as in, ascii code 00 should produce "\x00" in the yaml. It also states that it's not mandatory. Unlike yaml-cpp, rapidyaml does not respect this suggestion hehe. It simply outputs a control code 00 character raw. Subsequent parsing of these non-escaped characters works just fine however. It even works fine in yaml-cpp, so it's perfectly backwards compatible. As long as a string is in double quotes, everything goes.
-
I don't want any custom bugfixes.... unless ABSOLUTELY necessary.
1/ Output formatting I don't care about... if I understand correctly it is just a cosmetic thing, not functional, right?
2/ Trailing whitespaces... on the frontpage he writes something about known limitations (https://github.com/biojppm/rapidyaml?tab=readme-ov-file#known-limitations), there are trailing tabs mentioned there... don't know if it's the same or not... but if yes, then I'd rather ask modders to remove trailing whitespaces instead of losing 10% performance
3/ Regarding including rapidyaml, I'll need to discuss with Yankes and SupSuper, but my personal preference would be:
a/ "as is" (just copypaste source snapshot one to one into our repo)
b/ as single hpp file
c/ as static library
d/ as dynamic library -- this I really don't like, after all we've been through over the years with yamlcpp
e) use submodule, only drawback that after `git clone` will require additional operations to fetch all submodules.
f) fast google show that cmake can use `ExternalProject_Add` that allow grab some git repo
both have one benefits that we can point to fork at first, but when base lib get updated we can redirect both to official version.
For speed, with rapid we have lot of "free" speed budget grained after ditching yaml-cpp, this means we can spend it on making code more compatible with old version and still have big grain in speed after that.
-
Here's another simple test project for the current iteration of our wrapper that also includes a performance test.
I've removed the single header file and included src files from the rapidyaml. Note that rapidyaml has git folder references to other projects, so I had to get src files from those projects as well. Basically, get src files from:
rapidyaml/src/
rapidyaml/ext/ -> c4core/src/c4/
rapidyaml/ext/ -> c4code/src/c4/ext/ -> debugbreak/debugbreak.h
I had to add all of them to the project file, otherwise it wouldn't compile... I wonder if there's a way to build VS projects with external cpp files.
e) use submodule
rapidyaml author says to use:
git clone --recursive https://github.com/biojppm/rapidyaml
In the README he includes all the instructions about how to use it as a submodule as well. Ways that use CMake and stuff which I have no clue about lol
Btw, compiling with src files instead of a single header file takes waaay longer.
-
I'm done converting everything in /src/Savegame/. Now for the rest of the folders...
Btw, I think I found a small bug. When we're saving soldiers to yaml, if they're on a craft, we store the "craft unique id", and when we're loading soldiers from yaml, we set the soldier's _craft to the one that's in the base with equal "craft unique id". The problem is that we are storing a "target id" instead of "craft unique id". This is because Craft is missing an override of Target's virtual saveId() function (for instance, Ufo provides the saveId() override, but Craft does not). The consequence of this bug is that two redundant nodes are stored hehe.
rank: 1
craft:
lon: 1.803321073590076
lat: -0.46181407437014516
type: STR_PIRANHA_AIRBIKE
id: 1
gender: 1
versus
rank: 1
craft:
type: STR_PIRANHA_AIRBIKE
id: 1
gender: 1
The lon and lat are unnecessary; they're not read when loading a save file. Should I add a saveId() override to Craft to fix this?
-
Should I add a saveId() override to Craft to fix this?
No, don't add, remove or fix anything.
Also no (re)formatting of the old/existing code please.
When I (or anybody else) look at this change in the future, I want to see only stuff relevant to this migration, nothing else.
For ideas and bugfixes, create new posts, they will be handled later/separately.
-
No, don't add, remove or fix anything.
Also no (re)formatting of the old/existing code please.
When I (or anybody else) look at this change in the future, I want to see only stuff relevant to this migration, nothing else.
For ideas and bugfixes, create new posts, they will be handled later/separately.
This,
@Delian and if you find some things to reconsider or some thing you think is strange you can always add some comments.
-
I've released oxce 7.15, you can rebase your commit on that.
I won't push anything else before this is resolved (one way or another).
Here's another simple test project for the current iteration of our wrapper that also includes a performance test.
Btw, compiling with src files instead of a single header file takes waaay longer.
How much is "waaay longer" ?
Your test project compiled in 8 seconds for me.
a/ and e/ are practically same for me, submodule is just another form of 1:1 copy.
The most important for me is that it can be compiled on any imaginable platform easily, and that we can say which version exactly we use.
I don't want Linux/macOS tell me anymore that they distribute a different version (or even a different product with unreliable -compat package) and I can go f myself.
-
Ok, I've synced my fork. But you shouldn't stop just because of me...
Yeah, it takes about 8s for me as well (7.1s) to compile the test with the 1:1 src files copy. But if I instead use the amalgamated single header file, the test project compiles in 2.7s. The single header that comes with each rapidyaml release still contains all the src code. It doesn't make much sense lol
Well, in the end it's just an extra 5s. Whether compiling the whole OpenXcom project takes 45s, or 50s, it shouldn't matter. I will again test the difference after I'm done with the conversion, to see if the difference is the same 5s or greater.
-
Whether compiling the whole OpenXcom project takes 45s, or 50s, it shouldn't matter.
It takes 3 minutes and 50 seconds for me, 8 seconds will indeed get lost there.
But you shouldn't stop just because of me...
It will be easier for me too.
-
It takes 3 minutes and 50 seconds for me, 8 seconds will indeed get lost there.
(https://i.postimg.cc/RVsFB7wt/meme.png)
You gotta invest in more cores man!
if you find some things to reconsider or some thing you think is strange you can always add some comments.
You mean add comments to the code? Or to talk about what I find here?
In the Engine/Options.cpp there's a function called writeNode(). I cannot convert this function. What to do?
I know what the function does and I can easily replicate the functionality in a much simpler way. But the function itself is useless and unconvertable because ryml doesn't have such manual emitting capabilities. So what do I do? Remove the function? Comment it out?
-
I have 10 cores, but they're all lazy drunks :)
-
You mean add comments to the code? Or to talk about what I find here?
In the Engine/Options.cpp there's a function called writeNode(). I cannot convert this function. What to do?
I know what the function does and I can easily replicate the functionality in a much simpler way. But the function itself is useless and unconvertable because ryml doesn't have such manual emitting capabilities. So what do I do? Remove the function? Comment it out?
I mean normal code base comments like:
foo(); // this code is have no sesne as it should be `bar()` here
for this `writeNode` probably if you can recreate end result of `bool save(const std::string &filename)` then you could probably remove this functions.
After git digging show this: https://github.com/OpenXcom/OpenXcom/commit/01f78550bdfb39f2a862f314d0fba32d683ca0d7
As I see goal was to have sorted positions in saved config file.
-
As I see goal was to have sorted positions in saved config file.
Yes, all it does is sort map nodes based on node keys. And I accomplish the same by changing:
for (const auto& optionInfo : _info)
{
optionInfo.save(node);
}
into
auto sortedInfo = _info;
std::sort(sortedInfo.begin(), sortedInfo.end(), [](const OptionInfo& a, const OptionInfo& b) { return a.id() < b.id(); });
for (const auto& optionInfo : sortedInfo)
{
optionInfo.save(optionsWriter);
}
By sorting the vector before the nodes are even created. 2 lines of code. Same output.
-
There is a potential bug in Engine/Language::loadFile()
When loading a language file, if the yaml is missing the language specifier (e.g. en-US), and the 1st entry contains plurality, loading the language file will fail.
Suggested fix: Instead of checking if the first child of the root is a mapping, check if the root node has a 2nd child. If it's missing a 2nd child, then we can be sure the 1st child is the language specifier, and not coincidentally an entry with plurality.
if (doc.begin()->second.IsMap())
{
lang = doc.begin()->second;
}
to
if (std::next(doc.begin()) == doc.end())
{
lang = doc.begin()->second;
}
Just leaving this here for later.
Next problem: The trailing tabs that we talked about. Yeah, some mods have .rul files with such tabs, and those files fail to parse. So removing both trailing spaces, and trailing tabs will be necessary for the modders. Regex that finds trailing spaces after a flow style line, or trailing tabs, or trailing tabs with a comment: (?<=[\]\}])[ ][\t ]*$|\t[\t ]*$|\t+#.*$|\t[\t ]*(?= #.*$)
Lastly, I need a function that gets a size of a file, but I have no clue how to write one.
When saving game, I want to check if the destination .sav file already exists, and if it exists, I want to get its file size. This is because I want to use that filesize for better estimation of the needed writer buffer size.
-
I noticed that we're using yaml tags. I can understand custom tags like !add !remove and !info. But why do I see !!map and !!seq in the code? Yankes, do you remember anything about those?
In RuleEventScript.cpp there's a block of code with a comment // deprecated, remove after July 2022.
In RuleItem.h there's two functions (loadCost, loadPercent) that are missing a function body.
Progress? Only Mod.cpp left to convert...
-
`!!map` and `!!seq` are buldin yaml tags used for map and seq. In most cases used only in OXCE as info name as yaml-cpp use longer `tag:yaml.org,2002:seq` name for same thing. If I recall correcty if you put in file value `!!seq`, yaml node should return tag `tag:yaml.org,2002:seq`.
I use them to detect what type of node it is and how I should handle it in helpers
For "deprecated" is old version of code that is keep only for sake of backward compatibility, I would keep it for now or add `checkForObsoleteErrorByYear`
as I added automatic code that "disable" things like this based on compilation time (and have option to for some time still allow to load old non compatible mods).
For `loadCost` this is leftover after old code that was moved to dedicated class, you can remove it (best if it will be in different commit as is irrelevant to your refactor).
-
`!!map` and `!!seq` are buldin yaml tags used for map and seq. In most cases used only in OXCE as info name as yaml-cpp use longer `tag:yaml.org,2002:seq` name for same thing. If I recall correcty if you put in file value `!!seq`, yaml node should return tag `tag:yaml.org,2002:seq`.
I use them to detect what type of node it is and how I should handle it in helpers
I think the tags are only supposed to tell you how a node should translate into a native data structure in the application. They're little more than suggestions. They don't tell you what type of a node a yaml node is. If you use yaml-cpp, and you try to get Tag() of a mapping node, you will get "" or "?" back, not "!!map" and not "!<tag:yaml.org,2002:map>". Yaml node types, and types defined with tags, the two are entirely independent of each other. To detect what type of node it is, the functions IsMap, IsSequence, IsScalar, etc., are all you need. It's possible to manually write !!map to a sequence node in the yaml, but that's just personal error (and it won't affect parsing anyway).
Anyway, rapidyaml doesn't care about implicit tags so it's a tiny bit different than yaml-cpp. Unless a tag is explicitly defined, node.has_val_tag() will return false, and node.val_tag() will throw. Certain functions like "isMapHelper(node)" are effectively the same as node.isMap(), so I'll just remove them.
In the Mod::loadFile() I've been having trouble converting the lambda iterators... maybe I should implement an iterator interface for YamlNodeReader, instead of the current children() method that builds and returns a vector. It feels a bit inefficient.
Edit:
I'll have to study some of those helpers a bit more...
-
1>Done building project "OpenXcom.2010.vcxproj".
========== Build: 1 succeeded, 0 failed, 0 up-to-date, 0 skipped ==========
8)
-
In the Mod::loadFile() I've been having trouble converting the lambda iterators... maybe I should implement an iterator interface for YamlNodeReader, instead of the current children() method that builds and returns a vector. It feels a bit inefficient.Yes iterator would be preferred, as it should be possible without any allocations, you could probably make "fat" iterator that have all state and implement only `++`, `*` and `==`.
-
Error in bin/common/SoldierName/Polynesia.nam
last line:
- 'Akamu
Not valid YAML: unclosed single-quoted flow scalar
Should I wrap it in double-quotes, e.g. - "'Akamu" to fix the error?
-
if `'` is "letter" and not quote then, Yes
-
Ok. Some more bugs from before:
Armor::load()
_allowsMoving = node["allowsMoving"].as<bool>(_allowsMoving);
should be
loadBoolNullable(_allowsKneeling, node["allowsKneeling"]);
Because in the yaml it's stored as bool, but in program it's loaded as sint8
I also have a problem:
In a few rare cases, in the yaml I have a node with a value 10.0, which is obviously a float, but deserialization crashes because it wants to deserialize into an int. The question is, is this faulty yaml? Or faulty deserialization?
-
Starting up game (XPZ) in debug mode...
Startup time:
Before: 130s
After: 17s
RAM usage in debug mode:
Before: 1450MB
After: 750MB
Before, if you ran XPZ in debug mode and loaded a large save, game would crash if it reached 1.8GB RAM (out of memory), so couldn't even debug certain large saves...
Some screenshots of a profiling run
Before:
(https://i.postimg.cc/xTkFFpK8/Screenshot-2024-11-07-162908.png)
After
(https://i.postimg.cc/dtzSMRBB/Screenshot-2024-11-07-162959.png)
I haven't tested game saving and loading yet, which is next on the list.
-
Ok. Some more bugs from before:
Armor::load()
_allowsMoving = node["allowsMoving"].as<bool>(_allowsMoving);
should be
loadBoolNullable(_allowsKneeling, node["allowsKneeling"]);
Because in the yaml it's stored as bool, but in program it's loaded as sint8
I also have a problem:
In a few rare cases, in the yaml I have a node with a value 10.0, which is obviously a float, but deserialization crashes because it wants to deserialize into an int. The question is, is this faulty yaml? Or faulty deserialization?
What property exactly? overall if game save `10.0` we should load it as `float` even if we expect `int`, we need keep backward compatiblity, we fix saving.
I think probaby best would be custom function of it like `loadIntFixed` or some thing like that that check for `float` too.
-
Before, if you ran XPZ in debug mode and loaded a large save, game would crash if it reached 1.8GB RAM (out of memory), so couldn't even debug certain large saves...
Yeah, happened to me many times.
Btw. why does it crash at 1.8GB if I have 32GB RAM? Something to do with 32bit? Why not 2.0GB?
Maybe a stupid question, but I'm curious.
-
What property exactly? overall if game save `10.0` we should load it as `float` even if we expect `int`, we need keep backward compatiblity, we fix saving.
I think probaby best would be custom function of it like `loadIntFixed` or some thing like that that check for `float` too.
There are 8 places where Dio accidentally set Item.damageAlter.FireThreshold to a float value instead of int, that's all. No other problematic places for this sort of error in XPZ.
According to the yaml standard, if an untagged plain scalar matches the regular expression -?(0|[1-9][0-9]*), it should resolve to int. So technically 10.0 cannot be int and it's correct parser behavior that parsing fails there.
As far as backwards compatibility goes, that's true. But this migration already breaks compatibility due to the issues with trailing space and trailing tabs. If the modders will have to fix those, we might as well tell them where they put in floats instead of integers.
Btw. why does it crash at 1.8GB if I have 32GB RAM? Something to do with 32bit? Why not 2.0GB?
Maybe a stupid question, but I'm curious.
I think it's got something to do with shared graphics memory. Some address space is reserved so that the application can address graphics card's memory, but then it cannot use those address space itself.
-
There are 8 places where Dio accidentally set Item.damageAlter.FireThreshold to a float value instead of int, that's all. No other problematic places for this sort of error in XPZ.
According to the yaml standard, if an untagged plain scalar matches the regular expression -?(0|[1-9][0-9]*), it should resolve to int. So technically 10.0 cannot be int and it's correct parser behavior that parsing fails there.
As far as backwards compatibility goes, that's true. But this migration already breaks compatibility due to the issues with trailing space and trailing tabs. If the modders will have to fix those, we might as well tell them where they put in floats instead of integers.
right, I forget you work with `Mod` and not with `SaveGame`, in this case this was wrong and mod should be updated as it was never expected to accept floats.
Optionally we could add "grace period" of year before we remove loading of `float`s (`Mod` class have special functions for this)
-
Btw. why does it crash at 1.8GB if I have 32GB RAM? Something to do with 32bit? Why not 2.0GB?
Maybe a stupid question, but I'm curious.
32 bit apps in Windows can only allocate 2 GB of memory (4 GB for 32 bit address, 2 GB system space and 2 GB user-space). 2 GB is already past limit, so it crashes a bit before that.
Memory can be increased to 3 GB by using /LARGEADDRESSAWARE linker flag. In fact, I can only launch XPZ in 32-bit OXCE with it enabled.
-
ok, I recalled that 64bit had split of address space where "negative" values belong to OS and rest of to user space.
Add reserved memory by hardware and we end up with 1.8GiB possible memory to use on 32bit OS per process.
I think we could anyway add some thing like this:
https://stackoverflow.com/questions/14035065/codeblocks-compile-with-large-adress-aware-flag
-
There are 8 places where Dio accidentally set Item.damageAlter.FireThreshold to a float value instead of int, that's all. No other problematic places for this sort of error in XPZ.
According to the yaml standard, if an untagged plain scalar matches the regular expression -?(0|[1-9][0-9]*), it should resolve to int. So technically 10.0 cannot be int and it's correct parser behavior that parsing fails there.
As far as backwards compatibility goes, that's true. But this migration already breaks compatibility due to the issues with trailing space and trailing tabs. If the modders will have to fix those, we might as well tell them where they put in floats instead of integers.
I think it's got something to do with shared graphics memory. Some address space is reserved so that the application can address graphics card's memory, but then it cannot use those address space itself.
I make some digging and I see that I make some mistake in code as property is `int` but loaded as `float`, this mean Dio is technically correct using `10.0`.
Now I only need `int` as final damage is always integer but on other hand is easy make mistake as only two of dozen of properties there are `float` there.
-
Hmm... oh, maybe that's why yaml-cpp didn't throw an error before.
Anyway, man, debugging yankes code is so hard because all the variable names are a single letter. Also, I'm basically done. Loaded+saved a few large battlescape sav files, and the output was basically identical. I can pull the code to my git fork now if you want.
Everything works so nice and fast now... well, almost everything. Script parsing and vector sorting are now the major factors of game startup. Sorting wouldn't be necessary if everyone just used the correct data structures in the first place, but that's a problem for another time.
Btw, Yankes, in Script.cpp:694 - parse(), you have a stringstream there. That ss alone now consumes 4% of the whole game load time lol
Now where was I... ah, I gotta download and test all the other mods...
-
Yes, please push the code to your remote repo for review.
-
Hmm... oh, maybe that's why yaml-cpp didn't throw an error before.
Anyway, man, debugging yankes code is so hard because all the variable names are a single letter. Also, I'm basically done. Loaded+saved a few large battlescape sav files, and the output was basically identical. I can pull the code to my git fork now if you want.
Everything works so nice and fast now... well, almost everything. Script parsing and vector sorting are now the major factors of game startup. Sorting wouldn't be necessary if everyone just used the correct data structures in the first place, but that's a problem for another time.
Btw, Yankes, in Script.cpp:694 - parse(), you have a stringstream there. That ss alone now consumes 4% of the whole game load time lol
Now where was I... ah, I gotta download and test all the other mods...
it was 0.4% before :P
Beside vector sorting should be in most cases `O(n*log(n))` plus access `O(log(n))` and its effective "flat-map" (in case comparing only pointers adresses).
I do not see big potential (max `log(n)`) in speedup as current version is very cache friendly. Of core this is only my assumption and could not match reality fully but most C++ talks say that vector is best data structure even if you remove elements some times.
I think I could drop this last 4% to by switching to some better int parser in scripts.
-
Hmmm... maybe worth knowing, but maybe not... I've switched a lot of the resource loading and unloading to Json for the Lua/Vulkan branch. Granted, it's not a released branch and I'm still a fair ways away from releasing it, but I thought it might be worth mentioning.
The way I deal with save game, mod loading(rulesets and so on) and config stuff is through serializers which are either hand crafted or auto-generated from the run time type information. What this means is that there is a considerable increase in compile times and executable size due in large part to the compilation and linking of a code generator which then needs further compilation, but saves time on loading during runtime. For Lua and Json, all the bindings are automatic, but since a goal of the Lua/Vulkan project is to retain backwards compatibility, then I manually put in the YAML serializers so they conform to the original OXCE rules. In fact, it makes it nicer because I can put whatever I want in the Json and not have to worry if I might break some mod out there that someone wrote a long time ago. Granted, I've only made it as far as the config stuff at the moment(config, localization, and a few other bits), so I might run into some issue down the line which might make it more of a problem, but working with Json in debug is so much faster than YAML. However, equally, it is worth mentioning that I barely notice YAML in release mode.
Use this information however you see fit. I'd be happy to see SOMETHING improve debug loading and unloading of mods. The Lua/Vulkan branch is really broken at the moment and I won't make any promises as to when it will be released, so there's also that to consider :D
-
How this is relevant? Goal of this thread is make loading faster not throwing away all code and making some "framework" that do not give anything more useful that current code do.
-
Umm, are all the line endings in OpenXcom src files LF? or CLRF?
If I download the project from the remote repo, the src files are in LF. But if I clone the repo, I get CLRF endings in all the files. I don't want to commit the files with wrong line endings lol
-
Since 2018, they get automatically converted to CRLF on checkin
and automatically converted to CRLF or LF or CR on checkout (depending on your operating system).
https://github.com/OpenXcom/OpenXcom/pull/1183
-
Ok, https://github.com/Delian0/OpenXcom
I went through all the changes again. Also, I had to convert the project to rapidyaml src instead of single hpp file.
There was a conflict between some files. Savegame\Node.cpp and Engine\Language.cpp with rapidyaml\...\Node.cpp and , and rapidyaml\...\Language.cpp. To fix the problem, I went to the properties of the two of our files and set the Output Files -> Object File Name from $(IntDir) to $(IntDir)%(RelativeDir). However, I think this slows down compiling by a bit because for some reason those two don't compile in parallel with the rest of the files anymore. Not sure.
I also had to modify .gitignore and change [Dd]ebug*/ to [Dd]ebug/ , otherwise it would ignore the \rapidyaml\c4\ext\debugbreak\ folder.
-
One compilation error.
But after removing that line, it compiles for me too.
-
Oops, fixed. yaml-cpp is hard to unroot...
Anyway, again, if you wanna test it with XPZ, there will be parsing errors, and you'll need to use the (?<=[\]\}])[ ][\t ]*$|\t[\t ]*$|\t+#.*$|\t[\t ]*(?= #.*$) regex to find and replace the found places in... Piratez.rul, Piratez_Factions.rul, Piratez_Planet.rul, and remove the offending decimals from lines with FireThreshold: [0-9]+\.[0-9] in Piratez.rul
-
When parsing or serializing fails, rapidyml usually throws a std::runtime_error. I have implemented a custom error handler for rapidyml exceptions, but for now it simply rethrows the exception. Should I make it throw a different type of exception? Before we had a YAML::Exception thingy, so maybe it would be a good idea to re-implement that and throw that instead of std::runtime_error.
X-com files:
Error in ufos_XCOMFILES.rul, line 4444: comment not preceded by whitespace
According to the standard (https://yaml.org/spec/1.2.2/#66-comments), comments that don't start at the beginning of a line have to be preceded by a whitespace. Surprising yaml-cpp ignored that one. I thought yaml-cpp was more adherent to the standards. I'll have to modify the above regex a bit hehe.
Whitespace error in ufoTrajectories_XCOMFILES.rul
Missing semicolon in AgentName\Belarusian.nam line 304
Unclosed quote in AgentName\Polynesia.nam, same 'Akamu issue
Could not deserialize value in 29 places in items_XCOMFILES.rul... oh, FireThreshold again. Ha, so this means Dio just copied SS's mistake!
There was a problem with parsing UTF-8 files with BOM. I had to make a workaround that skips the BOM if it encounters it. I guess it's a bug in rapidyaml. Issue reported! Anyway, X-Com Files seems to work fine now.
-
Yes custom exception is preferred.
For `FireThreshold` I would now lean to consider this as your error not Dio or Sol. And consider their version as "canonical".
-
Ok, YAML::Exception done.
lol, don't blame me for uncovering your error, I just made the code work correctly, in line with the specifications. If you want special handling of that one particular node, that's possible, but decide on it with Meridian.
Hmm. This makes me wonder, how would yaml-cpp handle the error? I tested it, and it's very troubling. If there's a float in yaml (e.g. "1.0"), but you try to parse it out as int, you get a YAML::Exception or a fallback value. Since we often use fallback values this means that... yaml-cpp just quietly fails, which is terrible! It leaves modders wondering why something doesn't work, without giving any error.
-
If you want special handling of that one particular node, that's possible, but decide on it with Meridian.
Yes, probably Meridian will lean too on backward compatibility in this case.
It leaves modders wondering why something doesn't work, without giving any error.
Yes, this is know quirk of how yaml-cpp handle it, this is why when I write new code I used version without fallback that make hard error.
-
Final Mod Pack:
Belarusian.nam, Polynesia.nam
research_FMP.rul:637 "listOrder: ???" Expected int, found three question marks
alienDeployments_FMP.rul flow style trailing whitespace
Hardmode Expansion:
No errors
40k:
Scripts40k.rul:1389,1475 comment without preceding space
scripts_4x4_40k.rul, Crafts_40k.rul, alienDeployments_40k.rul: flow style trailing whitespaces
TWoTS:
Polynesia.nam in 3 folders
en-US.yml (and other language files) comment without preceding space. Use regex [^ \r\n]# to find the instances
Any other important mods?
-
Here's another profiling run, this time just of saving and loading game. Release build, load and save a 12MB .sav file 10 times:
Before:
(https://i.postimg.cc/BncSRYcx/Screenshot-2024-11-10-215800.png)
(https://i.postimg.cc/ydW7gVqP/Screenshot-2024-11-10-215833.png)
On average, 2742ms to load and 3074ms to save
After:
(https://i.postimg.cc/s2TVgMLx/Screenshot-2024-11-10-214826.png)
(https://i.postimg.cc/6QWWKrYF/Screenshot-2024-11-10-215211.png)
On average 71ms to load (39x faster) and 19ms to save (162x faster)
Note that I have relatively beefy desktop PC. And average player would probably get much slower times. Still, I couldn't do a Debug build test because of an out of memory crash with a 12MB .sav file hehe. Anyway, I think I tested it as well as I could. Anything else I should work on as far as this migration goes?
-
make it "stable" and try find all possible bugs, save/load is relative easy to check as you can check round-trip, lot harder is check if one of rulesets properties was not skip (like type during refactor/copy paste error). Probably closest thing would be make dump of "stat for nerds" to text file and compare result of before and after. But this is outside scope of your work and probably would not catch all possible bugs as its not used for all possible rulesets.
After your refactor, I plan with Meridian change some sub-properties that now have "override-all" behavior that is not intuitive for modders.
-
I know that I didn't accidentally duplicate or miss anything, because I looked at all the changes with a diff app, so if some lines were missing or added, it would be easy to spot in the differences, and I went through all the differences at least twice.
I often used tight regex to replace, because regex doesn't make errors. And even when I used regex, I never used "replace all", but manually checked each match to make sure that replacement was correctly applied.
I also never used F2 to rename variable names. I intentionally named our variables "reader" and "writer", so if I forgot something, I'd get error because "node" variable didn't exist.
I'm not sure what you mean with "stable". OXCE never randomly crashed for me. However, I can't be sure because I don't have linux or other OS's to test on...
I wish that our RuleX classes also had save() methods, that way testing would be a lot easier since I could test round trips with those classes as well. Should I try writing save() functions for all these classes? Wouldn't that be too much work? Hmm, stats for nerds dump? Is that possible? I was looking for a way to dump the state of all objects to a file, but I couldn't find anything. Everyone says to write a serializer for your objects...
I'm also not sure what you mean with "override-all" behavior. What isn't intuitive?
-
"stable" in sense that work correctly and in expected way.
Should I try writing save() functions
No, this is probably too much work.
For dump, I mean SfN had already lot of code to prepare textual presentation of most of code use `std::ostringstream &ss`.
But this too would need some work.
I'm also not sure what you mean with "override-all" behavior. What isn't intuitive?
https://openxcom.org/forum/index.php?topic=12317.msg167703#msg167703
-
Anything else I should work on as far as this migration goes?
Next steps (not necessarily in this order):
0. Delian: test the ziploader, and the embedded loader (=common/standard resources embedded in the executable directly)... not sure if embedded can be tested on Windows or only on Android (as far as I know nobody is using this, but we should test it anyway)
1. Meridian(+Delian): test compilation on all supported platforms -- I may need help merging code and/or updating compilation scripts
2. Meridian: style review (just some OCD stuff, nothing too important)
3. Meridian: manual completeness review (manually check source code if something was forgotten during refactoring, etc.)
4. Meridian: very quick load/save/startup tests on all supported platforms
5. Meridian: create a documentation post with incompatibilities and migration guide for modders
6. Meridian+Yankes: decide, which incompatibilities we want to fix and which not (I will tend to do as few RYML code changes as possible)
7. Yankes: actual functional and architectural review (I am just a hobby-programmer these days, so I can't do that), Yankes needs to review, because he will be the one supporting it in the future
8. Meridian: create a new HowToCompileOXCE thread with updated instructions for OXCE 8.0+
9. Meridian: prepare a preview build for modders to update their mods to OXCE 8.0, potentially help with PRs to FMP, XCF and other mods on github
10. all: merry Xmas
-
@Delian
I see you go with local `libs` approach in distributing rapid, question is if this "pristine" version of rapid?
I recall that you mentions some corner cases that was not handled correctly by it, did you fix in its source code?
Another question, what exact version of rapid was used?
Right now I have plan to tweak your commits a bit.
First I will split main commit in two, one that add lib itself, and second that update game logic.
If there was some fix to lib, I would add it in separated commit to not mix "pristine" version with our fixed one.
-
The rapidyaml source files in my commit are the original unmodified files from github, the latest 0.7.2 version. After Meridian said to avoid custom fixes, I didn't use modified rapidyaml anymore, so all the testing I did on the mods was with the original src files. As mentioned before, I copied them from 3 places:
rapidyaml/src/
rapidyaml/ext/ -> c4core/src/c4/
rapidyaml/ext/ -> c4code/src/c4/ext/ -> debugbreak/debugbreak.h
You can modify the file structure, function names, commits, whatever you want hehe. Hmm, in the VS project, it would probably be better if the rapidyaml folder was inside the Engine folder.
Oh, there's a couple of old unrelated commits in my repository which you must NOT include. The changes to src/Geoscape/ConfirmDestinationState.cpp and src/Geoscape/CraftNotEnoughPilotsState.cpp . You should discard those.
Btw, guess what I've been doing all day long!
-
So what I did was, I used WinDbg to debug OXCE, break at a certain point in the Mod.cpp code (before AfterLoad calls, to avoid too much recursion), and then use WinDbg's command line to dump Mod object. I did a 7-level recursive traverse (with Natvis, because std::map and std::vector's data structures are otherwise too deep to dump), output to a file, which generated a 1.8GB 18M line text file. I did this both with the yaml-cpp and rapidyaml, which took a couple of hours each. I then edited the output files, censored pointer addresses and structure offsets, and compared the results. I found the following differences:
_mod->_scriptGlobal->_refList[xx].value.data
_Val and _Pad are different. Are these just pointers that randomly change on each startup?
_mod->_scriptGlobal->_events[xx][yy]._proc
Some bytes change from one run to the other. Is this because the compiled scripts contain pointers, and these changed bytes are just pointers that always change on each startup?
_mod->_globe._textures[xx].second._terrain[xx]
Some values (of type double) are different... well, this is complicated.
As we know, our rules allow mods to add on top of one another. Two mods can define the same rule, where properties of the 2nd definition add to, or overwrite properties of the 1st definition.
rapidyaml, coincidentally, happens to match behavior. If we try to deserialize a yaml sequence into an existing std::vector, each yaml sequence element will be deserialized into an existing vector element, updating the fields.
yaml-cpp however, clears the std::vector before trying to deserialize into it.
So this difference is (probably a bug) caused by:
- Dio not deleting existing globe textures before updating them with his own.
- rapidyaml in-place deseralizing into existing std::vector
I'm not sure which behavior is correct. For now I've disabled rapidyaml's support for std::map and std::vector and copied the implementation into our Yaml.h, where I modified it to clear vector/map before deserializing into them. This replicates the yaml-cpp's behavior, and while it is now backwards-compatible, one could argue that the behavior is incorrect.
_mod->_transparencies[xx].second[yy]
This one was my fault. In old Mod.cpp line 2543 the for loop was increasing two counters at the same time and I missed the 2nd one. Fixed, tested, and now it works.
_mod->_invs[xx].second._costs[xx].second
Similar problem like with the std::vector, except this time it's a std::map. But it's worse. If rapidyaml tries to deserialize into an existing std::map, which isn't empty, the deserialization for the elements with matching keys quietly fails. The previous solution of clearing the map before deserializing into it solved the problem. I've also reported the inconsistency issue between vector and map to library author.
_mod->_armors[xx].second._moveCostFlyWalk.TimePercent
This one was also my fault. In the ArmorMoveCost::load() I put in UInt8 as deserialize type. I don't know how that happened. Changed it to int and problem fixed.
YAML::Node changed to YAML::YamlString
As expected, this causes some changes in the memory layout.
There was also another problem related to deserializing into map, but I fixed it in the custom map deserialize method.
I pushed the fixes to the remote repo.
-
_Val and _Pad are different. Are these just pointers that randomly change on each startup?
I do not know what `_Val` or `_Pad` is. As name suggest this is c++ lib members, ask them what its means.
But for whole `_scriptGlobal`, yes it contains lot of self referencing pointers that are different on each load.
yaml-cpp however, clears the std::vector before trying to deserialize into it.
You need preserve this behavior, this is how near all list/maps work as default.
This is why I created all `loadNames` and other similar functions to override this behavior.
We can't change as it will break near every mod.
If you add custom function that clear vector before load, add `!add` and `!info` tag support and allow
preserving original vector when someone use `!add` tag on this node.
I don't know how that happened. Changed it to int and problem fixed.
`char` happens, this is not numeric type but characters, I would even force all `char` or `unsigned char` to load as `int`.
btw I added some comments to your repo do you look on them?
-
Yeah, some ScriptRawMemory = std::aligned_storage internals. Scripts work fine, so it's probably nothing.
>add `!add` and `!info` tag support
That would be new functionality that isn't a part of this migration hehe
I've looked through the comments and answered them all. Fixed what was recommended. I also found and fixed another bug which I missed yesterday. Dumping the whole Mod object was a really good idea haha.
Next I'll test the ziploader. And I'll see if I can get embedded loader to work.
-
Yeah, some ScriptRawMemory = std::aligned_storage internals. Scripts work fine, so it's probably nothing.
ok, this is "container" that sometime have text pointers or `int` values
>add `!add` and `!info` tag support
That would be new functionality that isn't a part of this migration hehe
ok
I've looked through the comments and answered them all. Fixed what was recommended. I also found and fixed another bug which I missed yesterday. Dumping the whole Mod object was a really good idea haha.
Next I'll test the ziploader. And I'll see if I can get embedded loader to work.
I added one critical response about `untilLastIf` and some less critical for `std::vector` vs `[]`.
And probably in this week if I will have more time I will look again on this code.
-
some less critical for `std::vector` vs `[]`.
Have you measured how many nanoseconds this saved? :D
I have 10 cores, but they're all lazy drunks :)
I think I figured out why your build times are so slow.
When I build the project (24 core threads), it takes 45s
(https://i.postimg.cc/KzmWfRDX/Screenshot-2024-11-14-220814.png)
But if I tab out during the building, it takes over 2min
(https://i.postimg.cc/ZYGMs7cF/Screenshot-2024-11-14-220528.png)
What's causing the throttling to 33% of total CPU when VS window isn't active?
It's Windows. And the solution for me (Windows 11) was to go to Settings -> System -> Power, and changing the "Power Mode" from "Balanced" to "Best Performance". Now building remains fast for me even if I tab out.
Edit: If you do that, also make sure that in VS you go to Tools > Options > Projects and Solutions > Build and Run and turn on "Run build at low process priority" option, that way foreground tasks won't stutter even if building in the background is using 100% of CPU.
-
Have you measured how many nanoseconds this saved? :D
I do not know if one allocation can be measured but after year of changes like this game will be
faster by 0.5% :) Effective opposite of "Death by a Thousand Cuts". This is more a discipline that
direct optimization. Evert time I see some allocation I ask myself "do I need it?".
Author of `yaml-cpp` did not ask this question and result is apparent.
Another example of lack of "disciple" was my use of `std::stringstream` as "other parts of game use this too and this will be fine",
but as you show it was no fine when it take 4% of whole time.
-
Oh, that's minor if we compare it to the time you're wasting by calling std::sort() 42785 times, in the ModScript constructor.
(https://i.postimg.cc/MK53QDQb/Screenshot-2024-11-15-111541.png)
Talk about a death by a thousand cuts hehe
(that makes up about 15-20% of the rules loading time)
-
ok, but this is not allocation :D
And its not "wasting" as each operation add new element to "flat map", but indeed as this is "hot" path I can improve this.
Overall its impossible to replace this `std::vector` by `std::unordered_multimap` as I many times need rage of values using partial search by prefix.
Right now I can try use `insert` & `partition_point` to avoid sorting whole vector and move only elements only when I find correct spot.
Another more invasive fix is move sorting after all elements where added, but this would need update script "framework" to catch place where is safe
to add elements.
-
I many times need range of values using partial search by prefix.
Sounds like you should be using an Adaptive Radix Tree (an advanced form of Trie - data structure specifically designed for string prefix searches)). Too bad it's not among standard containers hehe.
Alternatively, you could use a hashmap of hashmaps, where the key is prefix, and value is a hashmap of entries that fall under that prefix.
But yeah, normally the sorting should only happen after all the mods and rules are loaded, so sorting only happens once.
0. Delian: test the ziploader, and the embedded loader (=common/standard resources embedded in the executable directly)
For now I've tested the ziploader, or at least I think that was the ziploader.
UFO.zip and TFTD.zip in the exe folder or user folder works fine.
common.zip and standard.zip in exe folder works.
common.zip and standard.zip in user folder doesn't work.
common.zip in user folder and standard.zip in user/mods folder works.
piratez.zip in user/mods folder works.
The only problem was that, if I used piratez.zip, the process would consume 1000MB of RAM instead of 600MB, which could be bad.
As for the embedded loader, I'm still trying to figure it out.
-
Sounds like you should be using an Adaptive Radix Tree (an advanced form of Trie - data structure specifically designed for string prefix searches)). Too bad it's not among standard containers hehe.
Alternatively, you could use a hashmap of hashmaps, where the key is prefix, and value is a hashmap of entries that fall under that prefix.
But yeah, normally the sorting should only happen after all the mods and rules are loaded, so sorting only happens once.
I did check my simple fix and it look it work fine, its reduce time around by 90%. Technically this was only change `O(n * log n)` to `O(n)` but cache
friendlies make most of work probably.
You can check how big improvements it was:
https://github.com/Yankes/OpenXcom/commit/02604ecb9b7fb07472ff50facd5a6068f6559b25
https://github.com/Yankes/OpenXcom/commit/784af7587fc9c7a3accf8f5ab86e1ce2b3662361 (cleanup commit)
https://github.com/Yankes/OpenXcom/commit/e490a469dcfdf14b81f8c5edd3df9b69e27d1492
-
>O(n * log n)` to `O(n)
I think partition_point uses binary search, therefore it's O(n*log(n)) to O(log(n))
But yeah, it's much faster now. I can confirm that not calling std::sort() 42785 times makes a big difference! Another 15% faster startup time.
At this point, I don't see any real way to improve the startup anymore. In Release build, half of the startup time is consumed by Sound::load(); sdl_mixer.dll issues I suppose.
In Debug build, another 20% (>2s) speedup would be possible if RuleStatBonus::load() wasn't attaching a script to everything. Basically, 4000 items and armors, each attaching 6 scripts for the stat multipliers... it's a lot of script parsing.
-
>O(n * log n)` to `O(n)
I think partition_point uses binary search, therefore it's O(n*log(n)) to O(log(n))
But yeah, it's much faster now. I can confirm that not calling std::sort() 42785 times makes a big difference! Another 15% faster startup time.
At this point, I don't see any real way to improve the startup anymore. In Release build, half of the startup time is consumed by Sound::load(); sdl_mixer.dll issues I suppose.
In Debug build, another 20% (>2s) speedup would be possible if RuleStatBonus::load() wasn't attaching a script to everything. Basically, 4000 items and armors, each attaching 6 scripts for the stat multipliers... it's a lot of script parsing.
Is `O(n)` because I do `inseter` and this dwarf search complexity (as I could try push objects in reverse order in worst case).
This `RuleStatBonus` is simply cost of "robustness", we need pay somewhere for this, like make code lot more complex, drop some functionalists etc.
Only thing I see that is bit redundant is conversion of old node format to text representation and then parsing it but it would need new script representation.
Alterative all default scripts could share data in some way.
-
Either share data, or have 1 global script that handles all RuleStatBonus, or have multiple global scripts, one for each type of stat. Or something. I don't know.
Or maybe, have an unordered_map cache of <script text, compiled script>. So if a script was already compiled, you don't need to compile it again, just copy the compiled script from the cache? And clear the cache after the loading is finished.
Edit: I just tested it, and it seems to work pretty well. RuleStatBonus::load() goes from 2050ms down to 320ms if I use a cache of compiled scripts.
-
did you copy vectors content? this is not right as some operations refer to original memory in that vector.
-
If the script contains pointers, then obviously copying the compiled script isn't right. But as far as I can tell, the scripts generated in RuleStatBonus::load() don't contain any pointers. But let me check...
...nope, I don't see any pointers. Or at least none are dynamic. To check, I stored all the compiled scripts and compared the scripts with the same script text. And the conclusion is that all the scripts with the same script text always generated the same vector of bytes (in the _container._current._proc). For instance, all the scripts with the text:
mul bonus 1000; unit.firingBonusStats bonus 1000000 0 0 0; if ge bonus 0; add bonus 500; else; sub bonus 500; end; div bonus 1000; return bonus;
generated the same 87 bytes:
[13 00 00 e8 03 00 00 72 92 17 42 00 04 00 00 00 40 42 0f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 06 00 00 00 00 00 00 3b 00 00 00 47 00 00 00 0f 00 00 f4 01 00 00 01 4e 00 00 00 11 00 00 f4 01 00 00 25 00 00 e8 03 00 00 00 00]
-
add debug text and it will not be true anymore. Overall optimization like this is ticking bomb as idea was scripts "proc buffer" is not copyable.
Changing this will hinder my future plans like adding const arrays that will be stored at end of this "proc buffer".
For me better solution would be change "owning" model of scripts that instead of vector they store only pointer to final "proc buffer" that is stored in common
object, similar how whole "global scripts" share data. Probably with some simple recounting.
Some pseudo code:
std::map<std::string, std::shared_ptr<const std::vector<Uint8>>> _cache;
auto c = _cache.at("script ...");
std::shared_ptr<const Uint8[]> _proc(c, c->data()); // only one indirection to access "proc buffer"
_cache.clear(); // drop all not used vectors after load
-
>add debug text and it will not be true anymore
I dumped the whole ModScriptGlobal and its internal state before and after the calls to RuleStatBonus._container.load() was the same. The only change in memory were the _proc vectors. Am I misunderstanding something? What is not true?
>scripts "proc buffer" is not copyable
Haven't you heard of self-modifying code? :p
>change "owning" model of scripts
That might reduce script memory usage. But would it reduce the CPU time needed for parsing?
My point is that I would only add caching for the scripts generated in RuleStatBonus::load(), because these scripts are simple (probably because they only read and write from CPU registers). Hmm. Maybe there exists a more general way to determine when scripts are simple? You could include a "simplicity" bool in the cache. So you can ask the cache: Is this script text a simple script? Yes -> then copy bytecode from cache. No -> don't copy the bytecode, just parse normally.
-
Please don't lose focus.
Let's return to yaml lib migration.
This optimisation can be done separately.
-
Please don't lose focus.
Let's return to yaml lib migration.
This optimisation can be done separately.
True, before scripts was around 3% of total load time and any improvement would be barely visible and now its only "problem" because everything else is running a lot faster. We should deliver this "faster" version before we try deliver "fastest" one.
>add debug text and it will not be true anymore
I dumped the whole ModScriptGlobal and its internal state before and after the calls to RuleStatBonus._container.load() was the same. The only change in memory were the _proc vectors. Am I misunderstanding something? What is not true?
>scripts "proc buffer" is not copyable
Haven't you heard of self-modifying code? :p
>change "owning" model of scripts
That might reduce script memory usage. But would it reduce the CPU time needed for parsing?
My point is that I would only add caching for the scripts generated in RuleStatBonus::load(), because these scripts are simple (probably because they only read and write from CPU registers). Hmm. Maybe there exists a more general way to determine when scripts are simple? You could include a "simplicity" bool in the cache. So you can ask the cache: Is this script text a simple script? Yes -> then copy bytecode from cache. No -> don't copy the bytecode, just parse normally.
when I mean debug script I mean `debug_log "Test" x;` code like this can easy happens when modder try override script or I add some debug lines to test default scripts. This is "footgun", it will fork fine for years but when someone forget it every thing will explode. I prefer that is physically impossible to make error like this.
Yes, adding some additional flags could be solution but is more a bandaid than fixing underling problem, this mean there is multiple scripts that are duplication of each other, my solution is simply represent this reality in script data itself by using `std::shared_ptr`.
In theory it should be cheaper as we pay only for atomic counter increment and we skip most of coping and memory allocations.
but anyway, as Meridian point out, this is not point of this "task", we should focus on yaml itself and deliver this first. when this is in hand of players then we can make it even faster.
-
Please don't lose focus.
Let's return to yaml lib migration.
This optimisation can be done separately.
Ok, I've also tested the embedded loader now. It seems to work fine.
To enable embedding in a windows build:
1. In the Project properties, go to C/C++ > Preprocessor, and edit Preprocessor Definitions and add EMBED_ASSETS.
2. In the Project properties, go to Resources, and edit Preprocessor Definitions and add EMBED_ASSETS. Yes, it has to be defined in two places.
3. The zip files need to be at OpenXcom\common.zip and OpenXcom\standard.zip. So above the project dir.
This will generate an exe file that is 4MB larger than usual.
What's next? Note that I can only do testing and building on Windows.
Current list of incompatibilities:
If a yaml line ends with ] or }, the trailing spaces need to be removed.
" - a: [1, 2, 3] " is bad
" - a: { b: 2, c: 3 } " is bad
" - a: [1, 2, 3]" is good
" - a: { b: 2, c: 3 }" is good
" - a: [1, 2, 3] #comment" is good
If a yaml line contains any (trailing) tabs, they need to be removed
" - a: 1 " is bad
" - a: 1 #comment" is bad
" - a: 1" is good
" - a: 1 #comment" is good
If a yaml line ends with a comment, the comment needs a preceeding space
" - a: 1#comment" is bad
" - a: 1 #comment" is good
damageAlter: FireThreshold no longer accepts float values
" FireThreshold: 10.0" is bad
" FireThreshold: 10" is good
strings starting with a single or double quote require a closing quote
- 'Akamu bad
- Akamu good
- '''Akamu' good
- "'Akamu" good
-
What's next? Note that I can only do testing and building on Windows.
Thanks for the test, that's good news.
Now, I think it's up to me to test various builds and up to Yankes to continue review.
Give us some time, we're both rather busy with real life(tm).
-
There was a problem with parsing UTF-8 files with BOM. I had to make a workaround that skips the BOM if it encounters it. I guess it's a bug in rapidyaml. Issue reported! Anyway, X-Com Files seems to work fine now.
What was the issue exactly?
I tried removing your fix and load a file with BOM, but I didn't encounter any error.
PS: looks like ryml author started working on your reported issues
-
The BOM gets included in the key of the first element.
So if my options.cfg has a BOM, after parsing, the key of first element is not going to be:
mods
but
mods
So loading options will quietly fail because it will look for a "mods" key, but won't find it.
It's great that the author started working on those issues. But until he releases version 0.7.3 of rapidyaml, there's not much to do. And even after it's release, that will only fix one of the incompatibilities. A major one for sure, but there's no need to hold your breath hehe.
-
What's next? Note that I can only do testing and building on Windows.
I can test on various platforms.
But I have trouble understanding the instructions here: https://github.com/biojppm/rapidyaml?tab=readme-ov-file#using-ryml-in-your-project
I have practically zero experience with cmake and have no idea what to do.
Have you come across some sample project, which I could use as inspiration?
(our Linux, Macos and Android builds use cmake.)
In the meantime, I'll try the iOS build, which isn't based on cmake as far as I can tell.
-
Uhh... from what I can tell, all of those instructions are irrelevant. We don't use a single header file, and we aren't trying to amalgamate one. We aren't using the pre-built library. We aren't trying to make a library. We aren't using package managers to install anything, and aren't going to link the project in github (unless you want to). We aren't building a quickstart example, so the CMakeFiles.txt links don't matter. And we aren't using/changing any CMake variables because we're using default build options.
-
but we need use cmake to compile gcc version, btw I already took this task. and find some bugs like missing includes
(yaml-cpp brings some headers that now are missing).
One thing to you to fix could be reduce warning spam in GCC:
In file included from /home/Yankes/prog/openxcom/src/Ufopaedia/../Mod/RuleItem.h:23,
from /home/Yankes/prog/openxcom/src/Ufopaedia/ArticleState.h:21,
from /home/Yankes/prog/openxcom/src/Ufopaedia/Ufopaedia.h:24,
from /home/Yankes/prog/openxcom/src/Ufopaedia/UfopaediaSelectState.h:21,
from /home/Yankes/prog/openxcom/src/Ufopaedia/UfopaediaStartState.cpp:21:
/home/Yankes/prog/openxcom/src/Ufopaedia/../Mod/../Engine/Yaml.h:26: warning: ignoring ‘#pragma warning ’ [-Wunknown-pragmas]
26 | #pragma warning(push)
|
/home/Yankes/prog/openxcom/src/Ufopaedia/../Mod/../Engine/Yaml.h:27: warning: ignoring ‘#pragma warning ’ [-Wunknown-pragmas]
27 | #pragma warning(disable : 4668 6011 6255 6293 6386 26439 26495 26498 26819)
|
/home/Yankes/prog/openxcom/src/Ufopaedia/../Mod/../Engine/Yaml.h:30: warning: ignoring ‘#pragma warning ’ [-Wunknown-pragmas]
30 | #pragma warning(pop)
|
In file included from /home/Yankes/prog/openxcom/src/Ufopaedia/../Mod/../Engine/../../libs/rapidyaml/c4/yml/yml.hpp:11,
from /home/Yankes/prog/openxcom/src/Ufopaedia/../Mod/../Engine/../../libs/rapidyaml/ryml.hpp:4,
from /home/Yankes/prog/openxcom/src/Ufopaedia/../Mod/../Engine/Yaml.h:28:
/home/Yankes/prog/openxcom/libs/rapidyaml/c4/yml/parse.hpp:15:21: warning: redundant redeclaration of ‘c4::yml::id_type c4::yml::estimate_tree_capacity(c4::csubstr)’ in same scope [-Wredundant-decls]
15 | RYML_EXPORT id_type estimate_tree_capacity(csubstr src);
| ^~~~~~~~~~~~~~~~~~~~~~
In file included from /home/Yankes/prog/openxcom/src/Ufopaedia/../Mod/../Engine/../../libs/rapidyaml/c4/yml/yml.hpp:9:
/home/Yankes/prog/openxcom/libs/rapidyaml/c4/yml/parse_engine.hpp:767:21: note: previous declaration of ‘c4::yml::id_type c4::yml::estimate_tree_capacity(c4::csubstr)’
767 | RYML_EXPORT id_type estimate_tree_capacity(csubstr src);
| ^~~~~~~~~~~~~~~~~~~~~~
last one look like sloppy declaration in lib itself.
[ps]
some fixes:
https://github.com/Yankes/OpenXcom/tree/rapidyam
[ps2]
https://stackoverflow.com/a/55877109/1938348 probably best way to handle warnings like this in portable code.
-
So... Hedley?
Btw, there's one critical bug (https://github.com/Delian0/OpenXcom/commit/8a363cdc093d0a4d497552baa09939cb012d786a) which was fixed in the commits that I made after you made the fork.
-
ok,
btw after bulding new version, it crash on Xcom2:
[19-11-2024_15-36-13] [WARN] scanModDir('./', 'standard'): Bad metadata.yml xcom2, skipping.
Thread 14 "openxcom" received signal SIGABRT, Aborted.
call stack:
#7 0x00007ffa6690e12a in free (p=0xa00000000) at /usr/src/debug/cygwin-3.5.4-1/winsup/cygwin/mm/malloc_wrapper.cc:78
#8 0x00007ffa66927adb in _sigfe () at sigfe.s:36
#9 0x0000000100c50aae in c4::yml::Tree::~Tree() ()
#10 0x00000001007be7fa in OpenXcom::YAML::YamlRootNodeReader::~YamlRootNodeReader() ()
#11 0x00000001006a23de in OpenXcom::FileMap::scanModDir(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool) ()
#12 0x00000001006e5aa4 in OpenXcom::Options::refreshMods() ()
I did not yet spend time on analyzing it (nor fetch your last changes).
Do you know what could go wrong?
btw2.
I see you move rapid to `Engine`, I do not think is good move. This is not our code and probably better would be not mix it with our.
We only "bundle it" and someone should be able to use different version if he want (on his own responsibility :D), `libs` folder would in
my option best place for code like this.
-
>I do not think is good move
It's just for organizing files in Visual Studio. The actual files are still going to be in the ../libs/ folder. There are also other folders in the Engine folder that aren't our code. And other files lying unorganized in the top folder of the project. The logical location in the project doesn't have any significance other than for file organization. Basically, it shouldn't be mixed with folders in the top level, because that makes it harder to navigate the folders and files which we commonly access.
As for the crash... I'm not sure.
That's basically the 1st time game tries to read yaml. (normally the first time is trying to read options.cfg, but I'm assuming you don't have it created yet).
Reader is created, reads yaml. When it falls out of scope, it destroys itself and the tree, which holds buffers. But, it seems there was already a problem with parsing?
Parsing goes fine for me:
(https://i.postimg.cc/gjhhfWch/Screenshot-2024-11-19-191159.png)
The tree is created with 10 nodes and I don't get a bad metadata report. So the fact that it's reporting bad metadata, something must've gone wrong with the parsing... But if something went wrong, why didn't the parsing crash or report any errors. Weird.
Maybe you forgot to include some rapidyaml files in the build?
I'm assuming the abort() call came from rapidyaml\c4\ext\sg14\inplace_function.h:91, where it would normally throw a std::bad_function_call() exception.
But that call probably came because ~Tree destructor tried to call a "free" function, defined through a macro. Perhaps poorly defined, because it tried to call an non-existent free function.
-
ok, if this move is only in VS then this is fine, probably I jump gun too fast :D
for bug, it could be some combination of different effects, I use cygwin as my env, this mix win with linux and some could conflict.
Probably then this is bug purely on my side then.
-
Just a small update, MacOS build was successful, although with many warnings.
-
Just a small update, MacOS build was successful, although with many warnings.
Question is, does it run hehe.
Probably then this is bug purely on my side then.
No, rapidyaml allows you to provide it with your own "malloc" and "free" callback, so that's what I'm trying to do now.
...
Ok, try with this https://github.com/Delian0/OpenXcom/commit/452522af25541513a9bf26caf9d0f6e7551b4e46
-
[20-11-2024_01-50-14] [FATAL] YamlRootNodeReader(char* data, size_t size, std::string fileNameForError)
[20-11-2024_01-50-14] [FATAL] ~YamlRootNodeReader
[20-11-2024_01-50-14] [WARN] scanModDir('./', 'standard'): Bad metadata.yml xcom2, skipping.
[20-11-2024_01-50-14] [FATAL] ~YamlRootNodeReader
I think we hit bug with compiler, as far I know this code should not compilable because `YamlRootNodeReader` is not copyable type.
Later version of C++ there is concept for NRVO, but type need have at least move constructor.
Its look like MSVC apply NRVO but GCC try but forget to perform NRVO, because of this code call destructor on some garbage memory.
Tomorrow I will see how is easiest way to fix it.
-
Does memory allocation and freeing work ok now? If you add breakpoints inside s_allocate and s_free, do they get called?
Ok, how about this. Instead of
const auto& reader = frec->getYAML();
try
YAML::YamlRootNodeReader reader(frec->fullpath);
The code does exactly the same, but without any movement through return values.
I didn't want to add a move constructor to those classes because that allowed me to spot if any copy/move accidentally took place.
The problem is, there's a lot of other places in the code that have a YamlNodeReader as a return value. So if Copy elision doesn't work, I'd most likely have to add a move constructor to all the classes. Maybe you can add a flag to your compiler which applies move elision more aggressively?
-
Another update, the Android build was not successful.
What I did so far:
1. merged main branch into android branch -- that was actually pretty easy, only a few merge conflicts
2. updated cmake in the android project -- see screenshot 1
3. build failed -- see screnshot 2
4. I looked at the source code line indicated in the error message, but don't really know what to do -- see screenshot 3
Trying to solve it with Yankes atm.
-
Does memory allocation and freeing work ok now? If you add breakpoints inside s_allocate and s_free, do they get called?
Ok, how about this. Instead of
const auto& reader = frec->getYAML();
try
YAML::YamlRootNodeReader reader(frec->fullpath);
The code does exactly the same, but without any movement through return values.
I didn't want to add a move constructor to those classes because that allowed me to spot if any copy/move accidentally took place.
The problem is, there's a lot of other places in the code that have a YamlNodeReader as a return value. So if Copy elision doesn't work, I'd most likely have to add a move constructor to all the classes. Maybe you can add a flag to your compiler which applies move elision more aggressively?
this is not problem with memory allocation, this is pure compiler clobbering return value object, second destructor is called on garbage where should be returned object. I did not make minimal test case to say what go wrong. IMHO compiler should return error ("you try return unmovable type") instead of creating UB.
One solution is not try use NRVO but RVO that allow return anything when using`return X{ };` and we can make your code work this way.
Another thing is add fake constructor. because NRVO is allowed only when you have one, but whole point is that is never called, this mean never linked too.
This mean if you have only declaration but not definition (aka no body) then every thing will work, but if it try copy object, linking will fail.
I checkout this more today. Maybe I will find some easy solution.
-
@Yankes
https://github.com/Delian0/OpenXcom/commit/e88396630c8a8d596f906faef42413b4062033c7
Meh, I just added all the missing constructors. Some throw exceptions tho ::)
@Meridian
Researching...
...it seems rapidyaml is trying to forward declare certain std types (like std::string) for some reason. But the macro logic isn't able to determine which standard library it is.
Edit:
"Android NDK uses LLVM's libc++, with support for _LIBCPP_ABI_NAMESPACE inline namespace. Older versions of the Android NDK supported GNU libstdc++, indicated by __GLIBCXX__ or __GLIBCPP__."
You shouldn't have gotten the "unknown standard library" error, because the conditions check for those specific macros. I don't get it...
-
@Yankes
https://github.com/Delian0/OpenXcom/commit/e88396630c8a8d596f906faef42413b4062033c7
Meh, I just added all the missing constructors. Some throw exceptions tho ::)
YamlNodeReader::YamlNodeReader(const YamlNodeReader&& other) noexceptthis is wrong signature for move constructor. `const&&` is the crazy uncle nobody talks about in C++ community :)
Proper signature do not have `const` as whole point of move constructor is rip out all internals of other object and leave carcass that only job is
to dispose gracefully.
overall I push some commits that fix bugs:
https://github.com/Yankes/OpenXcom/commits/rapidyam/
first one I reported, I simply fix by using proper RVO, this mean by code like:
return YAML::YamlRootNodeReader{data, size, fullpath};
and delete of `data` was moved to constructor.
This is not perfect solution as this "steal" this memory but as far I see this was only one use of this constructor.
Second fix was:
str = ryml::csubstr(data, str.find("\n---") + 1); // "safe" when file do not have "---", it will crop out whole file but it should be invalid anyway
your version had three problems:
`\r\n` and line like `foo: bar---` both would break save. Not to mention some random file that do not have any `---` in it.
My version handle all cases (in last one trim whole file but this probably will not be problem).
if you do not want some functions to exists, you can mark them as `= deleted;` when compiler would try call them, it will break compilation.
another thought, how much of runtime of loading files is spend on allocations? because we allocate buffer copy content to it and then read from it and delete it.
we could probably in many cases skip this, or at least reuse this memory.
I had helper class `class RawData` that could be good tool for handling memory like this, `YamlRootNodeReader` could even store it and use to implicate parsing.
As free benefits, your class would stop being copyable and only movable because of `std::unique_ptr`.
Another thing would be make all members of `YamlRootNodeReader` as values, not pointers. With this `Parse` would become delegated constructor.
[ps]
For default copy-construcotrs, is seems I miss remember this:
https://en.cppreference.com/w/cpp/language/copy_constructor#Implicitly-defined_copy_constructor
said this is only "deprecated" when you define destructor, this mean we need go "rule of 5" or "rule of 0"
https://en.cppreference.com/w/cpp/language/rule_of_three
-
Some more updates:
1. cross-compilation for windows in MXE was successful... but the game silently crashes without any errors on loading 'standard' mods, i.e. xcom1
http://108.61.157.159/versions/win/Test-Extended-8.0-37400f2d7-2024-11-20-win32.7z
2. linux appimage compilation was successful... but I can't test it yet, will test when I get back home next week
https://github.com/MeridianOXC/OpenXcom/actions/runs/11948727493
3. compilation for ios in xcode: I had some trial and error to do with project settings, but I managed to add rapidyaml and also set up header file search paths so that I don't get "file not found" errors. The compilation started, but failed somewhere in the middle on something that looks like linker errors? Don't know how to continue... see attached screenshot
-
As for next steps, I suggest going step by step, solving only one issue/build at a time.
We have VisualStudio build working.
Now I would recommend concentrating on the MXE build... which probably suffers from the same issue that Yankes found and is discussed above.
Then move to Linux native build, Linux appimage build, MacOS native build, Android build and finally iOS build. In that order.
-
@Yankes
>this is wrong signature for move constructor.
Well, I made them const so that I could more easily determine what was being copied, and what was being moved. That allowed me to find a few places in code which I fixed. But yeah, move constructor with const doesn't make sense.
>your version had three problems:
Nice catch. I should mention that, if a sav file is missing ---, it's corrupted, because the header contains critical data.
>I had helper class `class RawData`
I should probably have made readFileRaw and getYamlSaveHeaderRaw return RawData instead of char*.
YamlRootNodeReader currently copies that data to ryml::Tree's buffer (arena), so there's no need to keep that data.
I could make a version that would use parse_in_place instead of parse_in_arena, but then data would have to outlive YamlRootNodeReader instance, which means it would have to keep the data. It's not worth the effort, because it only makes parsing maybe 1% faster (skips 1 memory allocation + copy).
Anyway, yeah, unique_ptr with RawData would probably be a better solution instead of YamlRootNodeReader calling free itself. Probably also safer, because if there's an exception, the free doesn't get called.
Should I change stuff to use unique_ptr + RawData?
1. cross-compilation for windows in MXE was successful... but the game silently crashes without any errors on loading 'standard' mods, i.e. xcom1
(1da8.32e4): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
*** WARNING: Unable to verify timestamp for openxcom.exe
openxcom!ZN2c43yml4TreeD2Ev+0x20:
00007ff7`570540a0 ff5350 call qword ptr [rbx+50h] ds:000002ad`1cf7dd10=feeefeeefeeefeee
It looks like the same issue that Yankes had and was fixed (https://github.com/Delian0/OpenXcom/commit/452522af25541513a9bf26caf9d0f6e7551b4e46). The issue with malloc and free.
But, it could also be the same problem with failed NRVO, which he also fixed (https://github.com/Yankes/OpenXcom/commit/929a446138ff8e318b91b59836fe2f7834b3e702) yesterday.
>3. compilation for ios in xcode
Based on those linker errors my best guess is that the rapidyaml cpp files aren't being compiled. Does a build log contain any rapidyaml hpp file compilation?
It could also be that the #include in Yaml.h (#include "../../libs/rapidyaml/ryml.hpp") is wrong, if the hpp files aren't at the right place.
-
I can report that the linux appimage crashes immediately at start (I was using XCF master mod):
[21-11-2024_12-45-01] [INFO] Scanning standard mods in '/tmp/.mount_oxce_gOsHz9A/usr/bin//../share/openxcom/'...
[21-11-2024_12-45-01] [FATAL] A fatal error has occurred: Segmentation fault.
[21-11-2024_12-45-01] [FATAL] ./oxce_git_2024_11_21_0839-x86_64.AppImage(OpenXcom::CrossPlatform::stackTrace(void*)+0x3d) [0x557fb310ccfd]
[21-11-2024_12-45-01] [FATAL] ./oxce_git_2024_11_21_0839-x86_64.AppImage(OpenXcom::CrossPlatform::crashDump(void*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)+0x3fa) [0x557fb310d6ea]
[21-11-2024_12-45-01] [FATAL] ./oxce_git_2024_11_21_0839-x86_64.AppImage(signalLogger(int)+0x45) [0x557fb2ebb915]
[21-11-2024_12-45-01] [FATAL] /lib64/libc.so.6(+0x3e730) [0x7f306883e730]
[21-11-2024_12-45-01] [FATAL] ./oxce_git_2024_11_21_0839-x86_64.AppImage(OpenXcom::YAML::YamlNodeReader::isMap() const+0x16) [0x557fb32527a6]
[21-11-2024_12-45-01] [FATAL] ./oxce_git_2024_11_21_0839-x86_64.AppImage(OpenXcom::FileMap::scanModDir(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool)+0xb2d) [0x557fb311f3ed]
[21-11-2024_12-45-01] [FATAL] ./oxce_git_2024_11_21_0839-x86_64.AppImage(OpenXcom::Options::refreshMods()+0x8e5) [0x557fb316cb25]
[21-11-2024_12-45-01] [FATAL] ./oxce_git_2024_11_21_0839-x86_64.AppImage(OpenXcom::Options::updateMods()+0x103) [0x557fb316e0e3]
[21-11-2024_12-45-01] [FATAL] ./oxce_git_2024_11_21_0839-x86_64.AppImage(OpenXcom::StartState::load(void*)+0x221) [0x557fb336ae11]
[21-11-2024_12-45-01] [FATAL] /tmp/.mount_oxce_gOsHz9A/usr/bin/../lib/libSDL-1.2.so.0(+0x15f3c) [0x7f3068fc5f3c]
[21-11-2024_12-45-01] [FATAL] /tmp/.mount_oxce_gOsHz9A/usr/bin/../lib/libSDL-1.2.so.0(+0x55baf) [0x7f3069005baf]
[21-11-2024_12-45-01] [FATAL] /lib64/libc.so.6(+0x89d82) [0x7f3068889d82]
[21-11-2024_12-45-01] [FATAL] /lib64/libc.so.6(+0x10ee20) [0x7f306890ee20]
[21-11-2024_12-45-20] [FATAL] OpenXcom has crashed: Segmentation fault.
-
@Yankes
>this is wrong signature for move constructor.
Well, I made them const so that I could more easily determine what was being copied, and what was being moved. That allowed me to find a few places in code which I fixed. But yeah, move constructor with const doesn't make sense.
>your version had three problems:
Nice catch. I should mention that, if a sav file is missing ---, it's corrupted, because the header contains critical data.
>I had helper class `class RawData`
I should probably have made readFileRaw and getYamlSaveHeaderRaw return RawData instead of char*.
YamlRootNodeReader currently copies that data to ryml::Tree's buffer (arena), so there's no need to keep that data.
I could make a version that would use parse_in_place instead of parse_in_arena, but then data would have to outlive YamlRootNodeReader instance, which means it would have to keep the data. It's not worth the effort, because it only makes parsing maybe 1% faster (skips 1 memory allocation + copy).
Anyway, yeah, unique_ptr with RawData would probably be a better solution instead of YamlRootNodeReader calling free itself. Probably also safer, because if there's an exception, the free doesn't get called.
Should I change stuff to use unique_ptr + RawData?
> and what was being moved
is this constructor even called? I recall that overload resolution always avoid this signature and `const&` is always preferred over `const&&`.
> Should I change stuff to use unique_ptr + RawData?
Yes, but RawData is unique_ptr + size. This means when you use one, you use both. For now we can replace `char*, size_t` args by `RawData`.
> because it only makes parsing maybe 1% faster
Ok, if we remove other allocations then it would rise to araund 4%? Probably too much changes for small profit. For now we can keep as is, maybe in future after we stabilize codebase we can improve things here (like script too).
-
>3. compilation for ios in xcode
Based on those linker errors my best guess is that the rapidyaml cpp files aren't being compiled. Does a build log contain any rapidyaml hpp file compilation?
It could also be that the #include in Yaml.h (#include "../../libs/rapidyaml/ryml.hpp") is wrong, if the hpp files aren't at the right place.
Yeah, it will be something like that.
I remember now, I had similar issues when miniz library was added.
I solved it by copying miniz sources to "src" (from "../../libs"), adding the new ones to the project and modifying includes to refer to new ones.
Ugly, but worked.
I tried the same hacks now with rapidyaml, but unfortunately it didn't work... there's too many includes to modify, the thing simply doesn't see itself.
I really don't get it how such a simple thing as adding files to a project can be so FUCKING complicated and unintuitive as in Apple XCode.
We'll need someone who understands XCode to do this... I don't have the nerves for this.
-
Last update for today.
iOS build is working (and game starts).
So, apparently, there are 2 kinds of folders in XCode projects.
Blue folders and yellow folders, I kid you not.
Blue is bad, yellow is good.
When adding folders via menu or via context menu, it creates blue folders without giving you any option.
So instead you need to use drag-and-drop, which gives you an option to create a blue or yellow folder.
Then just tell it to create a yellow folder and it will work.
Summary: Kids, don't do a career in IT. It will ruin your health, your sanity and your entire life.
-
Summary: Kids, don't do a career in IT. It will ruin your health, your sanity and your entire life.
"Apple software" not even once!
-
OpenXcom web app when?
So while trying to convert functions to use RawData I noticed something strange. In FileMap.cpp:58 we're calling mz_free() instead of SDL_free. But mz_free() is simply calling free().
Won't this fail on some platforms the same way rapidyaml failed? Meridian, do you remember having any crashes with handling zip files on other platforms in the past?
-
probably all this is finally `free` but if possible best would not mix them and keep symmetric between allocating and deallocating.
-
> and what was being moved
https://github.com/Delian0/OpenXcom/commit/a3088ba91a797d1f1d03252f88a926610d44efea and
https://github.com/Delian0/OpenXcom/commit/684e8e513bca4c50da82f16203a57aec29228627
>is this constructor even called? I recall that overload resolution always avoid this signature and `const&` is always preferred over `const&&`.
There is no "avoiding". const&& simply doesn't get called because it usually doesn't exist. So those calls get redirected to a copy constructor.
>For now we can replace `char*, size_t` args by `RawData`.
https://github.com/Delian0/OpenXcom/commit/af88e23bcc2bebd01a52ae19d7c62a486830734f
>if we remove other allocations then it would rise to araund 4%?
I dunno what allocations you're talking about. There's 1 allocation when yaml is copied to Tree's buffer. And there's 1 allocation (for node data) when I call _tree->reserve(yaml.len / 16); So during parsing there's 0 allocations lol (technically there are some for the newline data, for node locations in document)
But yeah, it's about 1% of the YamlRootNodeReader::Parse(). Or 0.15% of the whole startup process. You'll have to look elsewhere for performance gains hehe
-
> I dunno what allocations you're talking about. There's 1 allocation when yaml is copied to Tree's buffer. And there's 1 allocation (for node data)
I count at least 3 `new` there :P
-
You're... not wrong. But all those new's together don't add 1ms, because YamlRootNodeReader constructors are rarely called. Which is funny because I put them there to make class is smaller, but that doesn't matter because the class is never copied/moved.
-
Meridian, do you remember having any crashes with handling zip files on other platforms in the past?
Actually no, I don't remember any platform-specific issues with zip loader.
Stoddard tested quite extensively on win, linux and android before it was merged. And there were no macos or ios bugreports afaik.
-
Ok. Then the reason rapidyaml failed on some platforms was never because of malloc/free. It was other issues like compiler failing to apply NRVO, or failing to detect platform.
-
@Meridian
Researching...
...it seems rapidyaml is trying to forward declare certain std types (like std::string) for some reason. But the macro logic isn't able to determine which standard library it is.
Edit:
"Android NDK uses LLVM's libc++, with support for _LIBCPP_ABI_NAMESPACE inline namespace. Older versions of the Android NDK supported GNU libstdc++, indicated by __GLIBCXX__ or __GLIBCPP__."
You shouldn't have gotten the "unknown standard library" error, because the conditions check for those specific macros. I don't get it...
Don't know if it helps, but I am attaching two build logs:
1/ successful build log of OXCE 7.15 with yaml-cpp
2/ failed build log of new version with rapidyaml
There's still a good chance I didn't modify cmake script correctly, but right now I don't know what else to try.
(I wanted to try also a newer Android NDK/SDK, but upgrading doesn't look simple.)
-
Just as a test try changing:
rapidyaml\c4\std\string_fwd.hpp:31
from
#error "unknown standard library"
to
#include <string>
and rapidyaml\c4\std\vector_fwd.hpp:35
from
#error "unknown standard library"
to
#include <vector>
-
Yeah, that helped.
Compilation was successful and the game starts.
-
Well, that's annoying. It's the dreaded "custom library fix" that we were trying to avoid hehe.
It would probably be a good idea to make an issue about it on rapidyaml's git, but I'm not sure what to write in the issue. For a good report, I'd have to include info on the exact build environment, target environment, etc. And I'm not even sure if this is the correct fix, or it just coincidentally works (because our project already includes string/vector upstream of the build process).
The right solution would probably be to find out why, in an android build, _LIBCPP_ABI_NAMESPACE or __GLIBCXX__ or __GLIBCPP__ aren't being detected. If those aren't set, which macro or compiler variable should be checked for then?
I dunno. What should I do?
-
My idea was find what is exactly is in `<vector>` when we compile android.
This would answer what go wrong with this.
This could even give us fix for "upstream" to support this specific library in rapidyaml itself.
-
In one of the c4core issues (https://github.com/biojppm/c4core/issues/130) they get the same error, and they mention that _LIBCPP_ABI_NAMESPACE was only introduced with Clang 8.0.0-rc1.
You're using... NDK 18?
NDK r17 uses GCC
NDK r18 uses Clang 7.0.2
NDK r19 uses Clang 8.0.2
NDK r20 uses Clang 8.0.7
NDK r21 uses Clang 9.0.3
So, only NDK r18 is the issue because they don't use GCC anymore, and the _LIBCPP_ABI_NAMESPACE isn't introduced yet in Clang. How unlucky is that lol
-
yes, using Clang 7.0.2 on NDK r18b
NDK r19 wasn't compatible when we tried some years ago... project compiled, but the game just crashed.
I can try NDK r20, r21, maybe it's still relatively easy to upgrade...
-
I was able to use r19c, r20b and r21e.
(r22 and higher have some other structure and didn't work.)
All 3 versions I tried compiled without problems (and without any change in rapidyaml code).
However, none of them worked.
When I tried to run them on my phone (2-year old Samsung), all I got was a black screen :(
I'm pretty sure it's not a problem of those newer NDKs, or at least it is not caused only by them, because other people already compiled for example with r23b: https://github.com/MeridianOXC/openxcom-android/pull/3
...but until somebody gives me an idiot-proof guideline how to compile a working version with a newer SDK/NDK, I'm stuck with r18b, which is the only version that works for me.
"Custom library fix" is indeed annoying, but I'm willing to live with this one until there's a better solution.
-
Might avoid the custom library fix if you just manually defined _LIBCPP_ABI_NAMESPACE in one of our files in the android branch.
But yeah, sorry, I don't know much about building and testing android apps. If r22+ has a different structure, then how were they able to compile with r23b? Also, have you tried running their build on your phone?
-
But yeah, sorry, I don't know much about building and testing android apps. If r22+ has a different structure, then how were they able to compile with r23b? Also, have you tried running their build on your phone?
They used a different SDK and modified the build files too, and also used a newer build plugin.
(might have even built it in Android Studio directly, instead of console, not sure)
I don't have their build result (APK file), so I cannot test it.
Eventually, I'll ask some colleague(s) from work to help me upgrade, but not now.
Ah, sorry, didn't read documentation carefully enough. As for making a suggestion, I can't, because I'm not a modder and it's not a QoL suggestion.
Congratulations, you've been added to the whitelist: https://openxcom.org/forum/index.php?topic=11631.msg166226#msg166226
Enjoy the new benefits (in moderation pls).
-
(https://i.postimg.cc/jjYtfm9j/welcomesurprise.png)
Maybe, after this, we'll be able to afford 1ms to always have "discovered" and "autoSales" vectors sorted in the sav files. It's been super annoying testing round trips due to randomization of those two sections on every save.
-
If a yaml line contains any (trailing) tabs, they need to be removed
" - a: 1 " is bad
" - a: 1 #comment" is bad
" - a: 1" is good
" - a: 1 #comment" is good
I don't seem to get errors like this. (VS build)
Has something changed? (I use your last commit 78eab29)
Which file/line in the xcom1 ruleset could I use as an example to add a trailing tab?
-
Hmm. Oh, it only applies to flow-style lines. Not "a: 1" but "{a: 1}" or "[1, 2]"
You can try changing options.cfg, first mapping from
- active: true
id: piratez
to
- {active: true, id: piratez} tab tab
Also, to mapping or sequence start:
mods: tab tab
- active: true
This will also crash. However, I don't think this occurred in any of the mods.
-
XCF, FMP and FMPE have now been updated to be compatible, we can use them for testing
-
I retested them, and they seem to be working fine.
In the mean time, besides the increase in saving and loading speed, my goal has also been to make the current debug build startup faster than the yaml-cpp release build startup.
130,011 ms for yaml-cpp debug build startup.
7,856 ms for yaml-cpp release build startup.
16,077 ms (-113,934) for rapidyaml debug build startup.
It's fast, but still 2x slower than the yaml-cpp release build. However, if we consider other extra optimizations that aren't related to rapidyaml...
14,533 ms (-1,544) by avoiding (https://github.com/Delian0/OpenXcom/commit/342489dad957ca1386d6c1ca5cd8f2bb08985dd2) use of stringstreams (https://github.com/Delian0/OpenXcom/compare/caf679a3..44be274c#diff-a017ec30c469cca218f1d05a885fc9fdd4cf145ea946e6e3879d46dfc58ba3bf)
12,408 ms (-2,125) with smarter (https://github.com/Delian0/OpenXcom/commit/6b3906dca2fa72e9e62d70e83c1fd8e61e7dc364) sorting in ModScript constructor
9,661 ms (-2,747) by using script cache to avoid re-compilation of of built-in scripts.
8,667 ms (-994) with smarter (https://github.com/Delian0/OpenXcom/commit/78eab29bc741e504229c5e8bb909d0cfc9859c47) sorting in Mod::sortLists
7,887 ms (-780) by setting Linker > General > Enable Incremental Linking to "No (/INCREMENTAL:NO)"
Only 30ms left to go! But can't find anything major anymore... let's see...
7,764 ms (-123) by optimizing (https://github.com/Delian0/OpenXcom/commit/888a992c16d9f5148089150ab51fdd29c7832ca7) SoldierNamePool::load (it's still not perfect tho, because it keeps loading one the same .nam files over and over)
Woo, I did it! And I didn't even have to bring out the big guns (https://en.cppreference.com/w/cpp/thread/thread/hardware_concurrency).
About the Linker thing, I dunno why we had that turned on. It doesn't make building any faster, only slows down performance by 10% and puffs up the .exe file by 30%. If I make a small modification to a .cpp file, then usually only this .obj file will have to be rebuilt. Any larger change will require the whole project to be rebuilt, with or without incremental building. Please tell me if you can demonstrate a type of code change where incremental building would reduce build time.
I haven't put the script caching code anywhere yet because I'm still testing it.
Oh yeah, another thing I wanted to mention. Game decompresses and stores all the sounds in RAM, bloating it from 390MB to 760MB. This happens even with Options::lazyLoadResources enabled. Is there any reason we need to keep all the sounds in RAM all the time?
-
XcomUtil_Statstrings/XcomUtil_Statstrings.rul:8:19 ERROR: Could not deserialize value to type <int>!
Hmm... fixed (https://github.com/Delian0/OpenXcom/commit/8750ff3fd36bb0f7ea4058b76f7a20c7756a32a1).
-
While trying to get ROSIGMA to work with rapidyaml I ran into a problem with skills creating some kind of stack smashing. A small test mod next to pictures + log is attached.
I am building a debug build for Linux using this branch https://github.com/Yankes/OpenXcom/commits/rapidyam/
which currently is on this commit https://github.com/Yankes/OpenXcom/commit/d77c769ff147a7d0069f51d4baf54936b4295373
(ignore the Brutal stuff in the log in the pictures, I have exchanged the standard/common folder)
-
Besides the trailing whitespaces and incorrect float values, ROSIGMA had the following unusual errors:
- incorrectly defined missionBountyItem (expected string, found a sequence). But, this was also not properly reported by our api. Fixed (https://github.com/Delian0/OpenXcom/commit/62f3ec9e46b88ae079ac063d42e99c325422adce), so that it detects when trying to deserialize a non-scalar into string or integral type.
- incorrectly defined craft marker (expected int, found a map). But, this was also not properly reported by out api. Also fixed.
Stack smashing! It turns out that hard casting an enum* to int* is not a good idea hehe. Because some of our enums are defined as char and unsigned char. Fixed (https://github.com/Delian0/OpenXcom/commit/db120c8a2f5424132d2c8643b5a41d39f598eb4d).
-
You probably are testing with the ROSIGMA version on mod.io. Meridian was already kind enough to create a pull request to fix the major part of it. The Repo is here (https://github.com/BeatAroundTheBuscher/ROSIGMA)
Also I can confirm that the stack smashing error is gone.
-
@Delian I made some cleanups of your branch.
Probably biggest change is that nodes are not copyable anymore, you need use explicit `.alias()` function that create copy.
I do not name it "copy" as it refer same rapid node.
I will probably spend another day on further cleanups and checks of code.
Any feedback welcome.
https://github.com/Yankes/OpenXcom/tree/stash/rapidyaml-rc0
-
You made commits in such a way that I can't see what you changed lol
Not sure what you did, but those unique pointers are causing a crash as soon as the first YamlRootNodeReader is about to be disposed...
Ah, the pointers were being disposed in the wrong order... Parser has to be disposed before EventHandler.
-
I will look on this tomorrow
-
I've gone through the changes.
1. To fix the crash, you need to swap the order of definitions of YamlRootNodeReader._eventHandler and YamlRootNodeReader._parser.
2. I don't like the name alias(). I think a better name would be .clone() or .baseClone()
3. Can you explain the point of this function? Why is it better for the base class to have a clone function, instead of derived class to have a function that returns the base class?
4. I can't measure any performance difference to how it was before. There's more .alias calls now, where before there was nothing (with implicit copying).
-
I've gone through the changes.
1. To fix the crash, you need to swap the order of definitions of YamlRootNodeReader._eventHandler and YamlRootNodeReader._parser.
2. I don't like the name alias(). I think a better name would be .clone() or .baseClone()
3. Can you explain the point of this function? Why is it better for the base class to have a clone function, instead of derived class to have a function that returns the base class?
4. I can't measure any performance difference to how it was before. There's more .alias calls now, where before there was nothing (with implicit copying).
Yup, I already push this change for this bug.
I do not use `.clone()` as this still would refer to same node in yaml memory,
I would assume opposite, that if I clone object then values would be independent new node, but they are not.
`YamlNode*` work more like `std::ref`. Overall I would prefer drop this function and use `YAML::YamlNodeWriter&`
but this conflict with `reader["foo"]` as this return value and can't use references.
Another thing to consider was that implicit copy constructor was broken, if you copy "indexed reader", then game would crash
as it would copy pointer to new object without clearing old pointer. Another thing was, should copy copy index too? How much it will cost?
Do we expect this cost? Because of this ".alias()" is explicit light weigh copy and I ban all implicit operations.
I would make it inline function, but I tried follow your style in this file where you separate implantation of every function you can.
-
Then both alias and clone are bad names hehe. In that case, it was better before, when only the Root (derived) class had the function, because it was unambiguous about what it does.
Would YamlNodeWriter&& work instead of YamlNodeWriter& ?
Nevermind. Even if we use save(YamlNodeWriter) without ref or pointer, copying almost never happens because of RVO. So for YamlNodeWriter, I think it's better to have implicit copy and move constructors instead of alias(). It makes the code look better.
For YamlNodeReader I think instead of alias() it's better to have explicit copy and move constructors where _index gets copied or moved. I think that's fine because 1. We're only calling the copy/move constructor in a few places, 2. Those places don't use an indexed reader anyway.
-
Possible issue regarding submod inheritance found by Mercury in Rosigma.
Rosigma modifies the startingbase rulesets found in the base 40k mod, however if the facility section of startingBase for a certain difficulty/faction is not modified, and therefore not included in Rosigmas ruleset, the base starts with no facilities. It seems that the facility section is not getting inherited properly in the submod. Not sure if its a intended change or not, but since its behaving differently than OXCE v7.15, thought it worth reporting anyways.
(https://cdn.discordapp.com/attachments/842089696775372839/1312745240955846666/image.png?ex=674d9cc7&is=674c4b47&hm=6f6cb7d60921a5eb93016aabc15b95379de8d483e0db0d8d585ae071bef31a54&)
Factions/difficulties where Rosigma has modifies the starting facilities seems to work as intended.
Here is a comparison of the relevant starting base sections from the 40k mod and Rosigma
40k:
startingBaseSuperhuman: #*** Imperial Guard
facilities:
- type: STR_ACCESS_LIFT
x: 2
y: 2
- type: STR_HANGAR
x: 2
y: 0
- type: STR_HANGAR
x: 0
y: 4
- type: STR_HANGAR
x: 4
y: 4
- type: STR_GUARD_BARRACKS
x: 3
y: 2
- type: STR_GENERAL_STORES
x: 2
y: 3
- type: STR_LABORATORY
x: 3
y: 3
- type: STR_WORKSHOP
x: 4
y: 3
- type: STR_SMALL_RADAR_SYSTEM
x: 1
y: 3
randomSoldiers:
STR_GUARDSM: 16
STR_COMMISSAR: 1
STR_GUARD_OFFICER: 1
crafts:
- type: STR_CHIMERA
id: 1
fuel: 1000
damage: 0
weapons:
- type: STR_CRAFT_MULTILASER_UC
ammo: 100
items:
STR_GRENADE: 4
STR_LASER_RIFLEG: 14
STR_LASGUN_CLIP: 14
STR_LONGLAS: 1
STR_LASGUN_CLIP_HOTSHOT: 1
STR_HEAVY_STUBBER: 1
STR_HEAVY_STUBBER_CLIP: 1
STR_PISTOL: 1
STR_PISTOL_CLIP: 2
STR_OFFICERS_SWORD: 1
- type: STR_VALKYRIE
id: 1
fuel: 1800
damage: 0
weapons:
- type: STR_STINGRAY
ammo: 6
- type: STR_CANNON_UC
ammo: 100
status: STR_READY
- type: STR_LIGHTNING_INTERCEPTOR
id: 1
fuel: 1000
damage: 0
weapons:
- type: STR_STINGRAY
ammo: 6
- type: STR_CANNON_UC
ammo: 100
status: STR_READY
items:
STR_AVALANCHE_LAUNCHER: 1
STR_AVALANCHE_MISSILES: 10
STR_CANNON: 2
STR_HWP_CANNON_SHELLS: 1
STR_GRENADE: 10
STR_KRAK_GRENADE: 10
STR_LASER_RIFLEG: 2
STR_LASGUN_CLIP: 10
STR_LONGLAS: 1
STR_LASGUN_CLIP_HOTSHOT: 5
STR_ROCKET_LAUNCHER: 1
STR_SMALL_ROCKET: 4
STR_SMOKE_GRENADE: 8
STR_HEAVY_STUBBER: 1
STR_HEAVY_STUBBER_CLIP: 4
STR_HEAVY_BOLTER_GUARD: 1
STR_AC_AP_AMMO: 4
STR_GUARD_ARMORS_MEDIC: 2
STR_PISTOL_CLIP: 3
STR_CHAINSWORD: 1
STR_LASER_PISTOLG: 1
STR_LASPISTOL_CLIP: 5
STR_KILLPOINT_TOKEN: 180
STR_STINGRAY_LAUNCHER: 1
STR_STINGRAY_MISSILES: 25
scientists: 10
engineers: 10
Rosigma:
startingBaseSuperhuman: #*** Imperial Guard
randomSoldiers:
# STR_GUARDSM: 16
STR_PILOT_GUARD: 3
# STR_COMMISSAR: 1
# STR_GUARD_OFFICER: 1
# STR_PENITENT_GUARD: 4
crafts:
#- type: STR_CHIMERA
#- type: STR_VALKYRIE
#- type: STR_LIGHTNING_INTERCEPTOR
items:
STR_AVALANCHE_LAUNCHER: 1
STR_AVALANCHE_MISSILES: 10
STR_STINGRAY_LAUNCHER: 2
STR_STINGRAY_MISSILES: 25
STR_CANNON: 2
STR_HWP_CANNON_SHELLS: 10
STR_KRAK_GRENADE: 10
STR_LASER_RIFLEG: 16
STR_LASGUN_CLIP: 24
STR_STUB_RIFLE: 2
STR_STUB_GUN_AMMO: 10
STR_MISSILE_LAUNCHER_GUARD: 1
STR_SMALL_ROCKET: 4
STR_SMOKE_GRENADE: 8
STR_HEAVY_STUBBER: 2
STR_HEAVY_STUBBER_CLIP: 5
# STR_HEAVY_BOLTER_GUARD: 1
# STR_AC_AP_AMMO: 6
STR_BOLTPISTOL_LIGHT_ULTRA: 1
STR_LIGHT_BOLTPISTOL_AMMO: 6
STR_CHAINSWORD: 1
STR_LASER_PISTOLG: 1
STR_LASPISTOL_CLIP: 5
STR_KILLPOINT_TOKEN: 180
# STR_ALIEN_HABITAT: 3
STR_OFFICERS_CHAIN_SWORD: 1
STR_OFFICER_COMMISSION: 1
scientists: 10
engineers: 10
The Chamber Militant (Experienced) difficulty seems to have the same issue.
I tested this with both of these forks:
https://github.com/Delian0/OpenXcom
https://github.com/MeridianOXC/OpenXcom/tree/test-rc0-fix
Versions of mods used:
Rosigma: https://codeberg.org/LeflairKunstler/ROSIGMA/src/commit/59c9e24e1161894de2504d52f1862b31e4209a86
40k: https://github.com/BeatAroundTheBuscher/OXCE_40k/tree/c3043bc82257cdfe7802e47668001edd53d7f16e
-
Yeah, the templates weren't being properly merged. Fixed (https://github.com/Delian0/OpenXcom/commit/892aaafad233f4c97dcc79515158c826eaddac8d).
-
Do we have guarantee that first string end with new line? overall would be better have function that explicitly say what we do here, like:
`x.prependYaml(y)`, another thing do we have guarantee that comment `//rapidyaml supports duplicate keys (the first key has priority)`?
It always start from beginning searching for given name? I recall our discussion about search that start from last find and is `O(1)` for "stable sorted" names.
This is still case or you use different solution for that?
For previous discussion: ok, lest go full "value types" for normal "nodes", this means you can implicitly copy and move them at will.
Tomorrow I will prepare another "RC" with this changes.
-
Yes, rapidyaml always emits \n at the end of output string.
Yes, when searching for a key, the first node with the key is always returned.
This should apply... ah damn, I thought I was already using emplace. Added a change to Yaml.cpp and squashed (https://github.com/Delian0/OpenXcom/commit/892aaafad233f4c97dcc79515158c826eaddac8d) the last commit. Yeah, returning the first node with the key should also apply to our indexed reader because unordered_map.emplace doesn't do anything if trying to insert a key that already exists.
The search that starts from last find was sadly never implemented... YamlNodeReader always searches from the beginning if there's no index.
-
Typo in BattleUnit.cpp:633 (https://github.com/Delian0/OpenXcom/blob/892aaafad233f4c97dcc79515158c826eaddac8d/src/Savegame/BattleUnit.cpp#L633) ("turrentType") makes HWPs lose turrets when reloading saved game.
Also, https://github.com/Yankes/OpenXcom/tree/stash/rapidyaml-rc0 (https://github.com/Yankes/OpenXcom/tree/stash/rapidyaml-rc0) is the only version that works for me, when I build with VS 2019. Others crash when attempting to read any metadata.yml with stack trace similar to one in psavola post about linux appimage (https://openxcom.org/forum/index.php?topic=12283.msg167998#msg167998) earlier.
[01-12-2024_19-50-36] [FATAL] A fatal error has occurred: Memory access violation.
[01-12-2024_19-50-36] [FATAL] 0x4b04f0 c4::yml::NodeType::is_map (node_type.hpp:166)
[01-12-2024_19-50-36] [FATAL] 0x4b0550 c4::yml::Tree::is_map (tree.hpp:407)
[01-12-2024_19-50-36] [FATAL] 0x6dd4c0 c4::yml::detail::RoNodeMethods<c4::yml::ConstNodeRef,c4::yml::ConstNodeRef>::is_map (node.hpp:239)
[01-12-2024_19-50-36] [FATAL] 0xa54b00 OpenXcom::YAML::YamlNodeReader::isSeq (Yaml.cpp:164)
[01-12-2024_19-50-36] [FATAL] 0x89dda0 OpenXcom::FileMap::scanModDir (FileMap.cpp:1144)
[01-12-2024_19-50-36] [FATAL] 0x913200 OpenXcom::Options::refreshMods (Options.cpp:863)
[01-12-2024_19-50-36] [FATAL] 0x914330 OpenXcom::Options::updateMods (Options.cpp:1024)
[01-12-2024_19-50-36] [FATAL] 0xbf5670 OpenXcom::StartState::load (StartState.cpp:310)
[01-12-2024_19-50-36] [FATAL] 0xf80c710 SDL_KillThread
[01-12-2024_19-50-36] [FATAL] 0xf80ca40 SDL_SemWaitTimeout
[01-12-2024_19-50-36] [FATAL] 0xf80ca40 SDL_SemWaitTimeout
[01-12-2024_19-50-36] [FATAL] 0x722f690 crt_at_quick_exit
[01-12-2024_19-50-36] [FATAL] 0x75e9342b BaseThreadInitThunk
[01-12-2024_19-50-36] [FATAL] 0x770e979f RtlInitializeExceptionChain
[01-12-2024_19-50-36] [FATAL] 0x770e979f RtlInitializeExceptionChain
upd: changing FileRecord::getYAML() from
YAML::YamlRootNodeReader reader(data, fullpath);
return reader;to
return YAML::YamlRootNodeReader(data, fullpath);fixed it. Witchcraft!
-
Someone has fat fingers! (I guess I made the typo at the start when I wasn't yet using regex to convert stuff)
Man, it's so great having all these people help with the testing.
-
`x.prependYaml(y)`
I realize that the last bugfix was a bit of a hack. So I made a proper bugfix (https://github.com/Delian0/OpenXcom/commit/61b228ed04300561ffcf811dbddd2ad4c6efc41d) with proper merging, so there's no duplicate keys. So in case we want to implement faster node search in the future, there won't be issues. It was a bit complicated because Rapidyaml already has a function that merges nodes, but I couldn't use it because it a deep merge, and we only wanted a shallow merge.
-
look lot better
-
I made the faster node search that remembers the child iterator. It does make loading large saves about 10% faster.
There's just one issue. I had to do this:
const_cast<YamlNodeReader*>(this)->_nextChildId = ...
That's a no-go, right?
-
`mutable`?
[ps]
btw, for now we should keep all speedups to after deploy. Stash this change for later.
for now we need weed out all bugs that could linger in new version.
I planed today prepare new RC but I have very busy day and I did not find enough time to sit down and prepare all changes.
-
That's it! I didn't know that keyword existed lol. And it's a perfect fit for this use case. Well, I should've made _index mutable as well, so I wouldn't need to make a copy of the reader every time I needed to make one. Ok, I'll leave that change for later then, together with script caching (https://github.com/Delian0/OpenXcom/commit/7b9c4869dc564afa0041c20d5a006ea4c071389c), which I put on a separate branch. Although, doesn't it make sense to try to cram in as many changes as possible before everything is retested? I mean, if everything has to be retested anyways...
Oh, I also merged everything except for the alias stuff from the rc0 branch to mine.
-
if too many changes will be done at once then multiple bugs can creep in at once. And in worse case make everything "work" but when you try fix some corner cases then all bugs will surface again in other places.
When we include biggest change and test it fully, then small optimization could be tested locally without worrying that big change could interfere with small one.
btw I do not think merging is good strategy in this case, as I make rebase of your changes, next RC1 will be another rebase (I will push changes on RC0 too but without rebase, for you to see what I changed). I do this mainly because I try with Meridian keep linear hisotry and atomic commits as this allow easy bisection of bugs (as we can find exactly what commit break some mods).
-
New version for testing: https://github.com/Yankes/OpenXcom/tree/stash/rapidyaml-rc1
I stashed all speed improvements and only keep basic yaml changes + clean warning.
@Delian
For now I renamed `sansRoot` to `toBase` as your name at least for me is not clear.
But more I think about this, do we need this for anything? Slicing is only dangerous
when we have polymorphic types but in our case this is not true, sliced or not behavior is same.
Do you have any case when implicit slice would break our code?
-
sans = without. For instance, Sans-serif Font is a font without serifs.
YamlRootNodeReader without Root = YamlNodeReader. Get it? You're just removing the word "Root".
I know it's not a standard programming term, but I thought it was funny...
I don't think slicing causes any crashes, but Visual Studio is throwing warnings whenever it happens. So, it's mostly just to make the compiler/intellisense happy.
I tested rc1 and no issues for me. I added some comments to the rc0/rc1 commits tho.
-
@Delian
Are you fine with current state of RC1? If yes, then Meridian will start preparing to create new 8.0 release with it.
All speedups and other improvements will go to nightly versions like 8.0.1, 8.0.2, ... and when all will be tested then 8.1 will be published.
-
Yeah. I'm not finding any issues that would be related to the migration itself. In other words, the parsing and round trips work, so it's fine and you can start.
-
After deeply analyzing code I find couple place that could improve overall performance, some places are iterate tree times to allocate vector two times,
other times iterations are `O(n^2)` (not large list but still 10 vs 100 it a bit). Thanks to blazing fast and effective rapidyaml this places are hard to notice.
But if we add some helper structures I guess that overall performance could be improved two times (like reduce some operations to `O(n)` and avoid cache misses).
This will be done after main change get into OXCE.
Change will look like:
To `YamlRootNodeReader` add two vectors:
`std::vector`with struct that have `childsStartsPtr` and `childsSize`
And another
`std::vector` with struct `nodeKey` and `nodeId`
Size of both vectors will be equal to size of whole node list.
First vector points to sub-lists of nodes in second vector, we gather all node children and put then in second vector.
Now when we ask for children we check this both vectors instead of navigating nodes itself.
this should change this effective change current `list<node>` to `vector<node*>`
and it should have better cache characteristics.
We could to sort map keys to have `O(log(n))` performance too instead of current `O(n)` (unless we use index).
I need make test if my idea will have real benefits but I expect good grains from approach like this.
-
Rapidyaml doesn't use list<node>. Internally it already uses vector<NodeData> (It's array*, same thing; NodeData = 72 byte struct), and "NodeId" is just offset in this array.
The cache-locality of this array is already great. It would be very difficult to improve it. You can try, but I think constructing your vector would take more time than the time gained from theoretically improved cache-locality. Sorting... that's worse than hashing, isn't it? That's why std::map never beats std::unordered_map.
I can give you some current timing data. Let's say game startup (debug build with XPZ; Options::mute = true) takes 5000ms
Of these 5000ms, operations related to rapidyaml take:
- 1300ms to parse raw yaml into node trees
- 333ms searching for node keys
- 263ms deserializing node values
- 87+35ms building+destroying unordered_map node key indexes
- 32+13ms building+destroying YamlNodeReader.children() vectors
- 20ms building+destroying YamlNodeReader objects
- other negligible stuff
Of the 333ms of node key searching there is:
- 220ms (10ms in release build) of indexed searching
- 72ms (4ms in release build) of unindexed searching (with stored child iterator optimization)
What about game loading? In the release build, loading a 12MB sav file, all the unindexed searching together takes <2ms.
If your proposed optimization is targeting unindexed key searching, well, there's very little left to gain there. The majority of search calls (3 to 1) are already indexed. At least for game startup. For game-loading, the majority is unindexed, however that's mostly (80%) inside BattleUnitKills::load(), where the stored child iterator optimization gives it O(1).
Btw, what are the places that you say you found where there's too much iteration?
-
Rapidyaml doesn't use list<node>. Internally it already uses vector<NodeData> (It's array*, same thing; NodeData = 72 byte struct), and "NodeId" is just offset in this array.
The cache-locality of this array is already great. It would be very difficult to improve it. You can try, but I think constructing your vector would take more time than the time gained from theoretically improved cache-locality. Sorting... that's worse than hashing, isn't it? That's why std::map never beats std::unordered_map.
I can give you some current timing data. Let's say game startup (debug build with XPZ; Options::mute = true) takes 5000ms
Of these 5000ms, operations related to rapidyaml take:
- 1300ms to parse raw yaml into node trees
- 333ms searching for node keys
- 263ms deserializing node values
- 87+35ms building+destroying unordered_map node key indexes
- 32+13ms building+destroying YamlNodeReader.children() vectors
- 20ms building+destroying YamlNodeReader objects
- other negligible stuff
Of the 333ms of node key searching there is:
- 220ms (10ms in release build) of indexed searching
- 72ms (4ms in release build) of unindexed searching (with stored child iterator optimization)
What about game loading? In the release build, loading a 12MB sav file, all the unindexed searching together takes <2ms.
If your proposed optimization is targeting unindexed key searching, well, there's very little left to gain there. The majority of search calls (3 to 1) are already indexed. At least for game startup. For game-loading, the majority is unindexed, however that's mostly (80%) inside BattleUnitKills::load(), where the stored child iterator optimization gives it O(1).
It use "list" as nodes are not sequenced in memory, all exits in one big vector but you need jump between elements. it could be couple of cache lines between siblings. CPU can't preload next after sibling as it need know value in next sibling node first. With more "dense" layout, CPU can preload arbitrary sibling.
> That's why std::map never beats std::unordered_map
`std::flat_map` not `std::map`, even when we have unordered one, some people still need it and add it to standard:
https://en.cppreference.com/w/cpp/container/flat_map
There are use cases where it could be better.
overall thanks for detailed timings, at least I would need reevaluate my initial optimistic predictions.
Still I think that we can grain "more" time that even could be concluded from that only "yaml" use.
As memory cache is shared resource, improving one part can improve other unrelated parts too.
-
I did a small test. It's 100ms just to visit all the nodes of all the trees (no vector building). So, good luck~
>flat_map
flat_map still never beats unordered_map in insertion and random searching performance. The reason people need flat_map is because it's faster than map, and, like map, it gives you something unordered_map can't, which is, ordered traversal.
Even Adaptive Radix Tree, which is the ultimate tree-like structure for strings, is still 2-3 times slower than the hash map.
Anyway, if we absolutely needed to make some hot path fast, there's is a way to do that. Let's say we're currently doing:
reader.tryRead("foo", _foo)
reader.tryRead("bar", _bar)
reader.tryRead("baz", _baz)
Building an index has an overhead. But without an index it's problematic if we try looking for a key that doesn't exist. If a mapping node doesn't have any children with a key, it means you spent a lot of time looping through the children and doing string comparisons and returned empty-handed. To avoid that, a more optimal way of deserializing would be:
for (const auto& child : reader.children())
{
switch(child.key())
{
case("foo"): child.tryReadVal(_foo); break;
case("bar"): child.tryReadVal(_bar); break;
case("boo"): child.tryReadVal(_boo); break;
}
}So instead of searching for the keys that node's children may not have, we instead loop through the children. The obvious problem is, we can't do switch(string) in C++. So we would have to create a switch tree comparing one character at a time:
for (const auto& child : reader.children())
{
std::string_view key = child.key();
switch(key[0])
{
case('f'): if (key == "foo") child.tryReadVal(_foo); break;
case('b'):
switch(key[1])
{
case('a'): if (key == "bar") child.tryReadVal(_bar); break;
case('o'): if (key == "boo") child.tryReadVal(_boo); break;
}
}
}Heh, it's like we're simulating a Trie data structure. This would work, but not only does it look ugly, it's a nightmare to construct and nightmare to maintain. Unless we used some sort of fancy library (https://github.com/kampersanda/constexpr_doublearray) or whatever for it.
Lastly, I'd like to mention hashing. Hashing to the rescue!
for (const auto& child : reader.children())
{
//Uint32 newSeed = findPerfectSeed({"foo", "bar", "baz"}); // when the array of keys changes, update and run this, and replace seed with newSeed's value
Uint32 seed = 12345;
const std::string_view& key = child.key();
switch(ourHash(key, seed))
{ //ourHash is constexpr function, so case(x) will have constants for x, or fail to compile if seed is not perfect
case(ourHash("foo", seed)): if (key == "foo") child.tryReadVal(_foo); break;
case(ourHash("bar", seed)): if (key == "bar") child.tryReadVal(_bar); break;
case(ourHash("boo", seed)): if (key == "boo") child.tryReadVal(_boo); break;
default: break;
}
}
This would probably be the fastest performance-wise. It doesn't look the best, but it's easy to maintain.
-
flat_map still never beats unordered_map in insertion and random searching performance. The reason people need flat_map is because it's faster than map, and, like map, it gives you something unordered_map can't,
It can, if is in "hot cache" and `unordered_map` is "cold". And this is same reason why my idea of optimization fails as code have `O(n^2)` and in some cases work close to `std::list` but as all is small and fit cache, overall penalty is so small that preparation for `O(n + n)` version would cost more (as you, yourself point out).
Btw How much cache your CPU have? I check my and it had 6MiB of L3, this mean I could fit half of this save file. More cache you have, less my idea make sense.
Probably lot older CPU could see some benefits in theory when "working set" do not fit cache.
For most optimal solution, probably `constexpr` object that have runtime `find` function:
https://www.youtube.com/watch?v=INn3xa4pMfg
-
(https://i.postimg.cc/Njrk4mw9/cpu.png)
;)
I did link a constexpr library in the previous post.
10. all: merry Xmas
I'm starting to think Meridian is some sort of god, because that prediction was way too accurate.
-
Ok, I've done rebasing my branch to the 8.0 version.
Also, I found one bug (https://github.com/Delian0/OpenXcom/commit/e9986c3ca5a00c527d0fd905d1c57589395fdaec).
To reproduce the bug, have two items with stat multipliers in the first item defined as
firing: 0.5
throwing: 0.5
and in the other item as:
throwing 0.5
firing 0.5
In 7.15, in stats for nerds, both would be displayed as " 0.5*THROWING + 0.5*FIRING ", but in 8.0 the two item's multipliers would be displayed differently. This fix restores the display consistency.
The added error check can be tested, for instance, by changing one multiplier from "firing: 1.0" to "firin: 1.0" and checking the error log.
Btw, the fix was implemented using a feature from another commit (https://github.com/Delian0/OpenXcom/commit/21e3fd5dff00fe02b7a904688f321fb070d19064).
-
When trying out rosigma with the latest version, an assert (https://github.com/MeridianOXC/OpenXcom/blob/ae6ce0fb0b0fac97a4b49840156b31e796604afe/src/Engine/Yaml.cpp#L118) in debug build fails. The release build works fine tho.
Fixed (https://github.com/Delian0/OpenXcom/commit/0fbdbcd984cdf01105c680ae0f6e8008ab1a420d).
-
When trying out rosigma with the latest version, an assert (https://github.com/MeridianOXC/OpenXcom/blob/ae6ce0fb0b0fac97a4b49840156b31e796604afe/src/Engine/Yaml.cpp#L118) in debug build fails. The release build works fine tho.
Fixed (https://github.com/Delian0/OpenXcom/commit/0fbdbcd984cdf01105c680ae0f6e8008ab1a420d).
Merged.
Ok, I've done rebasing my branch to the 8.0 version.
The rest will be done by Yankes later.
-
I noticed some differences in the save files, see attachments.
Are these equivalent?
I.e. do they work both ways?
And even if yes, don't we want to use the old way?
(It was better readable.)
Would it have performance impact?
-
I noticed some differences in the save files, see attachments.
Lastly, the yaml standard says that certain non-printable characters should be escaped, as in, ascii code 00 should produce "\x00" in the yaml. It also states that it's not mandatory. Unlike yaml-cpp, rapidyaml does not respect this suggestion hehe. It simply outputs a control code 00 character raw. Subsequent parsing of these non-escaped characters works just fine however. It even works fine in yaml-cpp, so it's perfectly backwards compatible. As long as a string is in double quotes, everything goes.
Yes, they are equivalent. I'm not actually sure how to force rapidyaml to escape non-printable characters when serializing data.
It would have a noticable impact on serialization speed if we did it for each and every node, but if it's done only in a few places, there would be no impact. I'll see what I can do.
-
Good to know it's equivalent.
(I missed that older post.)
In that case, it's low prio, and should be done only for a few selected nodes, if at all.
-
It looks like, sadly, rapidyaml doesn't implement any detection of non-printable characters. I found out why. I spent a few hours trying to implementing something myself, but it quickly got too complicated.
The first problem is that, if you wanna do the detection correctly, then you need to implement a UTF-8 variable-width character iterator and iterate through the string that way.
The second problem is converting UTF-8 into Unicode. For instance, a character with a UTF-8 hex code 0xC285 when converted and escaped becomes "\u0085", so not a trivial thing.
And then there's detecting whether a character is printable or not. That wasn't hard to do, because the character ranges are well defined in the yaml standard.
Btw, the "tileIndexSize", "tileFireSize" etc nodes... why exactly are these nodes being serialized as strings? The values are Uint8 type, so one would expect them to be saved as numbers and not strings.
-
Not as string but as `char` (remeber `Uint8` lie to you), yaml-cpp use single quotes like `'\x01'` to somehow match C/C++ code.
Btw why support full unicode when problem is only with ASCII?
-
We use only 2 non-printing characters in strings really.
Unicode::replace(s, "{SMALLLINE}", "\x02"); // Unicode::TOK_NL_SMALL
Unicode::replace(s, "{ALT}", "\x01"); // Unicode::TOK_COLOR_FLIP
And from all fields in the save file, only 1 or 2 can actually contain them.
I wanted to manually replace \x01 with \\x01 but saving it was a bit confusing.
The same string gets saved differently depending on whether quotes are used or not.
See below, why is that the case?
std::string converted = "abc\\x01def";
headerWriter.write("craftOrBase", converted).setAsQuoted();
headerWriter.write("craftOrBase2", converted);
craftOrBase: "abc\\x01def"
craftOrBase2: abc\x01def
I would like to save it as
craftOrBase: "abc\x01def"
but I could not find a way how to do that?
-
Btw, the "tileIndexSize", "tileFireSize" etc nodes... why exactly are these nodes being serialized as strings? The values are Uint8 type, so one would expect them to be saved as numbers and not strings.
If I remember correctly, it's because certain yaml-cpp versions can/could not serialize/deserialize `Uint8` correctly and the game crashed.
So a workaround was made to save them as `char`.
-
Btw why support full unicode when problem is only with ASCII?
craftOrBase: "CRAFT> \x01AIRBUS*058-1" becomes
craftOrBase: "기체> \x01에어버스*058-1" in Korean localization.
Those are not ASCII characters hehe, so we can't be moving through the string 1 byte at a time.
why is that the case?
Well, inside double quotes, certain characters need to always be escaped. When you give it a backslash, it thinks you want a literal backslash in there.
I did find a way to hack it (https://github.com/Delian0/OpenXcom/commit/d5d3c0dfa75fd315aa2d087507f05944e8e312a9). Instead of setting the scalar style to be "double-quoted", you instead force a "plain" scalar style, and manually wrap it in double quotes. Then it won't automatically escape backslashes during saving, while correctly resolving the escaped hex code during the next load.
So a workaround was made to save them as `char`.
...why not simply cast it to Uint32 then?
Anyway, is it possible for us to change those strings to numbers now to avoid having to escape those non-printable characters? Or are there backwards-compatibility issues?
-
craftOrBase: "CRAFT> \x01AIRBUS*058-1" becomes
craftOrBase: "기체> \x01에어버스*058-1" in Korean localization.
Those are not ASCII characters hehe, so we can't be moving through the string 1 byte at a time.
But whole point of utf8 was is "compatible" with ASCII? you will never find `\x01` byte outside ascii char with value `1`.
All multi byte chars have all biggest bite (128) set. Only utf16 and utf32 could be confused as it can have zeros or other bytes in char.
This mean we can scan only for ASCII with value 1 or 2 and hack it, other can stay as is.
If we have some Unicode "unprintable" characters it will stay as is as this is orders of magnitude harder to handle.
-
...why not simply cast it to Uint32 then?
Anyway, is it possible for us to change those strings to numbers now to avoid having to escape those non-printable characters? Or are there backwards-compatibility issues?
We want "new" OXCE saves to work with older OXCE versions... and also with OXC... so far we were able to maintain that compatibility (which is almost unbelievable).
If we can't find (yet another) workaround, then I would prefer keeping the saves as they are now.
-
Ok. I rewrote (https://github.com/Delian0/OpenXcom/commit/40e51304edf7e13d469e667209ba0fcf4f81b2d2) the last solution then. When the setAsQuoted() is called, it checks if the node's scalar contains any non-printable ASCII characters, and if it does, it replaces it with a new one where the non-printables and special characters are escaped. All 2+ byte UTF-8 characters are assumed to be printable.
-
Ok. I rewrote (https://github.com/Delian0/OpenXcom/commit/40e51304edf7e13d469e667209ba0fcf4f81b2d2) the last solution then. When the setAsQuoted() is called, it checks if the node's scalar contains any non-printable ASCII characters, and if it does, it replaces it with a new one where the non-printables and special characters are escaped. All 2+ byte UTF-8 characters are assumed to be printable.
Merged.
https://github.com/MeridianOXC/OpenXcom/commit/5a8ca93dc623f5725d285825fc3ab654ec7b90e4
-
@Delian:
can you check if we can somehow help with this issue?: https://openxcom.org/forum/index.php?topic=11663.msg169391#msg169391
TL'DR: OXC/OXCE battlescape saves don't load in BOXCE
-
I notice that in some nested ruleset we lost context and messages are not helpful.
I manage to make small hack to re-enable line info errors:
https://github.com/Yankes/OpenXcom/commit/728235706ca2117463b15d470407588d5109ae5a
I think we could move in that direction further by dropping `_root` all together and use always indirection.
Only thing to be careful is not allow to this data leak outside of lifetime of root reader/writer.
What is your Delian option on this?
-
I'd want to see those "nested rulesets" that are causing the loss of error messages first.
Static is a no-go, because it breaks my multi-threading ;)
-
I'd want to see those "nested rulesets" that are causing the loss of error messages first.
Static is a no-go, because it breaks my multi-threading ;)
Where I used "static" variables that could be problem here? Only static is `YamlErrorHandler` that is stateless.
And all `tree` objects get its own "handler" as global is only fallback if not overridden.
And code that crash without info look like:
reinforcements:
- type: firstwave
minTurn: 1
maxTurn: 1
maxRuns: 1
mapBlockFilterType: 1
spawnZLevels: [7]
randomizeZLevels: falsee #crash here without info where it is
bool read(ryml::ConstNodeRef const& n, ReinforcementsData* val)
as now there is no `root` in sight. as alterative would be refactor all calls to propagate one pointer.
but as `yml::Callbacks` already need have pointer that we control, we could easy store here our `root`.
-
Hmm, nevermind that. So you're storing root in the callbacks...
Actually this looks good. It's probably a normal way to use Callbacks, and not a hack at all.
Yes, with that we can get rid of the _root reference from the YamlNodeReader/Writer and we can probably remove the global callbacks that I've set up as well.
-
Rapid YAML 0.8.0 has been released.
It fixes most of the stuff we've reported, such as problems with trailing whitespace, and UTF-8 BOM.
-
Good news.
I'll release OXCE 8.1 with the current ryml version still, because it is tested.
Later we can check if upgrading is worth it.
-
@Delian: can you please merge these 2 commits into OXCE?
https://github.com/OpenXcom/OpenXcom/commit/b6ec154611339938f5b643e411285764818a22d4
https://github.com/OpenXcom/OpenXcom/commit/0f3978894548ddf9fe6ee9c245b2253787a94702
-
Does it matter if non-existent options are put at the end of options.cfg, or do they need to be correctly sorted together with the rest of normal options?
-
Does it matter if non-existent options are put at the end of options.cfg, or do they need to be correctly sorted together with the rest of normal options?
They don't need to be sorted IMO, can be at the end.
-
https://github.com/Delian0/OpenXcom/commit/1888c52bcac470f46f68fdaaa9e9be672456813d
-
https://github.com/Delian0/OpenXcom/commit/1888c52bcac470f46f68fdaaa9e9be672456813d
Merged. Thank you.