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

PROCESS JSON COMMAND - New Features #3341

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

jetrotal
Copy link
Contributor

@jetrotal jetrotal commented Jan 23, 2025

The Operations were updated to:

0: GET VALUE
1: SET VALUE
2: GET LENGTH
3: GET KEYS
4: GET VAR TYPE
5: REMOVE VALUE
6: PUSH INTO ARRAW
7: POP FROM ARRAY
8: CHECK IF VALUE EXISTS

There are also new Flags:

  • Extract data from json path - similar to how stringvars extracts \v[1], \t[2], etc.
  • Prettify json - to make its output easier to read.

@Ghabry
Copy link
Member

Ghabry commented Jan 23, 2025

We had this discussion before: Exceptions (try-catch) are not enabled on emscripten to reduce binary size.

(and our homebrew platforms but they do not have JSON support enabled so you do not see them failing here)

The Operations were updated to:

0: GET
1: SET
2: GET LENGTH
3: GET KEYS
4: GET VAR TYPE

There are also new Flags:
 - Extract data from json path - similar to how stringvars extracts.
 - Prettify json - to make its output easier to read.

Update json_helper.cpp
@jetrotal
Copy link
Contributor Author

Sorry about the confusion, Now the code is way closer to how initially was

@jetrotal
Copy link
Contributor Author

jetrotal commented Jan 23, 2025

I found some OOB crashes from the older implementation,
When trying to reach /lists/data/6 from:

"lists": {
            "length": 5,
            "data": [{
                    "id": "MAP TYPE",
                    "length": 7,
                    "values": ["EasyRPG World", "EasyRPG Exterior", "EasyRPG Interior", "EasyRPG Dungeon", "EasyRPG Ship", "Uncanny World", "Danny's House"],
                    "chipset_files": ["EasyRPG_World", "EasyRPG_Exterior", "easyRPG_interiorB", "EasyRPG_Dungeon", "EasyRPG_Ship", "EasyRPG+gfragger_mkt_CCBY", "EasyRPG+kotatsuAkira_Interior_CC0"],
                    "tileset_locations": [1531, 1535, 1534, 1533, 1532, 523, 524],
                    "current": 0
                },
                {
                    "id": "TILES LAYER",
                    "length": 3,
                    "values": ["LOWER", "UPPER", "EVENTS"],
                    "current": 0
                },
                {
                    "id": "DRAW AUTOTILES",
                    "length": 2,
                    "values": ["YES", "NO"],
                    "current": 0
                },
                {
                    "id": "MAP PANORAMA",
                    "length": 4,
                    "values": ["[OFF]", "A", "B", "C"],
                    "current": 0
                },
                {
                    "id": "BACKGROUND MUSIC",
                    "length": 4,
                    "values": ["[OFF]", "A", "B", "C"],
                    "current": 0
                }
            ]
        },

I thought about adding some error messages, but those messages could conflict with cases such as:

[
[-6,-6,-6,-6,-6,-6,-6,-6,-6,-6],
[6,-6,-6,null,-6,-6,-6],[-6,-6,-6,-6,-6,-6,-6,null,null,null,-6,-6],
[-6,6,-6,null,-6,-6,-6,-6,-6,-6,-6,-6],
[-6,6,6,null,14,14,14,-6,14,14,14,-6,14,14,14,null,14,14,14,6],
[6,6,6,null,null,14,6,null,14,null,null,null,14,-6,-6,-6,-6,14]
]

where I wanted to have null values to fast walk through a list of map tiles.

@Ghabry Ghabry added this to the 0.8.1 milestone Jan 24, 2025
…lohmann::ordered_json

this can break for loops, and it's a weird extra step when processing json data
@Ghabry
Copy link
Member

Ghabry commented Feb 5, 2025

To fix (well, disable) nlohmann for Ubuntu 20.04 you must require a newer version of nlohmann. 3.9.1 appears to work.

In CMakeLists:

	player_find_package(NAME nlohmann_json
		VERSION 3.9.1
		CONDITION PLAYER_WITH_NLOHMANN_JSON
		DEFINITION HAVE_NLOHMANN_JSON
		TARGET nlohmann_json::nlohmann_json
		ONLY_CONFIG
	)

There are 2 cases where nlohmann_json is used in the file. Please add VERSION to both.

configure.ac:

EP_PKG_CHECK([NLOHMANN_JSON],[nlohmann_json >= 3.9.1],[Support processing of JSON files.])

@Ghabry
Copy link
Member

Ghabry commented Feb 21, 2025

Another observation:

  • The operations Get|Length|Keys|Type do nothing when the JSON path is missing (looks intentional)
  • The operations Set and Push do not modify the JSON when the source string var is empty (Bug?)
  • The operation Pop does not assign and not update the JSON when the popped element is an empty string (Bug?)

@Ghabry
Copy link
Member

Ghabry commented Feb 21, 2025

btw to handle the complexity I wrote unit tests to verify the JSON Api (will commit them when the remarks are addressed).

Can confirm that the rest of it appears to work 👍 . Good job.


Also found the GetPath function which looks incomplete (will not find elements when they are integers) and is unused. Guess you started working on this and then decided to not finish it. But this looks useful later so can imo stay this way for now.

@Ghabry Ghabry added Event/Interpreter EasyRPG New functionality exclusive to EasyRPG Player labels Feb 23, 2025
@jetrotal jetrotal force-pushed the JSON-UPDATES branch 2 times, most recently from fdcd3bb to 81f1b88 Compare February 24, 2025 00:04
@Ghabry Ghabry marked this pull request as draft February 24, 2025 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EasyRPG New functionality exclusive to EasyRPG Player Event/Interpreter
Development

Successfully merging this pull request may close these issues.

2 participants