jmonkeyengine
jmonkeyengine copied to clipboard
jMonkeyEngine
Improve GLTF support
This is the initial work to improve and extend the gltf support in jme. This pr:
- Fix an issue with cache poisoning when manipulating the first instance of a cached spatial
- Adds the UserData loader from Simsilica and makes it the default (as expected from a jme loader)
- Moves all the extensions related code to packages with their fully qualified name to make it easier to figure out which code belongs to which official or unofficial extension
- Adds a new JME_speaker extension to load speaker nodes directly from blender scenes into jme (this will require an extension also on the blender side)
- Makes default extension registrable statically, this allows specific modules (think about (j)bullet, minie etc) to statically register their own extension in the gltf loader.
Currently testing:
- JME_physics extension automatically registered by jbullet to load rigidbodies from blender scenes (needs blender addon)
@riccardobl thanks for your contribution
Please update the copyright date to 2023 on the modified classes.
In general, a PR should do one thing. The scope of a PR should be narrow, addressing a single refactor, issue, or feature. See Creating Good Pull Requests.
According to the description, this PR includes 3 new features, a bug fix, and major refactoring.
I request that the changes be split up into multiple PRs. This will make the changes easier to understand and review.
In general, a PR should do one thing. The scope of a PR should be narrow, addressing a single refactor, issue, or feature. See Creating Good Pull Requests.
According to the description, this PR includes 3 new features, a bug fix, and major refactoring.
I request that the changes be split up into multiple PRs. This will make the changes easier to understand and review.
You are right, but since i am submitting these PRs in between projects i am doing with the engine, i have limited time and working on multiple PRs that rely on each other is very time consuming.
Another thing: package names use only lowercase letters and digits (no underscores). See the Google Java Style Guide section 5.2.1.
i am submitting these PRs in between projects i am doing with the engine, i have limited time
Are you suggesting your PRs deserve special treatment because you work on other projects and have limited time?
Another thing: package names use only lowercase letters and digits (no underscores). See the Google Java Style Guide section 5.2.1.
I would make an exception in this case to match their registered extension name in gltf (https://github.com/KhronosGroup/glTF/blob/main/extensions/README.md) but i don't have a strong opinion in this regard
i am submitting these PRs in between projects i am doing with the engine, i have limited time
Are you suggesting your PRs deserve special treatment because you work on other projects and have limited time?
I am suggesting that if you want the perfect PRs we can leave them open indefinitely until i have time to follow all the rules, or we can accept my bad PRs to get features and bug fixes merged quickly as long as the code works.
I am always working on a fork, so i don't have any urge to get my code merged, and besides, last time i checked i had push rights to the repo.
Regarding package names, even though I do like your approach but yeah that seems to be not a standard Java package naming(?). Also as most of those extensions turn out to be a single class file anyway, I would recommend having a gltf.ext.khr package for all Khronos extensions, (but if you like you can also divide them like gltf.ext.khr.light, gltf.ext.khr.material, gltf.ext.khr.texture,....)
Next, we can have a gltf.ext.jme package for JME extensions. Similarly, if you think a JME extension is going to have multiple related classes (which sounds unlikely to me) you can make a subpackage for that. (e.g. gltf.ext.jme.audio,...)
Regarding PR complexity, looks like UserDataLoader is imported from the JmeConvert library(?), if so, I am already using that in my project and it works just fine, so I think we can skip reviewing the functionality of that class.
Next, we can have a
gltf.ext.jmepackage for JME extensions.
I think we can even omit jme from above, so we have just gltf.ext for custom JME extensions and gltf.ext.khr for Khronos extensions. What do you think?
Just my two cents!
we can leave them open indefinitely until i have time to follow all the rules, or we can accept my bad PRs to get features and bug fixes merged quickly
The goal is to do what's best for the project in the long term. Code added to the project requires maintenance, so we want PRs to be in as good shape as possible before they're integrated, while the burden of maintenance falls on the submittor, not the community.
If this PR is urgent for some reason, please document that reason.
besides, last time i checked i had push rights to the repo.
As do many other people. Why even bring that up? This is a discussion of proposed changes to the codebase, not a discussion of Riccardo. Let's keep the discussion as impersonal as possible.
we can leave them open indefinitely until i have time to follow all the rules, or we can accept my bad PRs to get features and bug fixes merged quickly
The goal is to do what's best for the project in the long term. Code added to the project requires maintenance, so we want PRs to be in as good shape as possible before they're integrated, while the burden of maintenance falls on the submittor, not the community.
If this PR is urgent for some reason, please document that reason.
besides, last time i checked i had push rights to the repo.
As do many other people. Why even bring that up? This is a discussion of proposed changes to the codebase, not a discussion of Riccardo. Let's keep the discussion as impersonal as possible.
Does the size of the PR reflects on the quality of the code?
As do many other people. Why even bring that up? This is a discussion of proposed changes to the codebase, not a discussion of Riccardo. Let's keep the discussion as impersonal as possible.
Pretend the PR wasn't submitted and consider the single commits as if they were pushed to the master, you will see that every change has its own commit and the fact they are grouped on a single PR by the github interface doesn't make it less true.