mrdoob / three.js

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

BoundingBoxHelper for buffer geometry does not use drawrange neither groups #8690

Open tonnot opened 8 years ago

tonnot commented 8 years ago
Description of the problem

BoundingBoxHelper.update (setFromObject) for buffer geometry does not use drawrange.count ( neither groups), so the limits can include points with 0,0,0 (now it uses positions.length)

if you have the typedarray with data still with 0,0,0, the box dimension figured out is not correct.(0,0,0 is included...) (I have predimensioned positions array and filling as I need)

I'm using by now to create pointclouds but I imagine some problems in the future if the bufferGeometry is used to drawlines or triangles (using groups)

So I this this is a mix of bug & enhance ,? ...

Three.js version
WestLangley commented 8 years ago

See #8674

WestLangley commented 8 years ago

BoundingBoxHelper calls Box3.setFromObject().

Box3.setFromObject() does not honor drawRange or groups.

It is up for debate as to whether it should.

tonnot commented 8 years ago

Drawrange and Groups are used to draw the buffers so IMHO they should be used at setFromObject This code works for me:

( second part of setFromObject)

else if ( geometry instanceof THREE.BufferGeometry && geometry.attributes[ 'position' ] !== undefined ) {

                    var positions = geometry.attributes[ 'position' ].array;

                    var pseudoGroups = [];

                    var from,to;

                    if (geometry.groups.length==0)
                        pseudoGroups = [{'start': geometry.drawRange.start, 'count': geometry.drawRange.count}];
                    else
                        pseudoGroups = geometry.groups;

                    for ( var i = 0, il =pseudoGroups.length; i < il; ++i ) { 

                        from =  pseudoGroups[i].start*3;
                        to   = (pseudoGroups[i].start+pseudoGroups[i].count)*3;

                        for ( var j = from; j < to; j += 3 ) {

                            v1.fromArray( positions, j );
                            v1.applyMatrix4( node.matrixWorld );
                            scope.expandByPoint( v1 );

                        }

                    }

                }

The only thing to evaluate is wether it can be possible to have a drawrange start-count that limits the groups. (2 or 3 more lines of code).

Thanks

WestLangley commented 8 years ago

Continuing the discussion...

Groups exist for the purpose of assigning multiple materials to a geometry. I am not sure why groups should be considered here.

Also, BufferGeometry.computeBoundingBox() does not honor drawRange or groups.

Also, material.visible = false is not honored. Nor are morphs.

In any event, you would have to handle indexed and non-indexed BufferGeometry differently.

tonnot commented 8 years ago

Ups , sorry for the mistake. I was mixing concepts

Maybe this is more reasonable... The idea is use the right range, use a length value is wrong if I have a dynamic array I fill with new data and then, range.count is the valid value.

 else if ( geometry instanceof THREE.BufferGeometry && geometry.attributes[ 'position' ] !== undefined ) {

            var positions = geometry.attributes[ 'position' ].array;

           var  from = geometry.drawRange.start*3;
           var  to    = (geometry.drawRange.start + geometry.drawRange.count)*3;

                    for ( var j =from ; j < to; j += 3 ) {

                        v1.fromArray( positions, j );
                        v1.applyMatrix4( node.matrixWorld );
                        scope.expandByPoint( v1 );

                }

     }

Thanks and sorry

WestLangley commented 8 years ago

Like I said,

In any event, you would have to handle indexed and non-indexed BufferGeometry differently.

The draw range is over the index for indexed BufferGeometry.

But even so, it is debatable if drawRange should be honored.

mrdoob commented 4 years ago

Hmm, groups are definitely a can of worms. But drawRange could be interesting...