mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
101.7k stars 35.3k forks source link

r72 : .computeTangents() has been removed #7190

Closed RemusMar closed 8 years ago

RemusMar commented 8 years ago

r71: perfect http://necromanthus.com/Test/html5/SMC.html

r72: a mess http://necromanthus.com/Test/html5/SMC_r72.html

No other comments.

bhouston commented 8 years ago

What were you using Tangents for may I ask? It isn't clear to me with a quick look. It seems like whole objects are missing frmo your scene and also that some material brightnesses are completely out of whack.

RemusMar commented 8 years ago

What were you using Tangents for may I ask? It isn't clear to me with a quick look.

I didn't use it. .computeTangents() is needed by the SEA3D loader. But the main question is why it has been removed ???

sunag commented 8 years ago

Hi @RemusMar

Have you tried it with SEA3D 1.7v? https://github.com/sunag/sea3d/releases/tag/v1.7-r1

Also have the SEA3D TJS version optimized for Three.JS. http://community.poonya.com/160 example http://threejs.org/examples/#webgl_loader_sea3d_hierarchy

Cheers.

sunag commented 8 years ago

SEA3DLoader for Three.JS r72, only TJS files https://github.com/mrdoob/three.js/tree/master/examples/js/loaders/sea3d

bhouston commented 8 years ago

@sunag btw I think that the Sea3DLoader would benefit a lot if it was to load into the new Unity3D-like AnimationMixer set of classes for its animations. This PR has the code and also I've converted over most examples to use it -- it replaces Animation/AnimationHandler/etc.:

https://github.com/mrdoob/three.js/pull/6934

RemusMar commented 8 years ago

SEA3DLoader for Three.JS r72, only TJS files

With that you cannot even load the SEA file. You'll get this error:

"SEA3D 1.7.0.0" SEA3D.js "THREE.WebGLRenderer" "72" three.min_72.js uncaught exception: Sign 'S3D' not supported! Use SEA3D Studio to export. TypeError: this.container is undefined SEA3DLoader.js

Another issue: the latest minified SEA3D loader does not include DEFLATE ... !?! cheers

RemusMar commented 8 years ago

@sunag btw I think that the Sea3DLoader would benefit a lot if it was to load into the new Unity3D-like AnimationMixer set of classes for its animations.

Ben, SEA3D is already the best THREE loader by far. Talking about animations, it supports animation names and interpolation (animation blending). http://necromanthus.com/Test/html5/testA.html Just get closer to that dog and pay attention to the smooth transitions. cheers

bhouston commented 8 years ago

@RemusMar Animation names, blending, warping is supported by the AnimationMixer/AnimationClip system - including the ability to animate material properties, skinning, node properties, and morph targets, and blend shapes. I think the new animation system can easily be used by the Sea3D loader: https://github.com/mrdoob/three.js/pull/6934 So I am suggesting an improvement to Sea3D, not denigrating it in any way.

RemusMar commented 8 years ago

@RemusMar Animation names, blending, warping is supported by the AnimationMixer/AnimationClip system - including the ability to animate material properties, skinning, node properties, and morph targets, and blend shapes.

Are they included into the main package? Or are you talking about "examples" and external scripts?

model.play('walk', 0.5); 500 miliseconds interpolation between the current animation and the "walk" one. This is possible with SEA3D only. cheers

bhouston commented 8 years ago

Are they included into the main package? Or are you talking about "examples" and external scripts?

The PR is for inclusion of the new animation system within the core of ThreeJS: https://github.com/mrdoob/three.js/pull/6934

model.play('walk', 0.5); 500 miliseconds interpolation between the current animation and the "walk" one. This is possible with SEA3D only.

The new animation system supports cross fade between animation clips as well as warps (where it slowly adjusts the length of the cycles to match whlie doing the cross fade between the animations):

THREE.AnimationMixer.crossfade( fromAnimName, toAnimName, duration );
THREE.AnimationMixer.warp( fromAnimName, toAnimName, duration );

It was because the functionality of the new animation system matches the needs of Sea3D, I was suggesting that it also adopt it, rather than maintaining a separate but similar animation system.

RemusMar commented 8 years ago

THREE.AnimationMixer.crossfade( fromAnimName, toAnimName, duration );

A not very inspired syntax. Why do you need "fromAnimName"?

p.s. model.play('walk', 0.5); is the best approach by far.

bhouston commented 8 years ago

A not very inspired syntax.

