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

GltfLoader issues #2277

Closed
capdevon opened this issue Jun 1, 2024 · 18 comments
Closed

GltfLoader issues #2277

capdevon opened this issue Jun 1, 2024 · 18 comments
Milestone

Comments

@capdevon
Copy link
Contributor

capdevon commented Jun 1, 2024

The recent changes to the GLTF loader have introduced several problems:

  1. Duplicate Controllers: The new loader creates duplicate AnimComposer and SkinningControl controllers for each geometry in the model. (see here)
  2. Nested Geometry: Each geometry is now encapsulated within two parent nodes, which can complicate scene hierarchy management.
  3. Backward Compatibility: These changes may cause compatibility issues with existing code that relies on the previous behavior of the GLTF loader. (see here)

Other issues reported by adi.barda: (see here)

  • Blured colors - maybe normal map is missing?
  • Sometime missing animations - maybe related to 1
@stephengold
Copy link
Member

Please provide detailed instructions for reproducing this issue.
Has anyone identified which recent changes caused this issue?

@capdevon
Copy link
Contributor Author

capdevon commented Jun 1, 2024

Maybe this: #2103

@stephengold
Copy link
Member

Would reverting 2103 solve this issue?

@capdevon
Copy link
Contributor Author

capdevon commented Jun 1, 2024

Would reverting 2103 solve this issue?

Probably yes.

Please provide detailed instructions for reproducing this issue.

Here is the 3D Model for the test file

jME-3.7.0
image

jME-3.6.1-stable
image

@stephengold
Copy link
Member

Using a simple test app, I showed that the issue of YBot loading with multiple AnimComposers pre-dates PR 2103. I'll do a bisection search to find where the issue was introduced.

Here is the test app:

import com.jme3.anim.AnimComposer;
import com.jme3.app.SimpleApplication;
import com.jme3.scene.Node;
import com.jme3.scene.Spatial;
import com.jme3.scene.control.Control;
import com.jme3.scene.plugins.gltf.GltfModelKey;
import java.util.List;

public class TestYBot extends SimpleApplication {

    public static void main(String[] args) {
        TestYBot app = new TestYBot();
        app.start();
    }

    @Override
    public void simpleInitApp() {
        GltfModelKey modelKey = new GltfModelKey("YBot.gltf");
        Spatial modelRoot = assetManager.loadModel(modelKey);
        int numComposers = countControls(modelRoot, AnimComposer.class);
        System.out.println("numComposers = " + numComposers);
        stop();
    }

    /**
     * Count all controls of the specified type in the specified subtree of a
     * scene graph. Note: recursive!
     *
     * @param <T> subclass of Control
     * @param subtree the subtree to analyze (may be null, unaffected)
     * @param controlType the subclass of Control to search for
     * @return the count (&ge;0)
     */
    private static <T extends Control> int countControls(
            Spatial subtree, Class<T> controlType) {
        int result = 0;

        if (subtree != null) {
            int numControls = subtree.getNumControls();
            for (int controlI = 0; controlI < numControls; ++controlI) {
                Control control = subtree.getControl(controlI);
                if (controlType.isAssignableFrom(control.getClass())) {
                    ++result;
                }
            }
        }

        if (subtree instanceof Node) {
            Node node = (Node) subtree;
            List<Spatial> children = node.getChildren();
            for (Spatial child : children) {
                result += countControls(child, controlType);
            }
        }

        return result;
    }
}

@stephengold
Copy link
Member

The issue was introduced by PR #2060.

With "master" branch at f915e56, the test app prints numComposers = 1.
With "master" branch at 4c87531, the test app prints numComposers = 2.

@riccardobl
Copy link
Member

  1. Nested Geometry: Each geometry is now encapsulated within two parent nodes, which can complicate scene hierarchy management.
  2. Backward Compatibility: These changes may cause compatibility issues with existing code that relies on the previous behavior of the GLTF loader. (see here)

As pointed out in the pr #2103 , this is to solve several issue and inconsistencies with the scenegraph of imported models , it is not backward compatible, but the previous behavior was bugged and source of issues

@stephengold
Copy link
Member

The current behavior, with multiple anim composers per model, is clearly bugged. And that change appears to be unrelated to PR 2103.

JNightRider pushed a commit to JNightRider/jmonkeyengine that referenced this issue Sep 2, 2024
stephengold pushed a commit that referenced this issue Sep 4, 2024
* fix bug #2277

* update

* script error

* module implementation

* corrections...

---------

Co-authored-by: wil <wil@debian>
@stephengold
Copy link
Member

I think there's just one unsolved piece to this issue: the "blurred colors" change, which occurred between v3.6.1-stable and v3.7.0-beta1.2.2.
@scenemax3d sent me a copy of his test app, which I've now run with both releases. I hope to use a bisection search to identify the commit(s) that caused the change.

@stephengold
Copy link
Member

I simplified the test app so it doesn't require Minie:

package jme3test;

import com.jme3.app.SimpleApplication;
import com.jme3.light.AmbientLight;
import com.jme3.light.DirectionalLight;
import com.jme3.math.ColorRGBA;
import com.jme3.math.Quaternion;
import com.jme3.math.Vector3f;
import com.jme3.scene.Node;
import com.jme3.scene.Spatial;
import com.jme3.shadow.DirectionalLightShadowRenderer;
import com.jme3.system.AppSettings;

