search

R2Northstar / NorthstarLauncher

Launcher used to modify Titanfall 2 to allow mods to be loaded
MIT License
271 stars 125 forks source link

Choice of JSON Library | Migrating from RapidJSON #715

Closed Jan200101 closed 2 weeks ago

Jan200101 commented 1 month ago

While working on #713 I found out that newer toolchains fail to build RapidJSON due to const-qualified types being kept const https://github.com/Tencent/rapidjson/issues/2277

With RapidJSON not having had a release in almost a decade, maybe we should switch to a better maintained library?

Needs to be discussed.

Jan200101 commented 1 month ago

Personally I think https://github.com/nlohmann/json would be a good replacement. It's already considered the go-to C++ JSON library and would allow cmake integration.

Jan200101 commented 1 month ago

It's been mentioned on discord RapidJSON is still in development however they stopped tagging commits. I really hate this because it doesn't give us anchors to work with where we can point at a new release every now and then or where one can say "This fix was included with Version XYZ". This essentially puts more work on us where we have to keep track of changes ourselves.

We could continue cherry picking fixes or fixing stuff ourselves or move over to a library that respects the ecosystem and didn't just release it for luls like Tencent did.

Jan200101 commented 1 month ago

Digging through the chat log about previous discussions

[1:53 PM]Gecko: Does the nlohmann support [JSON5](https://json5.org/) ? ^^"

Cause rapidjson allows for comments in JSON which is part of JSON5 spec (but it also doesn't fully support JSON5 cause that would be too easy :dread:)

Basically, if planning to replace rapidjson, the replacement needs to be able to deal with comments (and maybe trailing commas) cause otherwise a bunch of mods will start to fail parsing due to comments in their mod.json...
Jan200101 commented 1 month ago

It was mentioned that one reason RapidJSON was chosen is performance. Here are some alternatives which are high performance JSON Libraries which we can use in C++:

Both of which consider themselves fasters than RapidJSON by a large margin.

GeckoEidechse commented 1 month ago

Depending on how substantial changing JSON library would be, I guess we could treat RapidJSON as a rolling release and use something like dependabot for informing us about new commits. Of course I understand that this is not optimal so I'd love input from anyone who might have more experience on the matter ^^

ASpoonPlaysGames commented 2 weeks ago

primedev moved over to nlohmann (or however it is spelt) and that seemed to go fine. Might be relatively trivial to port as well

Jan200101 commented 2 weeks ago

I'm also in favor of nlohmann since its saner than RapidJSON and actually makes releases but when this was discussed on Discord

[1:20 AM]p0358: also nlohmann sucks for performance so [1:20 AM]p0358: that'd be a downgrade for no particular reason [1:20 AM]Jan: oh sorry, I didn't know we use RapidJSON every frame [1:21 AM]p0358: oh sorry, I forgot Northstar already takes long as shit to boot up, might as well propose to make it even longer [1:21 AM]p0358: and spend yet extra of people's time implementing that

Which is why I am more in favor of yyjson to squash those complaints (not that RapidJSON was even that fast to begin with, since we never enabled SIMD optimizations)

GeckoEidechse commented 2 weeks ago

How much performance impact do we actually have from reading JSON? Like we read enabledmods.json, and the mod.json of however many mods the user has installed. That's it in regards to the launch sequence, right? ^^"

If primedev uses nlohmann as @ASpoonPlaysGames mentioned I'm slightly in favour of using that as well, primarily so that we keep the codebases closer together so that @ASpoonPlaysGames can port over the remaining changes more easily.

To improve launching performance of Northstar we should find out what causes the greatest bottleneck and start there rather than base our choice of JSON library on it when (I assume) reading the JSON files makes up a very minuscule part of the slow launch sequence.

GeckoEidechse commented 2 weeks ago

Ok, according to @klemmbaustein nlohmann does not support trailing commas. Given that our JSON parsing is pretty lenient, essentially almost JSON5 we need to keep up that leniency in order to not break too many existing mods.

Similar thing with comments which are used in a bunch of mods and why FlightCore and Viper just assume JSON5 for parsing mod.json. No clue what VTOL does but it also supports comments.

Basically, we need to support