Updates #1
Draft
Updates #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…re not. So I put a prefix of 'VHACD_' in front of all #defines in the code to avoid any chance of name clashing with user code.
… or as a series of Wavefront OBJ files
…OBJ to 'decomp.obj' but also in ASCII STL format as 'decomp.stl'. If you use the '-o stl' command line option, it will also save out each individual convex hull into a separate STL file.
fixed memory leaking
…nal example / test files
Very minor fixes
This makes the loading of the full file easier as all different single objects are visualized and useable in different tools for example blender.
…objects_to_full_decomp_obj_file feat(TestVHACD): add single objects to full decomp.obj file.
… generate test example is extremely slow, roughly 14 times slower
Change cmake command to include Release type
…ill suppress logging messages.
Use std::invoke_result instead of std::result_of to support C++20
…ariable names.
There's a couple of refactors. I don't like assumptions about memory allocations that were made in a number of places.
Reference style is the interface.
No indentation for top level declarations.
Class/struct names are PascalCase.
Variable names are camelCase, member variable names are m_camelCase, function names are PascalCase, member function names are PascalCase.
Class member functions are defined out of line.
Removed the multiple namespace VHACD parts. There's now only two namespace VHACD, the interface at the top, and the implementation.
Interesting thing to note. Determinant3x3(double[3][3], double*) and Determinant3x3(Googol[3][3]) both return -determinant of their argument.
They also do a bunch of looping to create the cofactor matrix that is used for Laplace expansion.
The order for that is a(ei - hf) - b(di * gf) + c(dh * ge), but the Determinant3x3 function reverses the sign of a, b, and c. ie -a(...) + b(...) - c(...)
There's another determinant function, inline double det(...) that calculates it correctly.
I've fixed Determinant3x3 by manually unrolling the loops, removing the Determinant2x2 function that was only used in there, and removing the det function.
Determinant3x3 was only used in ConvexHullFace::Evalue(), so that also returns a proper value.
ConvexHullFace::Evalue() is only used in ConvexHull::CalculateConvexHull3d() as a greater than comparison to zero, so I fixed those up.
Hopefully the new code is easier to understand, as I had a hell of a time understanding what was going on in Determinant3x3() and why it was different to det().
Removed VHACD::List<T>. It was only used in ConvexHull::CalculateConvexHull3d(). Replaced it with std::list. No change in performance, but more reduced code.
Unfortunate part of that bit of code is both boundaryFaces and ConvexHull::m_list are std::list<ConvexHullFace>, so it ends up being a mess of std::list<ConvexHullFace>::iterator with the iterators referring to different containers.
The original code was no better, with VHACD::List<ConvexHullFace>::ndNode* taking the place of std::list<ConvexHullFace>::iterator.
KdTreeNodeBundle is another linked list. Replaced the list part with std::list. Could replace it with std::forward_list since it's only storing elements, never iterated.
Expanded it into a generic NodeBundle class and used it in ConvexHull for memory allocations.
ConvexHull was using a std::vector for memory allocations, but had to make sure it never got reallocated.
This way, it allocates in bundles, same as KdTree does, which retains pointer stability.
Removed VHACD::Swap. std::swap is in the <utility> header and does the same thing.
VHACD::Sort does a three way comparison, so is not suitable for replacement for std::sort
Used = default where possible for constructors, along with in-class member initializers.
Added compatible type constructor to VHACD::Vect3<T>. This allows to construct a VHACD::Vect3<Googol> from a VHACD::Vect3<double>.
Helps in ConvexHullFace::Evalue(), as both matrix and exactMatrix can be initialized using vector math, which I think better represents what's going on there.
Replaced lots of double(1.0f) or similar with double(1.0) or whatever was in parentheses. With a find/replace on double, this can allow customization of the real type, eg for allowing it to run with float, long double, or __float128 for whatever reason if someone wants it.
ConvexHull::BuildHull(), ConvexHull::BuildTreeNew(), and ConvexHull::BuildTreeOld() were doing something strange.
BuildHull() was allocating and initializing a std::vector<ConvexHullAABB3DPointCluster> with treeCount + 256 elements. ie it was allocating and constructing a bunch of classes of non-trivial type.
It was then passing a pointer to the allocated memory and the size of the allocated memory to ConvexHull::InitVertexArray which then passed it along to ConvexHull::BuildTree*().
BuildTree*() was then placement newing either a ConvexHullAABB3DPointCluster or ConvexHullAABBTreeNode on the pointer, incrementing the pointer by the size of the constructed type, and doing it all over again.
Essentially, it was slicing up the memory of other objects. This would've been an issue if either of them had non-trivial destructors, but they didn't.
It did mean that reading through the code was harder than necessary since what was this placement new doing here? Where'd the memory come from? What's this type cast doing?
It wasn't saving memory either, since the same amount of memory was allocated regardless.
In short, it was weird code to construct a tree. I changed it to just emplace_back a new element, and then take the address of said element.
There's asserts to check that the std::vector doesn't reallocate since pointers to the allocated elements are used.
Moved BoundsAABB from a struct inside AABBTreeImpl (renamed AABBTree) to a utility class in the interface.
There was duplicated code and it makes sense for the AABB to be part of the ConvexHull, given there's an AABB in there already.
Didn't replace the mBmin and mBmax members because I think I've fiddled with the API enough. Also didn't rename them for the same reason.
Volume had a duplicate AABB bounds computing function, replaced with BoundsAABB where required.
VHACD::Voxel is constructed from 3 uint32_t's, where each should only be in the range [0,1024), but this isn't enforced anywhere. Added asserts to ensure it's never constructed with values outside of that range.
VoxelHull::FindConcavity{X,Y,Z} was 90% duplicate code. Only really changed in the inner loops. Replaced with a call to FindConcavity that takes an index and then switches on it in relevant places.
Removed RaycastMesh and Voxelize classes. They just passed their arguments onto AABBTree and Volume respectively. Implemented the required parts directly in AABBTree and Volume's functions.
Removed ShrinkWrap class. It was just a function.
Removed manual memory management in VHACDImpl. Lots of new and delete and storing the pointers in vectors and manually deleting them before clearing the vector.
Now they're vectors of std::unique_ptr so a lot fewer deletes and no need to make sure it's gone.
Fixed memory leak in VHACDImpl::findNearestConvexHull. It was allocating AABBTrees and storing them in a vector, but was never deleting them, only clearing the vector.
Renamed MyHACD_API to VHACDAsyncImpl to better describe what it is.
Changed m_VHACD from pointer to plain member. It's allocated in the constructor anyway, so let's just allocate it at the same time.
There's more I'd like to do, but it always ends up being c++14, 17, or 20.
The line change count is misleading, since it's primarily unindenting code.
Finishing the cleanups I started in the last PR
correct typographical errors in 6 source files
Renamed shadowed CostTask from t to ct, t is already the name of the Timer declared on 7548
Removed greater than or equal comparisons to zero on unsigned variables.
Remove unused variable count from ThreadPool
Fix CostTask t shadowing Timer t
Remove the upscaling of the voxel size in shrink-wrap
Make file formatting comply with POSIX and Unix standards
Print the value that the user used when printing out invalid ranges. Also add a space after colons. Signed-off-by: Gaël Écorchard <gael@km-robotics.cz>
Print the user value when invalid
Update comment for m_minEdgeLength to match code
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.