public class TestApp extends SimpleApplication {

    public static void main(String[] args) {
        TestApp app = new TestApp();
        app.showSettings = false;
        AppSettings settings = new AppSettings(true);

        settings.setWidth(1024);
        settings.setHeight(768);
        settings.setGammaCorrection(false);
        settings.setSamples(4); // anti-aliasing
        settings.setVSync(true);
        app.setSettings(settings);
        app.start(); // start the game
    }

    @Override
    public void simpleInitApp() {
        flyCam.setDragToRotate(true);
        cam.setLocation(new Vector3f(5.809729f, 3.3733122f, 4.834249f));
        cam.setRotation(new Quaternion(0.0018560609f, 0.9064233f, 0.0039831875f, -0.42234766f));

        loadModel1();
        addLighting();
    }

    private void addLighting() {
        DirectionalLight sun = new DirectionalLight();
        sun.setColor(ColorRGBA.White);
        sun.setDirection(new Vector3f(-0.1f, -0.7f, -1.0f));
        rootNode.addLight(sun);

        final int SHADOWMAP_SIZE = 1024;
        DirectionalLightShadowRenderer dlsr = new DirectionalLightShadowRenderer(assetManager, SHADOWMAP_SIZE, 3);
        dlsr.setLight(sun);
        viewPort.addProcessor(dlsr);

        AmbientLight al = new AmbientLight();
        al.setColor(ColorRGBA.White.mult(0.3f));
        al.setEnabled(true);
        rootNode.addLight(al);
    }

    private void loadModel1() {
        Spatial mm = assetManager.loadModel("Models/boss/boss.gltf");
        final Node model = new Node();
        model.attachChild(mm); // add it to the wrapper
        mm.scale(3f, 3f, 3f);
        rootNode.attachChild(model);
    }
}

@stephengold
Copy link
Member

The "blurred colors change" had already occurred as of efbcd03 (29 May 2023 in the "master" branch), which means it wasn't related to PR #2060.

It hadn't occurred yet as of fec08b3 (11 Feb 2023 in the "master" branch). So now I've got a 15-week window to search...

@stephengold
Copy link
Member

stephengold commented Oct 19, 2024

The "blurred colors" change resulted from integrating PR #1980 into "master" branch on 11 March 2023. The colors are sharp at f51681a but not at f771f9b.

@stephengold
Copy link
Member

Here's how the test model looked in v3.6.1-stable:
image

And here's how it looks at 5f54eb2 (current "master" branch):
image

I don't understand the shader code in PR #1980, so I can't say whether it's correct or not. However, I believe the appearance of the test model with v3.7.0-beta1.2.2 is actually more correct than with v3.6.1-stable. In support of this belief, here is how the model looks in the glTF viewer at https://gltf-viewer.donmccurdy.com/ :

image

Note the smoothness of the skin on the model's face and arms. Allowing for differences in lighting and perspective, McCurdy's viewer renders the model a lot like JME's "master" branch and much less like JME v3.6.1-stable . I suspect this is because v3.6.1-stable ignores glTF texture scaling.

The test model makes extensive use of texture scaling. For instance, the "Skin_MAT" material is defined in the glTF as:

            "doubleSided" : true,
            "name" : "Skin_MAT",
            "normalTexture" : {
                "index" : 0,
                "scale" : 0.1400168091058731,
                "texCoord" : 0
            },
            "pbrMetallicRoughness" : {
                "baseColorTexture" : {
                    "index" : 1,
                    "texCoord" : 0
                },
                "metallicFactor" : 0,
                "roughnessFactor" : 0.6278957724571228
            }

In the current "master" branch, if I increase the scaling of the Skin_MAT normal texture (from 0.1400168091058731) to 1.0, the model's arms look about like they did in v3.6.1-stable:

image

which is what one would expect if v3.6.1-stable were simply ignoring texture scaling.

@scenemax3d:
The bottom line: I believe the "blurred colors change" is a feature and an improvement, not a regression. If there are no other serious issues with the v3.7 branch, then it's ready for a stable release.

@stephengold
Copy link
Member

Also, this issue should probably be closed.

@scenemax3d
Copy link
Contributor

@stephengold
I think there are much more details in the v3.6 version - the muscles, the clothing, face, everything but maybe as you say being it viewed more detailed (v3.6) is a bug? and the way it is rendered in v3.7 is actually the correct way..
Now , thanks to your investigation, we know the commit that caused this feature-bug and we can discuss it further later.
I will move v3.7 to stable ASAP

@stephengold
Copy link
Member

@scenemax3d I would describe the enhanced details in v3.6 renders as a defect: those details don't reflect the intent of the artist who created the model.

If for your games you desire a model with exaggerated wrinkles, veins, and muscles, you can easily edit the glTF file in a text editor to tune the scaling of the relevant normal texture(s).

scenemax3d pushed a commit that referenced this issue Oct 21, 2024
* fix bug #2277

* update

* script error

* module implementation

* corrections...

---------

Co-authored-by: wil <wil@debian>
@capdevon
Copy link
Contributor Author

Thank you for your efforts guys. Let me know if I can close this Issue.

@stephengold stephengold added this to the v3.7.0 milestone Oct 22, 2024
@stephengold
Copy link
Member

Like I said:

this issue should probably be closed.

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

No branches or pull requests

4 participants