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

Improve GLTF support #2069

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

Conversation

riccardobl
Copy link
Member

@riccardobl riccardobl commented Sep 4, 2023

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)

Comment on lines +57 to +67
final float volume = jsonObject.getAsJsonPrimitive("volume").getAsFloat();
final float pitch = jsonObject.getAsJsonPrimitive("pitch").getAsFloat();
final float attenuation = jsonObject.getAsJsonPrimitive("attenuation").getAsFloat(); // unused
final float distanceMax = jsonObject.getAsJsonPrimitive("distance_max").getAsFloat();
final float distanceReference = jsonObject.getAsJsonPrimitive("distance_reference").getAsFloat();
final float volume_min = jsonObject.getAsJsonPrimitive("volume_min").getAsFloat(); // unused
final float volume_max = jsonObject.getAsJsonPrimitive("volume_max").getAsFloat(); // unused
final float angleOuterCone = jsonObject.getAsJsonPrimitive("angle_outer_cone").getAsFloat();
final float angleInnerCone = jsonObject.getAsJsonPrimitive("angle_inner_cone").getAsFloat();
final float outerConeVolume = jsonObject.getAsJsonPrimitive("outer_cone_volume").getAsFloat(); // unused
final String soundPath = jsonObject.getAsJsonPrimitive("sound_path").getAsString();
Copy link
Member

@Ali-RS Ali-RS Sep 5, 2023

Choose a reason for hiding this comment

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

Properties are named in camel case in the rest of the Gltf extensions so I would suggest keeping the same naming convention here as well, please. (i.e. distance_max -> distanceMax,...)

@@ -63,7 +61,7 @@ public abstract class MaterialAdapter {

protected abstract MatParam adaptMatParam(MatParam param);

protected void init(AssetManager assetManager) {
public void init(AssetManager assetManager) {
Copy link
Member

@Ali-RS Ali-RS Sep 5, 2023

Choose a reason for hiding this comment

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

Why method access modifier is changed to "public" here?

Comment on lines +8 to +42
/*
* $Id$
*
* Copyright (c) 2019, Simsilica, LLC
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
*
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
*
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in
* the documentation and/or other materials provided with the
* distribution.
*
* 3. Neither the name of the copyright holder nor the names of its
* contributors may be used to endorse or promote products derived
* from this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
* FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
* COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
* INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
* (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
* SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
* STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
* OF THE POSSIBILITY OF SUCH DAMAGE.
*/
Copy link
Member

@Ali-RS Ali-RS Sep 5, 2023

Choose a reason for hiding this comment

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

Please move the copyright notice to the top.

Comment on lines +63 to +82
public Object handleExtras(
GltfLoader loader,
String parentName,
JsonElement parent,
JsonElement extras,
Object input
) {
log.fine(
"handleExtras(" +
loader +
", " +
parentName +
", " +
parent +
", " +
extras +
", " +
input +
")"
);
Copy link
Member

@Ali-RS Ali-RS Sep 5, 2023

Choose a reason for hiding this comment

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

Regarding the code-formatting here and some other places, I like to place them in one line until the line length exceeds the max length, only then I would move them to the new line but this is just my own preference, not sure about others though!

Edit:
FYI, this is how the original code is formatted, which sounds nicer to me.
(4 lines vs. 20 lines)

https://github.com/Simsilica/JmeConvert/blob/ad52c7729cb9a2345eaeb88cc037fb64348d8779/src/main/java/com/simsilica/jmec/gltf/GltfExtrasLoader.java#L73-L76

@Ali-RS
Copy link
Member

Ali-RS commented Sep 5, 2023

@riccardobl thanks for your contribution

Please update the copyright date to 2023 on the modified classes.

} else if (jp.isString()) {
return jp.getAsString();
}
log.fine("Unhandled primitive:" + jp);
Copy link
Member

Choose a reason for hiding this comment

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

Seems this should be a "warning"?

} else if (el instanceof JsonPrimitive) {
return toAttribute(el.getAsJsonPrimitive(), nested);
}
log.fine("Unhandled extras element:" + el);
Copy link
Member

Choose a reason for hiding this comment

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

Seems this should be a "warning"?

@stephengold
Copy link
Member

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.

@riccardobl
Copy link
Member Author

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.

@stephengold
Copy link
Member

Another thing: package names use only lowercase letters and digits (no underscores). See the Google Java Style Guide section 5.2.1.

@stephengold
Copy link
Member

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?

@riccardobl
Copy link
Member Author

riccardobl commented Sep 6, 2023

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.

@Ali-RS
Copy link
Member

Ali-RS commented Sep 7, 2023

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,...)

@Ali-RS
Copy link
Member

Ali-RS commented Sep 7, 2023

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.

* An extension for the GLTF loader that loads speaker nodes.
* (This extension requires the jme-extras addon for blender)
*/
public class SpeakerExtensionLoader implements ExtensionLoader {
Copy link
Member

@Ali-RS Ali-RS Sep 7, 2023

Choose a reason for hiding this comment

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

I am fine with SpeakerExtensionLoader naming, however, I think something like AudioNodeExtensionLoader or AudioExtensionLoader would be nicer, as in JME, I believe we do not use the term "Speaker".

@Ali-RS
Copy link
Member

Ali-RS commented Sep 7, 2023

Next, we can have a gltf.ext.jme package 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!

@stephengold
Copy link
Member

stephengold commented Sep 7, 2023

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.

@riccardobl
Copy link
Member Author

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.

@stephengold
Copy link
Member

Does the size of the PR reflects on the quality of the code?

No.

@riccardobl riccardobl marked this pull request as draft October 14, 2023 12:43
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.

3 participants