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: clone bounding volumes in fromGeometry method #13428

Merged
merged 1 commit into from
Jun 26, 2019

Conversation

WJsjtu
Copy link
Contributor

@WJsjtu WJsjtu commented Feb 25, 2018

See #13420 for detail.
@Mugen87 The doc may need to be fixed, but I'm not sure whether to remove all the information about EventDispatcher in it or to make DirectGeometry extend EventDispatcher and add dispose method ( like BufferGeometry and Geometry ).

As I mentioned in PR 13420, The same code may need to be added in BufferGeometry.updateFromObject for Mesh and Points.

@WestLangley
Copy link
Collaborator

I do not think it makes sense to add these methods here.

DirectGeometry is a temporary data structure, and IMO does not require a separate class. I would remove it completely, and inline the fromGeometry() code in the single place it is used.

@WJsjtu
Copy link
Contributor Author

WJsjtu commented Feb 25, 2018

@WestLangley You are right, of course the code of DirectGeometry can be moved to Geometry and BufferGeometry (IMO maybe the InterleavedBuffer.js and InterleavedBufferAttribute.js have the same problem). And this is a design matter. what I cared at first is about the cloning behavior which is related to the problem of whether DirectGeometry should have bounding volumes mentioned in PR13420.

@WestLangley
Copy link
Collaborator

I understand. I guess @mrdoob will make the design decision. Thank you for pointing out these issues. :)

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 25, 2018

@WJsjtu I would focus on bounding volumes in this PR. If you like, you can remove DirectGeometry.computeBoundingSphere() and DirectGeometry.computeBoundingBox() from the docs. Otherwise perform the changes on the docs at once in a separate PR.

@mrdoob mrdoob added this to the r91 milestone Feb 25, 2018
@mrdoob
Copy link
Owner

mrdoob commented Mar 14, 2018

Hmm... I'm not sure DirectGeometry's boundingBox and/or boundingSphere is being used anywhere...?

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 14, 2018

They are copied in BufferGeometry.fromDirectGeometry().

if ( geometry.boundingSphere !== null ) {
this.boundingSphere = geometry.boundingSphere.clone();
}
if ( geometry.boundingBox !== null ) {
this.boundingBox = geometry.boundingBox.clone();
}

@mrdoob
Copy link
Owner

mrdoob commented Mar 14, 2018

Oh, there you go. I should have named that variable directgeometry.
Okay, will take a look next cycle...

@mrdoob mrdoob modified the milestones: r91, r92 Mar 14, 2018
@mrdoob mrdoob modified the milestones: r92, r93 Apr 18, 2018
@mrdoob mrdoob modified the milestones: r93, r94 May 22, 2018
@mrdoob mrdoob modified the milestones: r94, r95 Jun 27, 2018
@mrdoob mrdoob modified the milestones: r95, r96 Jul 31, 2018
@mrdoob mrdoob modified the milestones: r96, r97 Aug 29, 2018
@mrdoob mrdoob modified the milestones: r97, r98 Sep 25, 2018
@mrdoob mrdoob modified the milestones: r98, r99 Oct 31, 2018
@mrdoob mrdoob removed this from the r99 milestone Nov 29, 2018
@mrdoob mrdoob added this to the r100 milestone Nov 29, 2018
@mrdoob mrdoob modified the milestones: r100, r101 Dec 28, 2018
@mrdoob mrdoob modified the milestones: r101, r102 Jan 31, 2019
@mrdoob mrdoob modified the milestones: r102, r103 Feb 28, 2019
@mrdoob mrdoob modified the milestones: r103, r104 Mar 27, 2019
@mrdoob mrdoob modified the milestones: r104, r105 Apr 24, 2019
@mrdoob mrdoob modified the milestones: r105, r106 May 30, 2019
@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 25, 2019

@mrdoob I think it's okay to merge this fix for consistency reasons.

@mrdoob mrdoob modified the milestones: r106, r107 Jun 26, 2019
@mrdoob mrdoob merged commit 2d19c07 into mrdoob:dev Jun 26, 2019
@mrdoob
Copy link
Owner

mrdoob commented Jun 26, 2019

Thanks!

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.

4 participants