search

davideberly / GeometricTools

A collection of source code for computing in the fields of mathematics, geometry, graphics, image analysis and physics.
Boost Software License 1.0
1.08k stars 202 forks source link

MinimumVolumeBox3: Adjust class data to make it reusable for multiple data sets #43

Closed Leopard20 closed 1 year ago

Leopard20 commented 1 year ago

The primary problem was that mEdgeIndices was not being reset (it was being reserved and pushed to). I also changed convex hull to be a member. Not sure if it was a good idea. But it works fine in my tests (single-threaded)

This is a lazy optimization but at least it allowed me to save a few seconds for thousands of data sets.

davideberly commented 1 year ago

The resetting of members is fine. However, would you mind explaining why you replaced the local ConvexHull3 (ch3) by a class member (mConvexHull3)? There is no public accessor to mConvexHull3, and the constructor for ConvexHull3 has minimal overhead (all std::vector are defaulted to empty). Thanks.

Leopard20 commented 1 year ago

I didn't look too deep into the ConvexHull3 class, but based on a quick look I had back then I think it used hashmaps and vectors in its members. So I figured changing it to a memeber would minimize reallocations if the previous sizes of these containers were greater than the current required sizes. But if they don't have a lot of overhead I will put it back.

davideberly commented 1 year ago

It is difficult to determine statically whether or not the resizes in ConvexHull3 are better than starting with a new object each time. Have you run a profiler to find out whether it does make a difference? Regardless, having a member mConvexHull3 instead is probably useful in case someone wants that hull in addition to the minimum volume box. If you do not mind, add a read-only accessor to that object, say, "ConvexHull3 const& GetConvexHull3() const { return mConvexHull3; }" and I will merge your pull request with the code base.

Thank you for taking the time for providing feedback and modifying the code.

Leopard20 commented 1 year ago

Have you run a profiler to find out whether it does make a difference?

I will definitely try this later to see if it makes a noticeable difference.

If you do not mind, add a read-only accessor to that object, say, "ConvexHull3 const& GetConvexHull3() const { return mConvexHull3; }" and I will merge your pull request with the code base.

Added. I also added a note that this convex hull is not generated when using the overload that already uses the convex hull.

Thank you for taking the time for providing feedback and modifying the code.

No problem. Thank you very much for sharing this extremely useful library. It has saved me a lot of time.