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

JSON parser abstraction. #2060

Merged
merged 9 commits into from Aug 21, 2023
Merged

JSON parser abstraction. #2060

merged 9 commits into from Aug 21, 2023

Conversation

riccardobl
Copy link
Member

This PR substitutes the obligatory gson dependency in jme3-plugins with an abstraction, akin to the BufferAllocationFactory system. This change allows the developers to bind a custom JSON parser implementation through System properties as replacement for the default parser that uses GSON.

This PR introduces two new modules:

  • jme3-plugins-json: This module contains the abstraction layer.
  • jme3-plugins-json-gson: this module contains the gson implementation

Both modules are included as dependencies within jme3-plugins, to maintain backward compatibility. However, in the future, developers should be encouraged to manually add jme3-plugins-json-gson into their build.gradle files.

The goal of this PR is to improve compatibility with non-standard JVMs and platforms, since GSON is a pretty complex library that heavily relies on reflection and Java internals (such as Unsafe), and for this reason it is very hard to port on different platforms.

Copy link
Contributor

@Scrappers-glitch Scrappers-glitch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@riccardobl Thanks for the PR Ricc, if you don't mind, I would like to just add some suggested changes here, on top of that, I think we need a package-info.java for the new packages and an example illustrating the use of this new API.

@Ali-RS
Copy link
Member

Ali-RS commented Aug 10, 2023

The goal of this PR is to improve compatibility with non-standard JVMs and platforms, since GSON is a pretty complex library that heavily relies on reflection and Java internals (such as Unsafe), and for this reason it is very hard to port on different platforms.

Have you tried to see if GSON fails with teavm? And do you perhaps have another JSON library in mind that can be used safely with teavm on the web?

@riccardobl
Copy link
Member Author

The goal of this PR is to improve compatibility with non-standard JVMs and platforms, since GSON is a pretty complex library that heavily relies on reflection and Java internals (such as Unsafe), and for this reason it is very hard to port on different platforms.

Have you tried to see if GSON fails with teavm? And do you perhaps have another JSON library in mind that can be used safely with teavm on the web?

Yes, it fails. Luckily javascript has its own default JSON parser, so for the teavm backend it was just a matter of wrapping it in this abstraction, no external library needed.

@Ali-RS
Copy link
Member

Ali-RS commented Aug 10, 2023

Do you think jme3-plugins-json abstraction should also expose interface for mapping JSON to/from Java objects?

@riccardobl
Copy link
Member Author

@riccardobl Thanks for the PR Ricc, if you don't mind, I would like to just add some suggested changes here, on top of that, I think we need a package-info.java for the new packages and an example illustrating the use of this new API.

Thanks for the review.
I've added the package-info.
Regarding the examples, the goal of this pr is to make the engine compile without GSON, i am not sure if these modules will be useful outside of the core (i would consider them to be similar to the old jme3-bullet-native in scope)

Copy link
Contributor

@Scrappers-glitch Scrappers-glitch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, looks fine to me, please notice that there are still some wildcard imports and non-documented public fields distributed among some source files.

Thank you.

EDIT: and some code formatting errors too.

@riccardobl
Copy link
Member Author

Do you think jme3-plugins-json abstraction should also expose interface for mapping JSON to/from Java objects?

I don't think so, because that would be very hard to implement without using reflections or other tricks

@riccardobl riccardobl merged commit 4c87531 into jMonkeyEngine:master Aug 21, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants