Enable negative transform scale to flip objects without inverted normals

[bhouston]

Right now there is a parameter in the shaders that lets you render backfaces. It does so by setting CCW and it inverts the normals:

FlipSided changes CCW:

https://github.com/mrdoob/three.js/blob/master/src/renderers/WebGLRenderer.js#L5285

FlipSided also changes normal:

https://github.com/mrdoob/three.js/blob/master/src/renderers/webgl/WebGLProgram.js#L140

https://github.com/mrdoob/three.js/blob/master/src/renderers/shaders/ShaderChunk.js#L1355

But sometimes one has a negative scale in the transform matrix -- in DCC programs like 3DS Max, Maya, Softimage, etc, it means flip the object. The way to do this is to have CCW set (as opposed to CW) but keep the normals the way they are (because they are already flipped by the matrix transform.) To achieve this efficiently with ThreeJS it is necessary to be able to set CCW/CW independently of flipping the normals.

Right now there is no flag that allows for CCW but without inverting the normals. Can we add some option that enables this?

Even if we had another option called FLIP_NORMAL/flipNormals that would just invert normals from whatever they are set via flipSided/FLIP_SIDED. In our case we could set both flipSided and flipNormals to achieve our desired result. This would be backwards compatible.

We can make the PR if this is acceptable.

[mrdoob]

Sounds good to me.

[WestLangley]

three.js does not support reflections in the object matrix -- it only supports pure rotations and (positive) scale factors.

See the discussion in #4272 and #3845.

This is a can-of-worms, and I am not sure there is a way out of it.

[bhouston]

@WestLangley The cool thing about the fix we are proposing is not going to change anything in ThreeJS's core, it is just adding a new flag to materials to flip normals. The flag will have to be set manually. Yes, we are using this flag in http://Clara.io to support correct reflections in the object matrix, but that is not going to affect anyone else. We can even bake these reflections into the exported ThreeJS scene files so that no one sees these outside of Clara.io. For us, supporting this is a requirement because this is how artists work. Artists consider it a bug that they can not reflect objects just by entering in a negative scale.

[bhouston]

@WestLangley Also we fixed the matrix decomposition issue with reflected objects in this PR -- it preserved the scale properly: https://github.com/mrdoob/three.js/pull/4272

[WestLangley]

@bhouston OK. I will keep an open mind. : - )

[Kesshi]

@bhouston Some news about this issue? In my current project i need to be able to mirror objects. At the moment i need to duplicate every geometry to have a correct mirrored and not mirrored version of it. With this new feature i could just clone the material and set the appropriate flags.

[mrdoob]

I think if you do object.scale.x = -1 and object.material.side = THREE.DoubleSide that should do the trick render wise.

[WestLangley]

I think if you do object.scale.x = -1 and object.material.side = THREE.DoubleSide that should do the trick render wise.

That only works for MeshBasicMaterial. It does so because that material does not require normals.

three.js does not, in general, support reflections in the object matrix ( e.g., negative scale factors ).

The proper solution is to correctly mirror a geometry. For example, we could add:

geometry.mirrorX();
geometry.mirrorY();
geometry.mirrorZ();

Such methods would make sure normals and winding orders were correct after mirroring.

Or we could implement the feature as a modifier, and add it to the examples/modifier directory.

Or we could add the feature to GeometryUtils.

It would also have to support all types of geometry, I expect.

@mrdoob There is a GeometryUtils in /src and in /examples ?

[mrdoob]

@mrdoob There is a GeometryUtils in /src and in /examples ?

Not a big fan of GeometryUtils in core... Maybe we could move it to /examples and add mirrorX(), etc there?

[WestLangley]

So, would this be your preference?

THREE.GeometryUtils.mirrorX( geometry ); // also mirrorY and mirrorZ

THREE.BufferGeometryUtils.mirrorX( geometry ); // also mirrorY and mirrorZ

That is fine with me. I am thinking that it is more of a modifier, though. But the modifiers are not geometry-specific, so we would have to test for instanceof. Ugh.

[Kesshi]