I have a play function as well called mixer.play takes a duration for the fade in time. I'll add an option to schedule everything else to fade out at the same time -- thus it will cover your use case exactly:

https://github.com/mrdoob/three.js/pull/6934/files#diff-c21f40104b3fb390702d00a32f62cdb6R133

model.play('walk', 0.5); is the best approach by far.

I'll modify the code now to have the optional fade out of everything else using this exactly syntax.

Why do you need "fromAnimName"?

THREE.AnimationMixer is a general mixer and can support mixing together any number of animations -- such as facial animations, multiple bone animations, etc. So you can have a shoot animation for the arms bones, while fading from walk to run on the leg bones -- because it supports any number of concurrent animations, you likely do not just want to fade out all of them in favor of a new one -- thus the mixer itself needs to know which to fadeOut, especially if you are doing a warp.

There is also fadeIn, fadeOut, play, warp etc functions -- so you can full control over what happens.

Also, Sea3D doesn't have to change the UI they are providing to you as a user of the Sea3D classes, rather they could use the new animation mixer behind the scenes because it does more than their current animation system and it will reduce the amount of JavaScript required to download to run Sea3D scenes.

mrdoob commented 8 years ago

This is getting off-topic...

I think the main issue is that @remoe is not happy with the new SEA3D loader.

sunag commented 8 years ago

Hi

@RemusMar it is necessary to export from SEA3D Studio in File -> Publish... and choose Three.JS as output. The file turns with a TJS tag.

About the changes, my idea is to simplify the game creation process for increased productivity, for advanced users they can create their own characteristics over SEA3D. The interface of SEA3D was based on the use in our company and it has worked for several games, if migrating from Flash, Director for THREE will also be easier.

We have examples of animation Basics and Advanced here: http://sunag.github.io/sea3d/ http://threejs.org/examples/#webgl_loader_sea3d_keyframe crossfade, fades, timeScale, warp as offset, all this already have in sea3d animation.

Cheers

mrdoob commented 8 years ago

@sunag I think @remoe just wants to export from 3D Studio...?

sunag commented 8 years ago

@mrdoob @remoe = @RemusMar ?

