Author Topic: Converting OXCE to rapidyaml  (Read 3019 times)

Offline Meridian

  • Global Moderator
  • Commander
  • ***
  • Posts: 9061
    • View Profile
Re: Converting OXCE to rapidyaml
« Reply #30 on: November 01, 2024, 01:16:40 pm »
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.

Offline Yankes

  • Moderator
  • Commander
  • ***
  • Posts: 3331
    • View Profile
Re: Converting OXCE to rapidyaml
« Reply #31 on: November 01, 2024, 09:06:39 pm »
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.

Offline Meridian

  • Global Moderator
  • Commander
  • ***
  • Posts: 9061
    • View Profile
Re: Converting OXCE to rapidyaml
« Reply #32 on: November 02, 2024, 10:34:09 am »
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.

Offline Delian

  • Colonel
  • ****
  • Posts: 470
    • View Profile
Re: Converting OXCE to rapidyaml
« Reply #33 on: November 02, 2024, 12:19:47 pm »
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.
« Last Edit: November 02, 2024, 12:45:02 pm by Delian »

Offline Meridian

  • Global Moderator
  • Commander
  • ***
  • Posts: 9061
    • View Profile
Re: Converting OXCE to rapidyaml
« Reply #34 on: November 02, 2024, 12:31:18 pm »
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.

Offline Delian

  • Colonel
  • ****
  • Posts: 470
    • View Profile
Re: Converting OXCE to rapidyaml
« Reply #35 on: November 02, 2024, 01:40:44 pm »
It takes 3 minutes and 50 seconds for me, 8 seconds will indeed get lost there.



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?
« Last Edit: November 02, 2024, 03:11:01 pm by Delian »

Offline Meridian

  • Global Moderator
  • Commander
  • ***
  • Posts: 9061
    • View Profile
Re: Converting OXCE to rapidyaml
« Reply #36 on: November 02, 2024, 02:18:33 pm »
I have 10 cores, but they're all lazy drunks :)

Offline Yankes

  • Moderator
  • Commander
  • ***
  • Posts: 3331
    • View Profile
Re: Converting OXCE to rapidyaml
« Reply #37 on: November 02, 2024, 03:30:42 pm »
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:
Code: [Select]
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.

Offline Delian

  • Colonel
  • ****
  • Posts: 470
    • View Profile
Re: Converting OXCE to rapidyaml
« Reply #38 on: November 02, 2024, 04:24:38 pm »
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:
Code: [Select]
for (const auto& optionInfo : _info)
{
optionInfo.save(node);
}
into
Code: [Select]
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.

Offline Delian

  • Colonel
  • ****
  • Posts: 470
    • View Profile
Re: Converting OXCE to rapidyaml
« Reply #39 on: November 03, 2024, 01:08:29 am »
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.

Code: [Select]
if (doc.begin()->second.IsMap())
{
lang = doc.begin()->second;
}
to
Code: [Select]
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.
« Last Edit: November 10, 2024, 12:30:53 pm by Delian »

Offline Delian

  • Colonel
  • ****
  • Posts: 470
    • View Profile
Re: Converting OXCE to rapidyaml
« Reply #40 on: November 04, 2024, 12:35:30 am »
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...
« Last Edit: November 04, 2024, 10:53:25 pm by Delian »

Offline Yankes

  • Moderator
  • Commander
  • ***
  • Posts: 3331
    • View Profile
Re: Converting OXCE to rapidyaml
« Reply #41 on: November 04, 2024, 11:32:38 pm »
`!!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).


Offline Delian

  • Colonel
  • ****
  • Posts: 470
    • View Profile
Re: Converting OXCE to rapidyaml
« Reply #42 on: November 06, 2024, 01:09:51 am »
`!!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...
« Last Edit: November 06, 2024, 02:30:39 am by Delian »

Offline Delian

  • Colonel
  • ****
  • Posts: 470
    • View Profile
Re: Converting OXCE to rapidyaml
« Reply #43 on: November 06, 2024, 09:56:58 pm »
Code: [Select]
1>Done building project "OpenXcom.2010.vcxproj".
========== Build: 1 succeeded, 0 failed, 0 up-to-date, 0 skipped ==========

8)

Offline Yankes

  • Moderator
  • Commander
  • ***
  • Posts: 3331
    • View Profile
Re: Converting OXCE to rapidyaml
« Reply #44 on: November 06, 2024, 11:23:16 pm »
Code: [Select]
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 `==`.