jMonkeyEngine / jmonkeyengine

A complete 3-D game development suite written in Java.
http://jmonkeyengine.org
BSD 3-Clause "New" or "Revised" License
3.74k stars 1.12k forks source link

GltfLoader issues #2277

Open capdevon opened 1 month ago

capdevon commented 1 month ago

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)

stephengold commented 1 month ago

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

capdevon commented 1 month ago

Maybe this: https://github.com/jMonkeyEngine/jmonkeyengine/pull/2103

stephengold commented 1 month ago

Would reverting 2103 solve this issue?

capdevon commented 1 month ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago
  1. Nested Geometry: Each geometry is now encapsulated within two parent nodes, which can complicate scene hierarchy management.
    1. 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 https://github.com/jMonkeyEngine/jmonkeyengine/pull/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 commented 2 weeks ago

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