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

DirectGeometry: remove boundingBox & boundingSphere properties #13420

Closed
wants to merge 1 commit into from
Closed

DirectGeometry: remove boundingBox & boundingSphere properties #13420

wants to merge 1 commit into from

Conversation

WJsjtu
Copy link
Contributor

@WJsjtu WJsjtu commented Feb 24, 2018

this.boundingBox = null;
this.boundingSphere = null;

Are these two properties ever used?
<div>
Bounding box for the bufferGeometry, which can be calculated with
[page:.computeBoundingBox](). Default is *null*.
</div>
<h3>[property:Sphere boundingSphere]</h3>
<div>
Bounding sphere for the bufferGeometry, which can be calculated with
[page:.computeBoundingSphere](). Default is *null*.
</div>

The doc says they can be calculated, but no such functions are found for DirectGeometry.
<h3>[property:null computeBoundingBox]( )</h3>
<div>
See [page:Geometry.computeBoundingBox].
</div>
<h3>[property:null computeBoundingSphere]( )</h3>
<div>
See [page:Geometry.computeBoundingSphere].
</div>

Here too.
<h3>[page:EventDispatcher EventDispatcher] methods are available on this class.</h3>

And this is not true.

@WJsjtu WJsjtu changed the title DirectGeometry: remove boundingBox & boundingSphere property DirectGeometry: remove boundingBox & boundingSphere properties Feb 24, 2018
@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 24, 2018

Instead of removing the code, i think i would clone the bounding volumes from a geometry object in DirectGeometry.fromGeometry() if present.

if ( geometry.boundingSphere !== null ) {

   this.boundingSphere = geometry.boundingSphere.clone();

}

if ( geometry.boundingBox !== null ) {

   this.boundingBox = geometry.boundingBox.clone();

}

@WJsjtu
Copy link
Contributor Author

WJsjtu commented Feb 24, 2018

@Mugen87 Yes, maybe you are right.
Up to now, I haven't found any usage of these two properties.
As the doc writes, DirectGeometry is only used internally to convert from Geometry to BufferGeometry (as my understanding, the main usage is in updateFromObject, fromGeometry and fromDirectGeometry methods of BufferGeometry). Currently, the only use of BufferGeometry.fromDirectGeometry method is in this repo is BufferGeometry.fromGeometry, and BufferGeometry.fromGeometry is only used in BufferGeometry.updateFromObject and BufferGeometry.setFromObject.
The BufferGeometry.setFromObject which receives an object with geometry instance of Geometry as argument will clone the bounding volumes for Points and Line, but call BufferGeometry.fromGeometry for Mesh.
BufferGeometry.fromGeometry will create a DirectGeometry instance as __directGeometry property of the geometry, then DirectGeometry.fromGeometry is immediately called,.
However, DirectGeometry.fromGeometry doesn't clone the bounding volumes, as a result, the bounding volumes of Mesh's geometry won't be cloned in BufferGeometry.setFromObject as Points's or Line's (if bounding volumes is not null). And this is a little inconsistent in logic.
The BufferGeometry.updateFromObject won't copy the bounding volumes as well.
Maybe the cloning could be added either in BufferGeometry.fromGeometry (my opinion) or DirectGeometry.fromGeometry as your suggestion.

fromGeometry: function ( geometry ) {
geometry.__directGeometry = new DirectGeometry().fromGeometry( geometry );
return this.fromDirectGeometry( geometry.__directGeometry );
},

However both ways will affect BufferGeometry.updateFromObject, it will clone the bounding volumes only for Mesh?

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 24, 2018

However both ways will affect BufferGeometry.updateFromObject, it will clone the bounding volumes only for Mesh?

The conversion from Geometry to BufferGeometry is only supported for meshes.

Maybe the cloning could be added either in BufferGeometry.fromGeometry (my opinion)

I would not do this since the real conversion is delegated to DirectGeometry.fromGeometry() and BufferGeometry.fromDirectGeometry().

@WJsjtu
Copy link
Contributor Author

WJsjtu commented Feb 25, 2018

@Mugen87 Thanks for explanation. I'll close this pull request and make a new one as you suggested.

Instead of removing the code, i think i would clone the bounding volumes from a geometry object in DirectGeometry.fromGeometry() if present.

The conversion from Geometry to BufferGeometry is only supported for meshes.

You are right. What I mean is it doesn't matter which method to clone bounding volumes, but if BufferGeometry.fromGeometry copy the bounding volumes for Mesh, then BufferGeometry.setFromObject will be able to clone bounding volumes for objects, while BufferGeometry.updateFromObject will only clone copy the bounding volumes for Mesh not for Line or Points. And this is inconsistent.

To make it clear:

  • Currently, BufferGeometry.updateFromObject won't clone bounding volumes for any objects, BufferGeometry.setFromObject will only clone bounding volumes for Line and Points.
  • After modified, BufferGeometry.setFromObject will be fixed to clone bounding volumes for any object, however, BufferGeometry.updateFromObject will only clone copy the bounding volumes for Mesh not for Line or Points.

And BufferGeometry.updateFromObject is called by WebGLObjects for any object in WebGLRenderer.js.

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.

2 participants