Converting OXCE to rapidyaml

[Yankes]

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.

[Delian]

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

[Meridian]

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

[Delian]

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.

[Meridian]

One compilation error.
But after removing that line, it compiles for me too.

[Delian]

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

[Delian]

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

[Yankes]

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

[Delian]

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.

[Yankes]

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.

[Delian]

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?

[Delian]

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:


On average, 2742ms to load and 3074ms to save

After:


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?

[Yankes]

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.

[Delian]

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?

[Yankes]

"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