error of DirectGeometry #8591

Closed
Hcely opened this Issue Apr 11, 2016 · 10 comments

Comments

Projects
None yet
4 participants
@Hcely

Hcely commented Apr 11, 2016

hi.
DirectGeometry -> fromGeometry,forget to save index data

@Hcely

This comment has been minimized.

Show comment
Hide comment
@Hcely

Hcely Apr 11, 2016

this.vertices.push( vertices[ face.a ], vertices[ face.b ], vertices[ face.c ] );//this is not a good method.

Hcely commented Apr 11, 2016

this.vertices.push( vertices[ face.a ], vertices[ face.b ], vertices[ face.c ] );//this is not a good method.

@WestLangley

This comment has been minimized.

Show comment
Hide comment
@WestLangley

WestLangley Apr 11, 2016

Collaborator

Can you provide a test case, along with the information requested in "How to report a bug" in the guidelines?

Collaborator

WestLangley commented Apr 11, 2016

Can you provide a test case, along with the information requested in "How to report a bug" in the guidelines?

@Hcely

This comment has been minimized.

Show comment
Hide comment
@Hcely

Hcely Apr 11, 2016

I edit fromGeometry method.it is working now.

fromGeometry : function(geometry) {
        var faces = geometry.faces;
        this.vertices = geometry.vertices;
        var faceVertexUvs = geometry.faceVertexUvs;
        var hasFaceVertexUv = faceVertexUvs[0] && faceVertexUvs[0].length > 0;
        var hasFaceVertexUv2 = faceVertexUvs[1] && faceVertexUvs[1].length > 0;
        // morphs
        var morphTargets = geometry.morphTargets;
        var morphTargetsLength = morphTargets.length;
        var morphTargetsPosition;
        if (morphTargetsLength > 0) {
            morphTargetsPosition = [];
            for (var i = 0; i < morphTargetsLength; i++) {
                morphTargetsPosition[i] = [];
            }
            this.morphTargets.position = morphTargetsPosition;
        }
        var morphNormals = geometry.morphNormals;
        var morphNormalsLength = morphNormals.length;
        var morphTargetsNormal;
        if (morphNormalsLength > 0) {
            morphTargetsNormal = [];
            for (var i = 0; i < morphNormalsLength; i++) {
                morphTargetsNormal[i] = [];
            }
            this.morphTargets.normal = morphTargetsNormal;
        }
        // skins
        this.skinIndices = geometry.skinIndices;
        this.skinWeights = geometry.skinWeights;
        //
        for (var i = 0; i < faces.length; i++) {
            var face = faces[i];
            this.indices.push(face);
            var vertexNormals = face.vertexNormals;
            if (vertexNormals.length === 3) {
                this.normals[face.a] = vertexNormals[0];
                this.normals[face.b] = vertexNormals[1];
                this.normals[face.c] = vertexNormals[2];
            } else {
                this.normals[face.a] = face.normal;
                this.normals[face.b] = face.normal;
                this.normals[face.c] = face.normal;
            }
            var vertexColors = face.vertexColors;
            if (vertexColors.length === 3) {
                this.colors[face.a] = vertexColors[0];
                this.colors[face.b] = vertexColors[1];
                this.colors[face.c] = vertexColors[2];
            } else {
                this.colors[face.a] = face.color;
                this.colors[face.b] = face.color;
                this.colors[face.c] = face.color;
            }

            if (hasFaceVertexUv === true) {
                var vertexUvs = faceVertexUvs[0][i];
                if (vertexUvs !== undefined) {
                    this.uvs[face.a] = vertexUvs[0];
                    this.uvs[face.b] = vertexUvs[1];
                    this.uvs[face.c] = vertexUvs[2];
                } else {
                    console
                            .warn(
                                    'THREE.DirectGeometry.fromGeometry(): Undefined vertexUv ',
                                    i);
                    this.uvs[face.a] = new THREE.Vector2();
                    this.uvs[face.b] = new THREE.Vector2();
                    this.uvs[face.c] = new THREE.Vector2();
                }
            }

            if (hasFaceVertexUv2 === true) {
                var vertexUvs = faceVertexUvs[1][i];
                if (vertexUvs !== undefined) {
                    this.uvs2[face.a] = vertexUvs[0];
                    this.uvs2[face.b] = vertexUvs[1];
                    this.uvs2[face.c] = vertexUvs[2];
                } else {
                    console
                            .warn(
                                    'THREE.DirectGeometry.fromGeometry(): Undefined vertexUv2 ',
                                    i);
                    this.uvs2[face.a] = new THREE.Vector2();
                    this.uvs2[face.b] = new THREE.Vector2();
                    this.uvs2[face.c] = new THREE.Vector2();
                }

            }
            // morphs
            for (var j = 0; j < morphTargetsLength; j++) {
                var morphTarget = morphTargets[j].vertices;
                morphTargetsPosition[j].push(morphTarget[face.a],
                        morphTarget[face.b], morphTarget[face.c]);
            }
            for (var j = 0; j < morphNormalsLength; j++) {
                var morphNormal = morphNormals[j].vertexNormals[i];
                morphTargetsNormal[j].push(morphNormal.a, morphNormal.b,
                        morphNormal.c);
            }
        }

        this.computeGroups(geometry);
        this.verticesNeedUpdate = geometry.verticesNeedUpdate;
        this.normalsNeedUpdate = geometry.normalsNeedUpdate;
        this.colorsNeedUpdate = geometry.colorsNeedUpdate;
        this.uvsNeedUpdate = geometry.uvsNeedUpdate;
        this.groupsNeedUpdate = geometry.groupsNeedUpdate;
        return this;
    }