That means i need to duplicate the geometry. :-( I made some tests with negative scale and i didn't encounter an issue yet. The only thing missing is separate control over face winding and normal flipping at the material. I'm with bhousten in this topic. Using a negative scale to mirror an object is a very common workflow and i was suprised about the missing support in threejs. How is this handled in all the importers (e.g. collada)? I'm sure that there are a lot of collada files in the web which contain a negative scale.

[WestLangley]

That means i need to duplicate the geometry.

Correct.

I made some tests with negative scale and i didn't encounter an issue yet. The only thing missing is separate control over face winding and normal flipping at the material.

Well, in addition, uv's need to be corrected, and vertex colors if present, tangents if present, and the 2nd set of uv's if present.

So you want to be able to set a negative scale, and also set a flag in the material to render a geometry mirrored?

[Kesshi]

What is the problem with UVs? See this fiddle: http://jsfiddle.net/mo9t0uLu/1/ Everything except the normals is correct. I just need a way to flip them. I didn't test it but i think the vertex colors should also work.

[mrdoob]

Using a negative scale to mirror an object is a very common workflow and i was suprised about the missing support in threejs.

The issue was that, once we supported the rendering part, we then have to support it for raycasting and other stuff.

[WestLangley]

@Kesshi

What is the problem with UVs?

Sorry. We were cross-talking. I would have to fix uvs if I created a new geometry and manipulated the vertex winding order. You do not want to create a new geometry.

Everything except the normals is correct. I just need a way to flip them.

The winding order in your model is now clockwise. This will have ramifications elsewhere in the library.

That being said, I now understand what @bhouston and @Kesshi are advocating. It is sounding more reasonable to me now, if we could get it to work.

[rohan-deshpande]

@mrdoob https://github.com/mrdoob/three.js/issues/4904#issuecomment-146907084 is currently broken in r84 for SpriteMaterial see my stackoverflow ticket.

[andreasplesch]

I would like to understand why the normals reverse direction (flip) when a negative scaling is used. I suspect this happens because normals are generated after the reflection of vertices but I am unsure. Is this the case ?

If so, it seems that generating normals using the untransformed vertices and then applying the transformation matrix to both vertices and normals would be a more rigorous implementation ?

[ErikHEissig]

I figured out that you need to do a little workaround to fix this. My Problem was that the negative scaled side looked darker than the other side. Now i use this code to fix this:

if (Mesh.scale.x < 0) {
    Mesh.scale.x *= -1;
    var tmpGeo = new THREE.Geometry().copy(Mesh.geometry);
    for (var i = 0; i < tmpGeo.vertices.length; i++) {          
        tmpGeo.vertices[i].x *= -1;
    }
    var tmpMesh = new THREE.Mesh(tmpGeo, Mesh.material);
    Mesh.geometry.merge(tmpGeo);
}

so i am not negative scaling the mesh. I just negative scaling the points.

This needs to be made after this, otherwise there will be no effect

Mesh.geometry.computeVertexNormals();
Mesh.geometry.mergeVertices();

Can you include something like this into three.js?

[looeee]

Related: #11911

@ErikHEissig the solution from that issue is:

function fixInvertedNormal( object ) {

    object.traverse( ( child ) => {

        if ( child instanceof THREE.Mesh ) {

            if ( child.matrixWorld.determinant() < 0 ) {

                const l = child.geometry.attributes.position.array.length;

                const positions = child.geometry.attributes.position.array;
                const normals = child.geometry.attributes.normal.array;

                for ( let i = 0; i < l; i += 9 ) {

                                       // reverse winding order
                    const tempX = positions[ i + 0 ];
                    const tempY = positions[ i + 1 ];
                    const tempZ = positions[ i + 2 ];

                    positions[ i + 0 ] = positions[ i + 6 ];
                    positions[ i + 1 ] = positions[ i + 7 ];
                    positions[ i + 2 ] = positions[ i + 8 ];

                    positions[ i + 6 ] = tempX;
                    positions[ i + 7 ] = tempY;
                    positions[ i + 8 ] = tempZ;

                                        // switch vertex normals
                    const tempNX = normals[ i + 0 ];
                    const tempNY = normals[ i + 1 ];
                    const tempNZ = normals[ i + 2 ];

                    normals[ i + 0 ] = normals[ i + 6 ];
                    normals[ i + 1 ] = normals[ i + 7 ];
                    normals[ i + 2 ] = normals[ i + 8 ];

                    normals[ i + 6 ] = tempNX;
                    normals[ i + 7 ] = tempNY;
                    normals[ i + 8 ] = tempNZ;

                }

            }

        }

    } );

}

Which is more general case that your method, fixes an object and any children and should be much faster.

I don't think it's been tested enough to be included as a utility function somewhere, but I've been using it for quite a while with the FBXLoader and haven't come across any issues so far.

[bhouston]

Your solution @looeee is not robust. The reason it is not robust is that a single geometry may be referenced multiple times in a scene graph (e.g. by multiple Mesh nodes) and some of those Mesh nodes may have negative determinant matrices and some may not.

The current code you have will invert the geometry for each negative determinant it encounters. Thus a geometry is shared with two mesh nodes, each of which has a negative determinant, then it will invert the normals / switch winding order and then uninvert the normals and switch back the winding order. Thus giving an incorrect result. If some meshes have negative determinant and non-negative determinants, then this global modification of a shared geometry will necessarily give incorrect results for some of those nodes.

If you are going to modify this geometry, it requires copying the Geometry object, rather than modifying it in place. THis is still inefficient if a Geometry object is shared across many Mesh nodes as it will create copies for each negative determinant.

This idea of modifying the geometry is also not performant. If you happen to invert a determinant at run time, you need to modify the geometry and reload it to the GPU. That is pretty brutal.

The very best way to solve the issue is what I and @Nimanf proposed in this issue and related PR. Maybe our implementation wasn't as elegant as it could have been but we need to do something that is basically what we proposed.

[looeee]

@bhouston I knew there was going to be a catch. Thanks for the clear explanation, that makes a lot of sense. In my particular case though, loading an FBX model with no shared geometry, this does work well and is very fast, even if it isn't a general solution.

On the other hand though, I ran into this issue because I was given a model created in 3ds max in which the right limbs were created by mirroring the left. This is a common modelling technique, and there is no way to expect artists creating models, which may not even be originally intended for use with three.js, to know that they should avoid it. It took me a couple of days and a lot of wasted hours to figure out what was going on. Well, not entirely wasted since I learned a lot, but still...

So I support the adding of some kind of flags as suggested in #4910 - even if it seems that this makes things more complicated, for people that run into this issue it will ultimately make things a lot simpler.

[JackC09]

I have the same issue, why PR #4910 is still not merged since 2014 and it closed 23 days ago ?

[Mugen87]

The PR had merge conflicts and some feedback of @bhouston was not implemented. I suggest to start with a fresh one.

[JackC09]

Ok thank you for quick answer, I hope @bhouston will start new PR with his feedback

[WestLangley]

Closing. Addressed in #12787.