applySkeleton does not chk if mesh has skeleton

Its really minor, I think. Just to be sure tho…

Repro: https://playground.babylonjs.com/#NMU4ZM#76

comment/uncomment line 11 and you’ll find that applySkeleton does not throw an error if the cloned mesh has no skeleton. I was expecting smthg in console, threw me off a little.

1 Like

@bghgary did a recent change where now when we clone a mesh we clone the skeleton too :slight_smile: Fix issue with glTF skeletons by bghgary · Pull Request #11919 · BabylonJS/Babylon.js (github.com)

2 Likes

Yes, cloning a mesh will now clone assign the skeleton as well.

EDIT: Cloning a mesh doesn’t clone the skeleton, it just assign to the same one.

1 Like

Am a little confused. Is this just for glTF? Seems like a lot of files changed for just that. If that is now the case for any mesh, I’ll need to move a line in my exporter & regenerate source classes. I am / was overriding that behavior and supplying a skeleton already.


Edit: After looking at the Mesh file of the PR, yes it is all meshes with a skeleton. Looking at skeleton.clone(), I do not want to do that. I have my own Skeleton & bone sub-classes. That code creates BABYLON.Skeletons & Bones. Just going to keep same code & throw that away, then continue to call the source function to create a fresh sub-classed skeleton.

Can I just let the thing go out of scope, or do I need to dispose of it?

Hmm, we were thinking this is a bug since cloning should clone everything and not skip skeletons. Would it help if we added a flag to skip cloning the skeleton?

Actually what I did in 2015, was PR a change to Mesh, so that the source of the clone & cloning was done in the constructor. This enabled Mesh sub-classing much like the rest of the language planet. Many languages allow multiple constructors (JS is crap). A clone() function outside the constructor is hostile to sub-classing.

I realize that I am strangely the edge case?? If you did anything, putting a source arg in Skeleton, & Bone constructors would be it.

Here is Mesh constructor code I currently generate

class HumanMale extends QI_MH.Automaton {
    constructor(name, scene, source) {
        super(name, scene, null, source, true);

        const cloning = source && source !== null;
        this.skeleton = skel_reference(name, scene);
        this.numBoneInfluencers = 6;


        this.id = this.name;
        this.isPickable = false;
        if (!cloning){
            // Assign copies of donor VertexData, not donor.applyToMesh(), so dispose will not affect the donor.
            // Also, appyToMesh() has a updatable arg.  Only want it true for positions & normals, and only when shapekeys for mesh.
            let donor = RefMale.VertexData["HumanMale"];
            this.setVerticesData(BABYLON.VertexBuffer.PositionKind, donor.positions.slice(), true);
            this.setIndices(donor.indices.slice());

            this.setVerticesData(BABYLON.VertexBuffer.NormalKind, donor.normals.slice(), true);
            this.setVerticesData(BABYLON.VertexBuffer.UVKind, donor.uvs.slice(), false);
            this.setVerticesData(BABYLON.VertexBuffer.MatricesWeightsKind, donor.matricesWeights.slice(), false);

            this.setVerticesData(BABYLON.VertexBuffer.MatricesIndicesKind, QI.UNPACK(donor.matricesIndices), false);

            this.setVerticesData(BABYLON.VertexBuffer.MatricesWeightsExtraKind, donor.matricesWeightsExtra.slice(), false);

            this.setVerticesData(BABYLON.VertexBuffer.MatricesIndicesExtraKind, QI.UNPACK(donor.matricesIndicesExtra), false);

            this.setMaterialByID("RefMale.Multimaterial#0");
            this.subMeshes = [];
            new BABYLON.SubMesh(0, 0, 15850, 0, 75036, this);
            new BABYLON.SubMesh(1, 15850, 590, 75036, 2826, this);
            new BABYLON.SubMesh(2, 16440, 609, 77862, 2406, this);
            new BABYLON.SubMesh(3, 17049, 96, 80268, 516, this);
            new BABYLON.SubMesh(4, 17145, 5516, 80784, 21360, this);
            new BABYLON.SubMesh(5, 22661, 303, 102144, 1344, this);

            // assign shape key data
            this.assignShapeKeyDict(RefMale.MorphData["HumanMale"]);
        }
        if (this.postConstruction) this.postConstruction();
        this.makeVisible(false);
        this.onMeshReadyObservable.add(() => {
            // an indirect test for being a QI.Mesh subclass
            if (typeof this.grandEntrance === "function") this.grandEntrance();
            else this.isVisible = true;
        });

    }

    dispose(doNotRecurse) {
        super.dispose(doNotRecurse);
        if (this.skeleton) this.skeleton.dispose();
    }
}
RefMale.HumanMale = HumanMale;

I can just put a if(this.skeleton) this.skeleton.dispose; on a line before I assign it on the next line. I am just glad I caught it. I am probably going to need hours to go through all lines in changes.md when 5.0 comes out.

I just looked at the code again and I misspoke. We don’t clone the skeleton, but we now assign the skeleton. Before, it would leave the skeleton property unassigned.

I think for your case, it should behave the same as before since you are replacing the skeleton property anyways.

3 Likes

ok, understood. I guess this means the doc needs an update. Marking as solved. Thanks everyone!

1 Like

Where does the doc need an update?

I suppose the section on cloning should have a little footnote "as of ver xxx: cloning a mesh with skeleton will assign src's skeleton to the clone blah blah blah...". smthg along these lines.

1 Like

Something like this? https://github.com/BabylonJS/Babylon.js/blob/master/what’s%20new.md?plain=1#L453

1 Like

erm, I meant this doc: Clones | Babylon.js Documentation

This is a page with the least words on a topic that is often used, lol… Anyways, the meaning in what’s new is a little off. Cloning a mesh now copies skeletons. would mean 2 skeletons would be present in scene when a mesh gets cloned. When in actual case, there is only 1 skeleton shared between N meshes for N clones.

Yes, I can update the what’s new. Would you like to update the documentation?

Done, PR is up! Cheers ! :slight_smile:

2 Likes