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 file abstraction improvements #2344

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JNightRider
Copy link
Contributor

Hello everyone.

This PR improves the abstraction of JSON objects (files), as it is currently very limited and if there are null values ​​it is very difficult (cumbersome) to deal with as it does not provide mechanisms to identify it.

Also in this PR topic #2316 (implementation of method to identify the type of element) is addressed, since this topic is closely linked to the objective of this PR (which is to improve said abstraction) and is included in it,

At the moment, as far as I know, these changes don't break anything (especially the GLTF loader, in my tests everything works)

What do you think of these changes?

@capdevon
Copy link
Contributor

capdevon commented Jan 30, 2025

Hi @JNightRider,
Thank you for your efforts on the recent changes. However, I am reluctant to accept your modifications, especially those that introduce java generics and wrappers. I'm concerned these changes could further complicate an already complex API, particularly given the recent split of the jme3-plugins module into jme3-plugins-json and jme3-plugins-json-gson. This transformation led to model-loading bugs and fixes, which, as you know, took time to resolve and delayed the release of the latest version.

This module is very sensitive and is a critical component, as it is jME's only standard tool for loading glTF models. The splitting of the modules was driven by the effort to bring jMonkeyEngine to the Browser with WebGL2 and TeaVM .
(see the discussion here: link)

While I appreciate your efforts, I would advise against further risky micro-optimizations to this module. Any changes you introduce should be compatible with the Browser API, which is the main reason we are in this situation.

I understand it can be frustrating when proposed changes aren't accepted. My intention is to encourage your enthusiasm, not discourage it. If another maintainer reviews your PR and deems it low-risk, we can proceed. Otherwise, I suggest closing it.

@JNightRider
Copy link
Contributor Author

Hi @capdevon

However, I am reluctant to accept your modifications, especially those that introduce java generics and wrappers. I'm concerned these changes could further complicate an already complex API

In the case of the generic class I think it is an improvement since we do not have to do a "Casting" when using the elements of said objects, as for the wrappers, it is for
have better control over the conversion of GSON objects to JME-JSON.

I don't really think it makes the API more complicated, it simplifies it a bit more (when reading the code) and these changes apply more to the default jme3-plugins-json module implementation that JME offers, ie; If another abstraction is implemented, it is not necessary to use the jme3-plugins-json-gson module but simply the jme3-plugins-json module and said abstraction can take another path.

This module is very sensitive and is a critical component, as it is jME's only standard tool for loading glTF models

I completely agree with you, this issue needs to be addressed carefully to avoid bugs in future versions.

Any changes you introduce should be compatible with the Browser API, which is the main reason we are in this situation.

I would like to clarify this topic, the jme3-plugins-json module is the parent (the bridge, correct me if I'm wrong) for WebGL2 support, as far as I know the support for this technology has implemented its own abstraction (code here), so the generic classes and wrappers of the jme3-plugins-json-gson module would not affect browser compatibility.

As for the jme3-plugins-json module, it is possible to implement the new methods using TeaVM (so there would be no problems).

If another maintainer reviews your PR and deems it low-risk, we can proceed. Otherwise, I suggest closing it.

For now I will keep it open (possibly close it if the risk remains high).

Thanks for reviewing this PR 👍.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants