Converting OXCE to rapidyaml

[Yankes]

@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

[Delian]

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.

[Yankes]

I will look on this tomorrow

[Delian]

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

[Yankes]

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.

[Delian]

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.

[Filip H]

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.


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:

Code: [Select]

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:

Code: [Select]

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

[Delian]

Yeah, the templates weren't being properly merged. Fixed.

[Yankes]

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.

[Delian]

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

[zRrr]

Typo in BattleUnit.cpp:633 ("turrentType") makes HWPs lose turrets when reloading saved game.

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

Code: [Select]

[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

Code: [Select]

YAML::YamlRootNodeReader reader(data, fullpath);
return reader;to

Code: [Select]

return YAML::YamlRootNodeReader(data, fullpath);fixed it. Witchcraft!

[Delian]

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.

[Delian]

`x.prependYaml(y)`

I realize that the last bugfix was a bit of a hack. So I made a proper bugfix 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.

[Yankes]

look lot better

[Delian]

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?