Hcely commented Apr 11, 2016

I edit fromGeometry method.it is working now.

fromGeometry : function(geometry) {
        var faces = geometry.faces;
        this.vertices = geometry.vertices;
        var faceVertexUvs = geometry.faceVertexUvs;
        var hasFaceVertexUv = faceVertexUvs[0] && faceVertexUvs[0].length > 0;
        var hasFaceVertexUv2 = faceVertexUvs[1] && faceVertexUvs[1].length > 0;
        // morphs
        var morphTargets = geometry.morphTargets;
        var morphTargetsLength = morphTargets.length;
        var morphTargetsPosition;
        if (morphTargetsLength > 0) {
            morphTargetsPosition = [];
            for (var i = 0; i < morphTargetsLength; i++) {
                morphTargetsPosition[i] = [];
            }
            this.morphTargets.position = morphTargetsPosition;
        }
        var morphNormals = geometry.morphNormals;
        var morphNormalsLength = morphNormals.length;
        var morphTargetsNormal;
        if (morphNormalsLength > 0) {
            morphTargetsNormal = [];
            for (var i = 0; i < morphNormalsLength; i++) {
                morphTargetsNormal[i] = [];
            }
            this.morphTargets.normal = morphTargetsNormal;
        }
        // skins
        this.skinIndices = geometry.skinIndices;
        this.skinWeights = geometry.skinWeights;
        //
        for (var i = 0; i < faces.length; i++) {
            var face = faces[i];
            this.indices.push(face);
            var vertexNormals = face.vertexNormals;
            if (vertexNormals.length === 3) {
                this.normals[face.a] = vertexNormals[0];
                this.normals[face.b] = vertexNormals[1];
                this.normals[face.c] = vertexNormals[2];
            } else {
                this.normals[face.a] = face.normal;
                this.normals[face.b] = face.normal;
                this.normals[face.c] = face.normal;
            }
            var vertexColors = face.vertexColors;
            if (vertexColors.length === 3) {
                this.colors[face.a] = vertexColors[0];
                this.colors[face.b] = vertexColors[1];
                this.colors[face.c] = vertexColors[2];
            } else {
                this.colors[face.a] = face.color;
                this.colors[face.b] = face.color;
                this.colors[face.c] = face.color;
            }

            if (hasFaceVertexUv === true) {
                var vertexUvs = faceVertexUvs[0][i];
                if (vertexUvs !== undefined) {
                    this.uvs[face.a] = vertexUvs[0];
                    this.uvs[face.b] = vertexUvs[1];
                    this.uvs[face.c] = vertexUvs[2];
                } else {
                    console
                            .warn(
                                    'THREE.DirectGeometry.fromGeometry(): Undefined vertexUv ',
                                    i);
                    this.uvs[face.a] = new THREE.Vector2();
                    this.uvs[face.b] = new THREE.Vector2();
                    this.uvs[face.c] = new THREE.Vector2();
                }
            }

            if (hasFaceVertexUv2 === true) {
                var vertexUvs = faceVertexUvs[1][i];
                if (vertexUvs !== undefined) {
                    this.uvs2[face.a] = vertexUvs[0];
                    this.uvs2[face.b] = vertexUvs[1];
                    this.uvs2[face.c] = vertexUvs[2];
                } else {
                    console
                            .warn(
                                    'THREE.DirectGeometry.fromGeometry(): Undefined vertexUv2 ',
                                    i);
                    this.uvs2[face.a] = new THREE.Vector2();
                    this.uvs2[face.b] = new THREE.Vector2();
                    this.uvs2[face.c] = new THREE.Vector2();
                }

            }
            // morphs
            for (var j = 0; j < morphTargetsLength; j++) {
                var morphTarget = morphTargets[j].vertices;
                morphTargetsPosition[j].push(morphTarget[face.a],
                        morphTarget[face.b], morphTarget[face.c]);
            }
            for (var j = 0; j < morphNormalsLength; j++) {
                var morphNormal = morphNormals[j].vertexNormals[i];
                morphTargetsNormal[j].push(morphNormal.a, morphNormal.b,
                        morphNormal.c);
            }
        }

        this.computeGroups(geometry);
        this.verticesNeedUpdate = geometry.verticesNeedUpdate;
        this.normalsNeedUpdate = geometry.normalsNeedUpdate;
        this.colorsNeedUpdate = geometry.colorsNeedUpdate;
        this.uvsNeedUpdate = geometry.uvsNeedUpdate;
        this.groupsNeedUpdate = geometry.groupsNeedUpdate;
        return this;
    }
@brunoimbrizi

This comment has been minimized.

Show comment
Hide comment
@brunoimbrizi

brunoimbrizi Feb 13, 2017

The bug was not reported properly, but the issue exists.

Description of the problem

DirectGeometry.fromGeometry() duplicates vertices instead of using indices.

Here is a fiddle showing a BoxGeometry (8 vertices, 12 faces) and a BufferGeometry created using .fromGeometry and passing the same box (108 positions, index null).
https://jsfiddle.net/brunoimbrizi/3opvf7rd/4/

The expected result would be 24 positions (8 * 3) and 36 indices (12 * 3).

Three.js version
  • Dev
  • r84
Browser
  • All of them
OS
  • All of them

brunoimbrizi commented Feb 13, 2017

The bug was not reported properly, but the issue exists.

Description of the problem

DirectGeometry.fromGeometry() duplicates vertices instead of using indices.

Here is a fiddle showing a BoxGeometry (8 vertices, 12 faces) and a BufferGeometry created using .fromGeometry and passing the same box (108 positions, index null).
https://jsfiddle.net/brunoimbrizi/3opvf7rd/4/

The expected result would be 24 positions (8 * 3) and 36 indices (12 * 3).

Three.js version
  • Dev
  • r84
Browser
  • All of them
OS
  • All of them
@WestLangley

This comment has been minimized.

Show comment
Hide comment
@WestLangley

WestLangley Feb 13, 2017

Collaborator

