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

New Command 2055 - Process JSON Code #3215

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jetrotal
Copy link
Contributor

@jetrotal jetrotal commented May 3, 2024

This command allows you to Get or Set values inside a JSON string.
(Requires Maniacs Patch)

The syntax always comes in pairs. The first parameter of each pair indicates whether you are using a direct value or a variable/indirect variable, through ValueOrVariable().


Syntax (TPC):

@raw 2055,
      "",          // Path to the target value within the JSON data
      0, 0,        // Operation (0-1) -  0: Get (extract value), 1: Set (update value)
      0, 5,        // Source string variable ID containing the JSON data
      0, 2,        // Target variable Type (0-2) - 0: Switch, 1: Variable, 2: StringVariable
      0, 1,        // Target variable ID to receive data (for Get) or contain new value (for Set)
      0, 100,      // Usage of string variable instead of the path string.

Get Example:

  • Extract the value of "d" from the JSON data {"a":{"b":{"c":[{"d":"result"},"not_result"]}}} and store it in the string variable with ID 1.
@raw 2055,
      "a.b.c[0].d", // Path to the target value
      0, 0,         // Operation: Get
      0, 5,         // Source variable ID containing the JSON data
      0, 2,        // Target variable is a StringVar
      0, 1,         // Target variable ID to receive the extracted value
      0, 0          // Use literal path string

Set Example:

  • Update the value of "d" in the JSON data stored in the string variable with ID 5, using the value from the string variable with ID 2.
@raw 2055,
      "a.b.c[0].d", // Path to the target value
      1, 0,         // Operation: Set
      0, 5,         // Source variable ID containing the JSON data that will be modified
      0, 2,        // Target variable is a StringVar
      0, 2,         // Target variable ID containing the new value for "d"
      0, 0          // Use literal path string

This command works nice until you deal with a json string with more than 2mb:
image

Every time I call it, the engine freezes for some frames.
That may happen due to how I keep parsing and deleting entire JSON objects every time I call it. Maybe there's a way to keep it stored in memory for some time, to deal with multiple calls in sequence.

Here's a JSON file I used for testing bigger strings:
JSONdatabase.txt

@florianessl
Copy link
Member

Hm... neat idea. I was just thinking about how it would be nice to have a special storage type for user defined structs.
My current draft for ScopedVars rewrites the storage for Switches & Variables (possibly also strings - not tried yet) to share a common base class and the first thing that came to mind after this was done was: how about support for more complex data types? :D
Haven't thought about JSON, but this could work..

So, a possible solution for the freezes would be of course, to subclass this base storage for a new JSON type and use it to keep the data in memory. This would have the upside of already having access to all the common mechanisms (Get, Set, Range operations, Validation, Load&Save, special operations for "scoped" variables...) for the different data types without any extra implementation.
I'd have to share my current code of course. I think I can finally also create a PR for my little experiments...

@jetrotal
Copy link
Contributor Author

jetrotal commented May 3, 2024

@florianessl huh, thats cool!
My idea was ducktaped like this due to being easy to implement without creating issues in savegame.
my main needs was having a json file that I could import/export and a way to call objects by name.

Your idea seems to be more solid, though I don't have the knowledge to make it work.
Feel free to adopt it instead of my current solution, it's kinda messy as it is.

@Ghabry
Copy link
Member

Ghabry commented May 3, 2024

I like this idea. But I think this needs an extra argument to reason about the "data type".

So the code knows whether to read/write from an int variable or a string variable. Is currently a bit ambiguous (and sometimes you want that a number lands in a string directly).


You almost implemented JSON Path for the lookup. Was this intentional? https://goessner.net/articles/JsonPath/

JSON path is supported by certain libraries, so no need to implement this manually :).

I'm not sure if picojson supports this. Picojson actually only exists because it is a single header and is only used in emscripten. For any more serious work we can switch to any other JSON library. For tools we e.g. use nlohmann json.


For performance we could do some simple caching of a "json object" in memory in the String class to make this faster the first time this command is used.

@jetrotal
Copy link
Contributor Author

jetrotal commented May 3, 2024

@Ghabry right now my outputs are always string. There are ways on strvars to parse them as int later. But reducing the amount of steps as you suggested may be the better approach.

PicoJson was kinda rejected on the stuff I researched. I kept it because it was already used.

I'm leaning towards florianessi suggestions. Seems a proper evolution to what I proposed.

@Ghabry
Copy link
Member

Ghabry commented May 3, 2024

Okay then I will mark this as a draft first as it depends on work of @florianessl . Before we do the same work twice. ^^

@Ghabry Ghabry added Event/Interpreter Has PR Dependencies This PR depends on another PR EasyRPG New functionality exclusive to EasyRPG Player labels May 3, 2024
@Ghabry Ghabry marked this pull request as draft May 3, 2024 11:02
@jetrotal jetrotal force-pushed the jetrotal-jsonCMD branch 2 times, most recently from 3d444dc to f98c9fc Compare May 20, 2024 00:57
@jetrotal
Copy link
Contributor Author

OK @Ghabry, I guess this doesn't have PR dependencies, since I won't work directly with Florian's code by now.


I'm having a problem compiling this in some platforms.

Looks like some versions of easyRPG supports this by default:
#include <nlohmann/json.hpp>

Others (android, web, ubuntu, etc) can't find it.

How can I proceed?

@Ghabry
Copy link
Member

Ghabry commented May 20, 2024

This needs a detection in the build system because not all platforms have this library by default. I will add a detection for you in ~1 day.

@Ghabry Ghabry added Has PR Dependencies This PR depends on another PR and removed Has PR Dependencies This PR depends on another PR labels May 20, 2024
@Ghabry
Copy link
Member

Ghabry commented May 21, 2024

FYI: Because this is an optional dependency to make this work after #3228 is merged you must edit your files (see e.g. filesystem_lzh.cpp/.h where the same logic is used):

Header:

LICENSE

#ifndef
#define

#include "system.h" <- NEW

#ifdef HAVE_NLOHMANN_JSON <- NEW

[...] all the code here

#endif <- NEW

#endif

Source:

LICENSE

#include "json_helper.h"

#ifdef HAVE_NLOHMANN_JSON <- NEW

[...] all the other includes and code here

#endif <- NEW

In game_interpreter.cpp you must guard the event command:

// Inside JsonCommand

#ifndef HAVE_NLOHMANN_JSON
Output::Warning("CommandProcessJson: JSON not supported on this platform");
return true;
#else
All the normal event code here
#endif

@jetrotal jetrotal marked this pull request as ready for review July 13, 2024 03:31
@jetrotal
Copy link
Contributor Author

Rebased and no longer "Draft".

It's a bit faster and now you can tell if the target output is Switch, Variable, or StringVar.

Update game_message.cpp
@fdelapena fdelapena added the Awaiting Rebase Pull requests with conflicting files due to former merge label Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Rebase Pull requests with conflicting files due to former merge Building EasyRPG New functionality exclusive to EasyRPG Player Event/Interpreter Has PR Dependencies This PR depends on another PR
Development

Successfully merging this pull request may close these issues.

4 participants