another user requested me a TJS from the 3ds max exporter too, at the moment just from sea3d studio :( this is because has some things still pending. If all goes well until next month.

mrdoob commented 8 years ago

@mrdoob @remoe = @RemusMar ?

Oops! Silly Github auto-completion.

RemusMar commented 8 years ago

I think the main issue is that @RemusMar is not happy with the new SEA3D loader.

.computeTangents() has been removed Why?

@RemusMar it is necessary to export from SEA3D Studio in File -> Publish...

I don't want to REexport anything. On the internet you can find thousands of SEA files. Are you going to REexport them all?

A tip to all the software developers: THE BACKWARD COMPATIBIL.ITY IS A MUST. Otherwise less and less people will be interested in your work. cheers

p.s. This topic is not a question.

mrdoob commented 8 years ago

@RemusMar can you try adding this to your code?

THREE.BufferGeometry.prototype.computeTangents = function () {

    var index = this.index;
    var attributes = this.attributes;

    // based on http://www.terathon.com/code/tangent.html
    // (per vertex tangents)

    if ( index === null ||
         attributes.position === undefined ||
         attributes.normal === undefined ||
         attributes.uv === undefined ) {

        console.warn( 'THREE.BufferGeometry: Missing required attributes (index, position, normal or uv) in BufferGeometry.computeTangents()' );
        return;

    }

    var indices = index.array;
    var positions = attributes.position.array;
    var normals = attributes.normal.array;
    var uvs = attributes.uv.array;

    var nVertices = positions.length / 3;

    if ( attributes.tangent === undefined ) {

        this.addAttribute( 'tangent', new THREE.BufferAttribute( new Float32Array( 4 * nVertices ), 4 ) );

    }

    var tangents = attributes.tangent.array;

    var tan1 = [], tan2 = [];

    for ( var k = 0; k < nVertices; k ++ ) {

        tan1[ k ] = new THREE.Vector3();
        tan2[ k ] = new THREE.Vector3();

    }

    var vA = new THREE.Vector3(),
        vB = new THREE.Vector3(),
        vC = new THREE.Vector3(),

        uvA = new THREE.Vector2(),
        uvB = new THREE.Vector2(),
        uvC = new THREE.Vector2(),

        sdir = new THREE.Vector3(),
        tdir = new THREE.Vector3();

    function handleTriangle( a, b, c ) {

        vA.fromArray( positions, a * 3 );
        vB.fromArray( positions, b * 3 );
        vC.fromArray( positions, c * 3 );

        uvA.fromArray( uvs, a * 2 );
        uvB.fromArray( uvs, b * 2 );
        uvC.fromArray( uvs, c * 2 );

        var x1 = vB.x - vA.x;
        var x2 = vC.x - vA.x;

        var y1 = vB.y - vA.y;
        var y2 = vC.y - vA.y;

        var z1 = vB.z - vA.z;
        var z2 = vC.z - vA.z;

        var s1 = uvB.x - uvA.x;
        var s2 = uvC.x - uvA.x;

        var t1 = uvB.y - uvA.y;
        var t2 = uvC.y - uvA.y;

        var r = 1.0 / ( s1 * t2 - s2 * t1 );

        sdir.set(
            ( t2 * x1 - t1 * x2 ) * r,
            ( t2 * y1 - t1 * y2 ) * r,
            ( t2 * z1 - t1 * z2 ) * r
        );

        tdir.set(
            ( s1 * x2 - s2 * x1 ) * r,
            ( s1 * y2 - s2 * y1 ) * r,
            ( s1 * z2 - s2 * z1 ) * r
        );

        tan1[ a ].add( sdir );
        tan1[ b ].add( sdir );
        tan1[ c ].add( sdir );

        tan2[ a ].add( tdir );
        tan2[ b ].add( tdir );
        tan2[ c ].add( tdir );

    }

    var groups = this.groups;

    if ( groups.length === 0 ) {

        groups = [ {
            start: 0,
            count: indices.length
        } ];

    }

    for ( var j = 0, jl = groups.length; j < jl; ++ j ) {

        var group = groups[ j ];

        var start = group.start;
        var count = group.count;

        for ( var i = start, il = start + count; i < il; i += 3 ) {

            handleTriangle(
                indices[ i + 0 ],
                indices[ i + 1 ],
                indices[ i + 2 ]
            );

        }

    }

    var tmp = new THREE.Vector3(), tmp2 = new THREE.Vector3();
    var n = new THREE.Vector3(), n2 = new THREE.Vector3();
    var w, t, test;

    function handleVertex( v ) {

        n.fromArray( normals, v * 3 );
        n2.copy( n );

        t = tan1[ v ];

        // Gram-Schmidt orthogonalize

        tmp.copy( t );
        tmp.sub( n.multiplyScalar( n.dot( t ) ) ).normalize();

        // Calculate handedness

        tmp2.crossVectors( n2, t );
        test = tmp2.dot( tan2[ v ] );
        w = ( test < 0.0 ) ? - 1.0 : 1.0;

        tangents[ v * 4 ] = tmp.x;
        tangents[ v * 4 + 1 ] = tmp.y;
        tangents[ v * 4 + 2 ] = tmp.z;
        tangents[ v * 4 + 3 ] = w;

    }

    for ( var j = 0, jl = groups.length; j < jl; ++ j ) {

        var group = groups[ j ];

        var start = group.start;
        var count = group.count;

        for ( var i = start, il = start + count; i < il; i += 3 ) {

            handleVertex( indices[ i + 0 ] );
            handleVertex( indices[ i + 1 ] );
            handleVertex( indices[ i + 2 ] );

        }

    }

};
mrdoob commented 8 years ago

As per your question... The reason computeTangents() was removed was because the built-in materials/shaders no longer required tangents.

DVLP commented 8 years ago

That's also a problem which I've encountered when migrating to r72 today. Temporarily adding

THREE.BufferGeometry.prototype.computeTangents = function () {

var index = this.index;
var attributes = this.attributes; .....

to my code too until sea3d exporter for 3ds max will be compatible with r72

also adding:

THREE.Geometry.prototype.computeTangents = function () {};

just to mute the warnings.

Will it make any difference if instead of including full body of THREE.BufferGeometry.prototype.computeTangents function I'll just mute warnings too like below?

THREE.BufferGeometry.prototype.computeTangents = function () {};
RemusMar commented 8 years ago

@RemusMar can you try adding this to your code?

It doesn't change anything. Less warnings but the same messed up result. You can try it by yourself.

mrdoob commented 8 years ago

You can try it by yourself.

How? Do you mind zipping the test so I can investigate locally?

RemusMar commented 8 years ago

How? Do you mind zipping the test so I can investigate locally?

Ricardo, if you take a look at the page source you'll find: http://necromanthus.com/Test/html5/SMC_r72.html http://necromanthus.com/Test/html5/maps/SMC.sea http://necromanthus.com/Test/html5/JS/FPScount.js http://necromanthus.com/Test/html5/JS/sea3d.min.js http://necromanthus.com/Test/html5/JS/three.min_72.js "Save Link As ..."

sunag commented 8 years ago

@mrdoob @RemusMar I will be investigating this problem, It's probably something in sea3d loader.

mrdoob commented 8 years ago

@sunag thanks!

sunag commented 8 years ago

I found the two problems he was generating this.

Lightmap moved of multiply to additive, is necessary to use aoMap for the same purpose and BufferGeometry loader also had problems. I am sending a fix.

https://fhscript.com/12jP/sea3d.min.js

Still I will clean up for a update in root.

@RemusMar Your lightmap is inverted with diffuseMap. It will no longer work in r72.

diffuse-lightmap

Cheers.

RemusMar commented 8 years ago

I am sending a fix. https://fhscript.com/12jP/sea3d.min.js

I didn't find any fix.

Still I will clean up for a update in root.

The same problem with the minified version: DEFLATE is not included !?!

@RemusMar Your lightmap is inverted with diffuseMap. It will no longer work in r72.

In r71 if it's not inverted, it doesn't work !!

sunag commented 8 years ago

The same problem with the minified version: DEFLATE is not included !?!

The lib I sent you have deflate.

In r71 if it's not inverted, it doesn't work !!

This was because the UV1 and UV2 and textures is swapped in your original file (3ds max I suppose).

I didn't find any fix.

Did you get to test the file? I managed to fix this in three steps.

  1. Download the fix and replaces the previous sea3d.min.js.
  2. Swap your geometry "sand" UV1 to UV2. I did it in SEA3D Studio. swapuv
  3. Swap your diffuse to lightmap. swaplightmap

Result smc

I particularly recommend the TJS version, for that is optimized for Three.JS and you will find the latest updates. At the beginning of next month I will be posting the update SEA3DLegacyLoader.js In which case it would be this version of the loader. but legacy.js is not our priority at the time.

Cheers.

RemusMar commented 8 years ago

I am sending a fix. https://fhscript.com/12jP/sea3d.min.js The lib I sent you have deflate.

Again, that file cannot be loaded (it does not exist).

At the beginning of next month I will be posting the update SEA3DLegacyLoader.js In which case it would be this version of the loader. but legacy.js is not our priority at the time.

As I said before, the BACKWARD COMPATIBILITY is a must in any serious project. You can find a bunch of SEA files over the internet. Are you going to re-export them all from 3DS Max ???

mrdoob commented 8 years ago

@RemusMar

We are the first ones to hate breaking backwards compatibility. We go great lengths to avoid that but some times we have no option than correcting bad designs and decisions. On those cases we try to add code to warn the user and if possible to keep it working still.

It's hard for us to cover all the possible use cases, so it's imposible not to break some thing. For those cases, the last thing we want is to get attacked with a demanding tone like the one you give us.

It may be a language issue and it may not be your intention to come across like that. But, please, try to be respectful to us the same way we are to you.

RemusMar commented 8 years ago

some times we have no option than correcting bad designs and decisions.

I totally agree with that. But correcting the bad design and breaking the backward compatibility are two different things. cheers

sunag commented 8 years ago

Hi, I finished the loader update now hybrid, it works both for the TJS as to the Legacy.

@RemusMar @DVLP https://github.com/sunag/sea3d/blob/gh-pages/Build/Three.JS/sea3d.min.js

Compiler and files included. https://github.com/sunag/sea3d/blob/gh-pages/Compiler/build-three.js.bat

hello world threejs-r72 (legacy) http://sunag.github.io/sea3d/Examples/Programmer/WebGL/Three.JS/Basics/hello-world.html

Cheers.

RemusMar commented 8 years ago

Hi, I finished the loader update now hybrid, it works both for the TJS as to the Legacy. https://github.com/sunag/sea3d/blob/gh-pages/Build/Three.JS/sea3d.min.js

I see some improvements: all the models are loaded and I don't get errors. But there are still serious issues: 1) the textures with alpha channel (transparency) do not work 2) I have to rotate all the models with 180 degrees on the X axis. 3) the LightMap legacy (inverted Diffuse/LightMap) does not work

r71: perfect http://necromanthus.com/Test/html5/SMC.html

r72: issues http://necromanthus.com/Test/html5/SMC_r72.html

cheers

sunag commented 8 years ago

@RemusMar thanks for tests, the 1 and 2 items apparently they have been fixed.

@mrdoob aoMap only works with Ambient Light? https://github.com/mrdoob/three.js/blob/master/src/renderers/shaders/ShaderChunk/aomap_fragment.glsl

Cheers.

WestLangley commented 8 years ago

@sunag An ambient occlusion map occludes only ambient light sources. Hence its name.

Another term for ambient light is indirect light.

There are currently three models of indirect, diffuse light: AmbientLight, LightMap, and HemisphereLight.

The AO map occludes all three.

sunag commented 8 years ago

Thaks @WestLangley! But if I want to give the same effect of a shadowmap it is possible? Also because in my design AO is a soft shadow, looking by physics

RemusMar commented 8 years ago

@RemusMar thanks for tests, the 1 and 2 items apparently they have been fixed.

That's right. r71: http://necromanthus.com/Test/html5/SMC.html r72: http://necromanthus.com/Test/html5/SMC_r72.html But you should also fix the LightMap legacy (inverted Diffuse/LightMap). Why? Because there are hundreds or thousands of SEA files over the net. And most of them won't be re-exported from 3DS Max.

Anyway, I found a new bug: animation related r71: http://necromanthus.com/Test/html5/dog.html r72: http://necromanthus.com/Test/html5/dog_r72.html Please investigate and fix it. cheers

RemusMar commented 8 years ago

https://github.com/sunag/sea3d/commit/4d4eb0f69e4c854fc4056758a0b40c8584363f96

That bug was fixed, but there is another one: multiple animations related. r71: http://necromanthus.com/Test/html5/dog.html r72: http://necromanthus.com/Test/html5/dog_r72.html

Press key 3, after a few seconds press key 1, after a few seconds press key 2. Everything runs perfect in r71 In r72 ALL the motions are mixed and that's wrong. Please fix it. cheers

RemusMar commented 8 years ago

Thaks @WestLangley! But if I want to give the same effect of a shadowmap it is possible? Also because in my design AO is a soft shadow, looking by physics

See the "aoMapIntensity" value (1 by default) For example with "loader.materials[0].aoMapIntensity = 3;" I was FINALLY able to get the same result with r71 and r72: r71: http://necromanthus.com/Test/html5/SMC.html r72: http://necromanthus.com/Test/html5/SMC_r72.html

cheers

sunag commented 8 years ago

Hi @RemusMar

Because there are hundreds or thousands of SEA files over the net...

I'll be updating the Legacy to the next versions.

For example with "loader.materials[0].aoMapIntensity = 3;" I was FINALLY able to get the same result with r71 and r72:

The problem with this technique that just worked if there Indirect Light (in case Ambient Light). I made a change in native shader to AO be a Shadow Map for THREE.SEA3D.StandardMaterial.

That bug was fixed, but there is another one: multiple animations related. That bug was fixed, but there is another one: multiple animations related. r71: http://necromanthus.com/Test/html5/dog.html r72: http://necromanthus.com/Test/html5/dog_r72.html

Try use the SEA3D AnimationHandler in updater:

var delta = clock.getDelta();
THREE.SEA3D.AnimationHandler.update( delta );
THREE.AnimationHandler.update( delta );

Cheers.

RemusMar commented 8 years ago

I'll be updating the Legacy to the next versions.

Don't forget about that. As I said from the very beginning, the backward compatibility is a MUST in any serious project.

Try use the SEA3D AnimationHandler in updater:

var delta = clock.getDelta(); THREE.SEA3D.AnimationHandler.update( delta ); THREE.AnimationHandler.update( delta );

That did the trick have a great weekend

mrdoob commented 8 years ago

Many thanks guys!

pailhead commented 8 years ago

Hi,

I'm not sure if i should open up another issue, but i missed this one and there is a ton of stuff here about some specific loader.

Why was this removed? Is the idea to rely on derivatives and fragment shaders always? If so, i think this will more or less be useful for procedural surface like textures (orange skin, brick wall etc.) very not useful for any kind of a bake (maya, max, xnormal etc.).

mrdoob commented 8 years ago

If so, i think this will more or less be useful for procedural surface like textures (orange skin, brick wall etc.) very not useful for any kind of a bake (maya, max, xnormal etc.).

How so?

WestLangley commented 8 years ago

@pailhead Please see https://github.com/mrdoob/three.js/issues/7094. Perhaps you can continue the discussion there, instead.