@brunoimbrizi BufferGeometry.fromGeometry() produces non-indexed BufferGeometry by design. Two vertices cannot be shared with BufferGeometry unless the position and all attributes are identical. Creating non-indexed BufferGeometry is easier.

Collaborator

WestLangley commented Feb 13, 2017

@brunoimbrizi BufferGeometry.fromGeometry() produces non-indexed BufferGeometry by design. Two vertices cannot be shared with BufferGeometry unless the position and all attributes are identical. Creating non-indexed BufferGeometry is easier.

@brunoimbrizi

This comment has been minimized.

Show comment
Hide comment
@brunoimbrizi

brunoimbrizi Feb 13, 2017

@WestLangley Sorry to insist, but I don't understand the advantage of not using the index information from an existing Geometry when calling BufferGeometry.fromGeometry().

In other words, if the vertices are shared in a Geometry why wouldn't they be in their BufferGeometry equivalent?

Here is an update to the fiddle showing where positions and indices are manually set based on the vertices and faces of the original BoxGeometry.
https://jsfiddle.net/brunoimbrizi/3opvf7rd/5/


The issue is particularly evident when importing a complex model. I found it when importing a model with 774 positions and seeing the number go up to 4095 positions after .fromGeometry(). This had a huge impact in performance (4-5 fps on iOS Safari 9). After adding the positions and indices manually - as in the fiddle above - the numbers went down to exactly as they were in the original geometry and the performance improved immensely (60 fps on iOS Safari 9).

@WestLangley Sorry to insist, but I don't understand the advantage of not using the index information from an existing Geometry when calling BufferGeometry.fromGeometry().

In other words, if the vertices are shared in a Geometry why wouldn't they be in their BufferGeometry equivalent?

Here is an update to the fiddle showing where positions and indices are manually set based on the vertices and faces of the original BoxGeometry.
https://jsfiddle.net/brunoimbrizi/3opvf7rd/5/


The issue is particularly evident when importing a complex model. I found it when importing a model with 774 positions and seeing the number go up to 4095 positions after .fromGeometry(). This had a huge impact in performance (4-5 fps on iOS Safari 9). After adding the positions and indices manually - as in the fiddle above - the numbers went down to exactly as they were in the original geometry and the performance improved immensely (60 fps on iOS Safari 9).

@WestLangley

This comment has been minimized.

Show comment
Hide comment
@WestLangley

WestLangley Feb 13, 2017

Collaborator

In other words, if the vertices are shared in a Geometry why wouldn't they be in their BufferGeometry equivalent?

@brunoimbrizi Re-read what I wrote above. Your example uses position-only data, which is a trivial case.

I am surprised you are seeing such a performance impact. Most loaders return BufferGeometry now. Which loader are you using?

Collaborator

WestLangley commented Feb 13, 2017

In other words, if the vertices are shared in a Geometry why wouldn't they be in their BufferGeometry equivalent?

@brunoimbrizi Re-read what I wrote above. Your example uses position-only data, which is a trivial case.

I am surprised you are seeing such a performance impact. Most loaders return BufferGeometry now. Which loader are you using?

@brunoimbrizi

This comment has been minimized.

Show comment
Hide comment
@brunoimbrizi

brunoimbrizi Feb 13, 2017

@WestLangley I understand that position-only examples might be too simple. What I would like to understand is which other attributes could 'break' a BufferGeometry if its indices are set from a valid Geometry.

To answer your question, in the example I mentioned earlier I used JSONLoader, but I don't think the loader is the point.

I worked on another example to show you what I mean.
I searched for examples using fromGeometry in the three.js repository and thought that webgl_interactive_instances_gpu was a good fit. I forked the repo, created a new build with modified DirectGeometry and BufferGeometry and updated the example accordingly.

It uses fromGeometry() in the methods makeMerged and makeInstanced.

The original geometry of Suzanne.js has 507 vertices. The BufferGeometry created in the makeInstanced method has 8712 positions. Roughly 6 times more.

