Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use cJSON for JSON (un)marshalling instead of a custom implementation #1833

Open
jonbonazza opened this issue Nov 13, 2020 · 2 comments
Open

Comments

@jonbonazza
Copy link

jonbonazza commented Nov 13, 2020

Describe the project you are working on:
Any game that uses JSON (especially online games that communicate with webapis). Also relevant for the Godot engine code itself.

Describe the problem or limitation you are having in your project:
While fleshing out unit tests for Godot's JSON facilities, @Calinou has found several issues with the Godot JSON parser not being conformant to spec (PR Link) (TODO: Link to tracking issue once created). This is very bad in my opinion, especially for online games that rely on communications with various webapis (first and 3rd party) for various auxiliary (read: non-gameplay) functionality, such as matchmaking, database apis, commerce, etc... since these apis almost always use JSON for their content types.

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
From my understanding, Godot's JSON facilities are fairly ancient, and have existed long before Godot was even open sourced. Since then some very tiny, well reputed, battle tested, and portable json libraries have sprung up. One of such libs, cJSON has pretty much become the defacto JSON library for anything written in C or C++. It's used pretty much everywhere is incredibly tiny, fast and easy to use, is as portable as can be, and is 100% conformant to spec.

I suggest we add cjson as a Godot core dependency and use it for Godot's JSON needs.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:

Godot's JSON apis are already very isolated, so changing them to use cjson would be extremely easy. As I already mentioned, cjson is as portable as portable can be, and since most (all?) consoles and platforms already support or use it anyway, this should be enough. If we really wanted to be safe, however, we could make the json stuff part of the platform specific code, which would allow us to implement different json backends depending on platform, but I really don't think this is necessary.

If this enhancement will not be used often, can it be worked around with a few lines of script?:
No. JSON is a core feature

Is there a reason why this should be core and not an add-on in the asset library?:
Technically speaking, there's nothing stopping someone from creating a GDNative plugin for cjson, however since JSON is so integral to gamedev--especially online games as mentioned before, I believe that this makes the most sense as an enhancement to core.

@Calinou Calinou changed the title Use cJSON for JSON (un)marhsalling Use cJSON for JSON (un)marhsalling instead of a custom implementation Nov 13, 2020
@silverkorn
Copy link

Might simdjson be a potentially better choice?
It seems to be cross-platform and it's under Apache 2.0 license.
It might not be as mature as cJSON and easily implementable but it's worth to give it a look! 😄

@Calinou
Copy link
Member

Calinou commented Jan 13, 2021

Might simdjson be a potentially better choice?
It seems to be cross-platform and it's under Apache 2.0 license.
It might not be as mature as cJSON and easily implementable but it's worth to give it a look! smile

Performance isn't critical for our use cases. Portability is more important, as we need something that works well on x86, ARM and possibly more (consoles).

PS: Small reminder about implementing cJSON if we go for it:

Case Sensitivity

When cJSON was originally created, it didn't follow the JSON standard and didn't make a distinction between uppercase and lowercase letters. If you want the correct, standard compliant, behavior, you need to use the CaseSensitive functions where available.

@Calinou Calinou changed the title Use cJSON for JSON (un)marhsalling instead of a custom implementation Use cJSON for JSON (un)marshalling instead of a custom implementation Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants