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

SkinnedMesh: support weight normalization for BufferGeometry #7679

Merged
merged 3 commits into from
Nov 26, 2015

Conversation

WestLangley
Copy link
Collaborator

Fixes #7111 and #7197.

/ping @titansoftime Please have a look.


}

skinWeight.setXYZ( i, vec.x, vec.y, vec.z, vec.w )
Copy link
Owner

Choose a reason for hiding this comment

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

setXYZW() maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops... Good eyes! : - )

@titansoftime
Copy link
Contributor

It seems the // do nothing are still necessary. Without sw.set(1) and vec.set(1) I get some fairly odd visuals:

I will get a fiddle of this up soon.

With vec.set(1):
set

Without:
donothing

@WestLangley
Copy link
Collaborator Author

Please see if you can track down what is going on with your models via the debugger.

Vector4.set() takes 4 args, not 1.

@titansoftime
Copy link
Contributor

Here's the fiddle: http://jsfiddle.net/cndoe3re/

The set(1) was just what I took from the original function.

THREE.SkinnedMesh.prototype.normalizeSkinWeights = function () {

    if ( this.geometry instanceof THREE.Geometry ) {

        for ( var i = 0; i < this.geometry.skinIndices.length; i ++ ) {

            var sw = this.geometry.skinWeights[ i ];

            var scale = 1.0 / sw.lengthManhattan();

            if ( scale !== Infinity ) {

                sw.multiplyScalar( scale );

            } else {

                sw.set( 1 ); // this will be normalized by the shader anyway

            }

        }

    } else {

        // skinning weights assumed to be normalized for THREE.BufferGeometry

    }

};

EDIT* More info:

This url: http://www.titansoftime.com/webgl/Three74devb.full.js is used in http://jsfiddle.net/he21xL1p/ with the following function:

THREE.SkinnedMesh.prototype.normalizeSkinWeights = function () {

    if ( this.geometry instanceof THREE.Geometry ) {

        for ( var i = 0; i < this.geometry.skinWeights.length; i ++ ) {

            var sw = this.geometry.skinWeights[ i ];

            var scale = 1.0 / sw.lengthManhattan();

            if ( scale !== Infinity ) {

                sw.multiplyScalar( scale );

            } else {

                sw.set( 1 );

            }

        }

    } else if ( this.geometry instanceof THREE.BufferGeometry ) {

        var vec = new THREE.Vector4();

        var skinWeight = this.geometry.attributes.skinWeight;

        for ( var i = 0; i < skinWeight.count; i ++ ) {

            vec.x = skinWeight.getX( i );
            vec.y = skinWeight.getY( i );
            vec.z = skinWeight.getZ( i );
            vec.w = skinWeight.getW( i );

            var scale = 1.0 / vec.lengthManhattan();

            if ( scale !== Infinity ) {

                vec.multiplyScalar( scale );

            } else {

                vec.set( 1 );

            }

            skinWeight.setXYZ( i, vec.x, vec.y, vec.z, vec.w )

        }

    }

};

@WestLangley
Copy link
Collaborator Author

Like I said, Vector4.set( 1 ) is not a proper use of the method. Please see if you can figure out what is going on with your models. I expect you have all-zero weights in places.

@titansoftime
Copy link
Contributor

I am not the one who originally put in set(1), I'm not sure who wrote that, I simply copied it to your BufferGeometry section of the function.

I've had this model since r58 and it has been fine until now. So I highly doubt it's the model. I am not a modeler or nor an animator so any investigation on my part would be useless. I can tell you that the weights are not all 0.

http://www.titansoftime.com/utils.php?task=getModel&id=121

http://jsfiddle.net/he21xL1p/ works with the set(1) and http://jsfiddle.net/cndoe3re/ does not work without it. The function originally had the set(1), it's not of my doing, but it seems to work just fine. It's absence yields breakage. Perhaps the original author of the function THREE.SkinnedMesh.prototype.normalizeSkinWeights could shed some light on why they used sw.set( 1 ). sw is a Vector4 of course.

I'm just saying that what he/she did appears to have a reason though it's missing 3 args. Evidence being it works with it and not without it. I'm just the messenger =]

@titansoftime
Copy link
Contributor

lol @ // do something reasonable. Works great!

Something was going on other than skin weights of 0 though. You can see here that the weights are not 0:

http://www.titansoftime.com/utils.php?task=getModel&id=121

Regardless, all seems well. Thank you for your efforts =]

@WestLangley
Copy link
Collaborator Author

I expect you have all-zero weights in places.

And you do. Some vertices have all 4 weights equal to zero.

Also, I do not think you give yourself enough credit. I would not have asked for your help if I felt investigation on your part "would be useless" or you were "just a messenger".

@titansoftime
Copy link
Contributor

Some vertices have all 4 weights equal to zero.

Ah, I thought you were saying all weight at 0. Gotcha. A big "duh" on that lol. I assumed js worked like php and would halt on division by 0. Just when you think you know javascript...

Also, I do not think you give yourself enough credit.

That means a lot, thank you. I feel like you guys are beings from the 5th dimension when it comes to this kind of stuff though =]

mrdoob added a commit that referenced this pull request Nov 26, 2015
SkinnedMesh: support weight normalization for BufferGeometry
@mrdoob mrdoob merged commit e758065 into mrdoob:dev Nov 26, 2015
@mrdoob
Copy link
Owner

mrdoob commented Nov 26, 2015

Many thanks guys!

@titansoftime titansoftime mentioned this pull request Nov 26, 2015
@WestLangley WestLangley deleted the dev-skinning branch November 26, 2015 05:57
@MasterJames
Copy link
Contributor

As a side note every single example anyone has posted since well before I pointed out a month or two ago - continue to show up black ( on Samsung Tab3 chrome current browser ), but I thought we already know that and have addressed it so it must be that people are using older versions or something. Is this expected or did that fix not go through yet?

@titansoftime
Copy link
Contributor

The model is black on my tablet due to the fact that I am loading a dds texture which is not supported on my samsung galaxy 4. I suppose I should have just used a png for examples sake.

Or, are you saying the entire scene is black?

@MasterJames
Copy link
Contributor

Oh I see that explains this one. No it was just the model. The problem before was in the lighting and/or shader and nothing to do with models. I would have expected some visible shading even if the texture didn't load. I see a flat black palm and solid bright green floor on a white BG.

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.

4 participants