Using my modified code - which doesn't duplicate positions and use indices instead - the same method outputs 1521 positions (507 * 3).

The difference in performance is huge. We are lucky that this example already has all the tools to run performance tests.

Have a look at my build:
https://brunoimbrizi.github.io/three.js/examples/webgl_interactive_instances_gpu.html

And compare it with the current r84:
https://threejs.org/examples/webgl_interactive_instances_gpu.html


My results (Mac OS Chrome 55):
With 'render continuously' checked, 'number of geometry instances' set to 3000 and testing both instanced and merged as 'method of construction'.

  • my build: 45 fps (rotating) / 60 fps (still)
  • current r84: 20 fps (rotating) / 35 fps (still)

brunoimbrizi commented Feb 13, 2017

@WestLangley I understand that position-only examples might be too simple. What I would like to understand is which other attributes could 'break' a BufferGeometry if its indices are set from a valid Geometry.

To answer your question, in the example I mentioned earlier I used JSONLoader, but I don't think the loader is the point.

I worked on another example to show you what I mean.
I searched for examples using fromGeometry in the three.js repository and thought that webgl_interactive_instances_gpu was a good fit. I forked the repo, created a new build with modified DirectGeometry and BufferGeometry and updated the example accordingly.

It uses fromGeometry() in the methods makeMerged and makeInstanced.

The original geometry of Suzanne.js has 507 vertices. The BufferGeometry created in the makeInstanced method has 8712 positions. Roughly 6 times more.

Using my modified code - which doesn't duplicate positions and use indices instead - the same method outputs 1521 positions (507 * 3).

The difference in performance is huge. We are lucky that this example already has all the tools to run performance tests.

Have a look at my build:
https://brunoimbrizi.github.io/three.js/examples/webgl_interactive_instances_gpu.html

And compare it with the current r84:
https://threejs.org/examples/webgl_interactive_instances_gpu.html


My results (Mac OS Chrome 55):
With 'render continuously' checked, 'number of geometry instances' set to 3000 and testing both instanced and merged as 'method of construction'.

  • my build: 45 fps (rotating) / 60 fps (still)
  • current r84: 20 fps (rotating) / 35 fps (still)
@brunoimbrizi

This comment has been minimized.

Show comment
Hide comment
@brunoimbrizi

brunoimbrizi Feb 13, 2017

@WestLangley To your point: maybe there are some non-trivial cases where BufferGeometry.fromGeometry() needs to return non-indexed results for stuff to work properly. I don't know. However, other trivial cases may pay the price with an unnecessary high vertex count.

Would it be the case of giving the user an option? Maybe a second parameter .fromGeometry( geometry, indexed ) where indexed is a boolean.

@WestLangley To your point: maybe there are some non-trivial cases where BufferGeometry.fromGeometry() needs to return non-indexed results for stuff to work properly. I don't know. However, other trivial cases may pay the price with an unnecessary high vertex count.

Would it be the case of giving the user an option? Maybe a second parameter .fromGeometry( geometry, indexed ) where indexed is a boolean.

@mrdoob

This comment has been minimized.

Show comment
Hide comment
@mrdoob

mrdoob Feb 13, 2017

Owner

Basically, it's very hard to convert a "indexed" Geometry into a indexed BufferGeometry and still support the needsUpdate stuff. This was the only sane way I could commit to maintain.

Maybe you may want to consider using gltf? I think https://github.com/Kupoman/blendergltf produces indexed BufferGeometry.

Owner

mrdoob commented Feb 13, 2017

Basically, it's very hard to convert a "indexed" Geometry into a indexed BufferGeometry and still support the needsUpdate stuff. This was the only sane way I could commit to maintain.

Maybe you may want to consider using gltf? I think https://github.com/Kupoman/blendergltf produces indexed BufferGeometry.

@mrdoob mrdoob closed this Feb 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment