Skip to content

Conversation

@Wilt
Copy link
Contributor

@Wilt Wilt commented Sep 1, 2016

Hej Collin Hover,

I found some time to more seriously work my way through your threeoctree.js
I changed quite some things, but I hope you can agree on the changes i made in this PR. I think the code for raycasting/intersecting was simplified a bit by my changes. The advantage of this refactor is that there is no longer need for customizing the THREE.Raycaster class. I needed to borrow some code from the raycast method in the THREE.Mesh class to optimize the raycasting for the useFaces case. Sorting of objects is as a result now internally handled by the raycaster and no longer part of the octree code.

I also updated the docs and added a new changelog for migration to r78.
And added bower.json for bower support.

The examples are also updated and all seems to work fine.

The face count (numFaces) shows a bit of a different number, but that is because I did think that the the faces for objects that still need to be raycasted (the boxes) should be included in the count (that is a more fair comparison).

If you want any changes please leave a comment.

Here the changes:

  • Removed extending THREE.RayCaster with custom methods
  • Added raycast method on THREE.Octree
  • Added raycast method on THREE.OctreeObjectData
  • Removed THREE.Face4 support and its corresponding methods (THREE.Face4 was deprecated in r60)
  • Removed exchanging of faces array on geometry while intersecting THREE.Octree
  • Removed getting radius from object.boundRadius (was replaced in r66(?) by object.geometry.boundingSphere.radius)
  • Renamed this.vertices inside THREE.OctreeObjectData to this.vertex (it holds an instance of THREE.Vector3)
  • Renamed this.faces inside THREE.OctreeObjectData to this.face (it holds an instance of THREE.Face3)
  • Renamed getFace3BoundingRadius back to previous getFaceBoundingRadius (since THREE.Face4 support is removed)
  • Changed code formatting @mrdoob style (https://zz85.github.io/mrdoobapproves/)

Next will be to check support for THREE.BufferGeometry instances...

Note: I removed my findClosestVertex. I realized the method was incomplete, it was only working for objects added with useVertices. I will think about what to do with it. Maybe I will make another PR later, for now I think it would be better to make two octrees in such case. One specially for finding closest vertex where objects are added by vertices only...

@Wilt
Copy link
Contributor Author

Wilt commented Sep 1, 2016

One more suggestion I would like to make is to reconsider the organizeByObject param. It seems this functionality lost it purpose and I am not totally sure what it could be used for right now... I also think the results returned from that method are a bit confusing. It only returns faces and vertices for the objects that were added by vertex/face. Maybe you can give a user case/example where this method would still be used?
If you would like to keep it there should maybe be some more documentation added on what this method does. It could also be moved to a separate sortResults method where octreeResults can be sorted in different ways.

Another improvement could be to split the THREE.OctreeObjectData into three different classes. For example:

  • THREE.OctreeObjectData
  • THREE.OctreeFaceData
  • THREE.OctreeVertexData

Like that we can make 3 diffent raycast methods that are specific to the object. Now the method has to cover all three cases.
Such a solution could eliminate all the additional if/else clauses where there is a need to check with what data the class is dealing with. The data would be constructed with a separate class that has the corresponding logic in its prototyped methods.

Please tell me what you think about this...

@Wilt
Copy link
Contributor Author

Wilt commented Sep 1, 2016

I added a test/example for raycasting on THREE.BufferGeometry. It seems to work correctly for adding objects as normally. I will still add changes for support for face and vertices. I think it would be another reason to split into three separate THREE.OctreeObjectData classes, since it would need to explicitly check for BufferGeometry instances in those cases and extract face/vertex from the index and/or position THREE.BufferAttribute.

@Cobertos
Copy link

+1, can this get PR get some love?

I need this functionality. I also have built functionality to support the faces mode on THREE.BufferGeometry but wanted to see this go through first. (It's just really slow for some reason, not sure if this library is supposed to be able to handle 500k faces?).

@collinhover
Copy link
Owner

Not sure how I missed this PR. This is a huge amount of work and I apologize for not seeing it sooner. Honestly I haven't used THREEJS in too long and don't have the time to review your changes to make the call on whether this is safe to merge. I'll leave the PR open for now so others can use your changes as needed.

@mrdoob
Copy link

mrdoob commented Feb 27, 2018

Maybe to avoid confusion you can point people to the main repo?
We make sure Octree.js stays up to date 🙂

https://github.com/mrdoob/three.js/blob/dev/examples/js/Octree.js

@collinhover
Copy link
Owner

Done: https://github.com/collinhover/threeoctree/blob/master/README.md

Thanks guys.

@3kynox
Copy link

3kynox commented Jan 1, 2020

Any news on this? That's just very sad that I don't have the knowledge yet to achieve this conversion.
Does this conversion have been done by you in this PR @Wilt ?
Does it works with bufferGeometry in your examples or there's still work to do?

Thanks anyway :)

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.

5 participants