Use flying edges for mesh generation#1741
Conversation
Signed-off-by: Perminder <permindersingh089@gmail.com>
Signed-off-by: Perminder <permindersingh089@gmail.com>
Signed-off-by: Perminder <permindersingh089@gmail.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Perminder <permindersingh089@gmail.com>
|
Here are the build results |
Signed-off-by: Perminder <permindersingh089@gmail.com>
| // Add triangles | ||
| const char* caseTri = caseTriangles[caseId]; // size 16 | ||
| for(int idx = 0; caseTri[idx] != -1; idx += 3) | ||
| { | ||
|
|
||
|
|
||
| size_t globalIdx = globalIdxs[caseTri[idx]]; | ||
| size_t globalIdx1 = globalIdxs[caseTri[idx + 1]]; | ||
| size_t globalIdx2 = globalIdxs[caseTri[idx + 2]]; | ||
|
|
||
|
|
||
|
|
||
| m_vertices.push_back(points[globalIdx]); | ||
| m_vertices.push_back(points[globalIdx1]); | ||
| m_vertices.push_back(points[globalIdx2]); | ||
|
|
||
| m_normals.push_back(normals[globalIdx]); | ||
| m_normals.push_back(normals[globalIdx1]); | ||
| m_normals.push_back(normals[globalIdx2]); | ||
|
|
|
I can do cleanups in the code once I start getting the expected output. Also tagging @matterhorn103 and @aerkiaga for their inputs. Let me know if you guys have any feedbacks/suggestions? |
|
@perminder-17 I'm afraid rendering is something I know absolutely nothing about |
Aah... Thanks for your reply. No problem, I figured that out. It was like, we need to pass the triangles as well. |
|
Great, I'll start a review tomorrow |
Signed-off-by: Perminder <permindersingh089@gmail.com>
Signed-off-by: Perminder <permindersingh089@gmail.com>
| unsigned char edgeCaseZ = edgeCases[edgeCaseIdxZ]; | ||
|
|
||
| // If the sum is odd, the edge along the z-axis is cut | ||
| if ((edgeCase + edgeCaseZ) % 2 == 1) |
Check warning
Code scanning / CodeQL
Bad check for oddness
|
Here are the build results |
ghutchis
changed the title
|
This pull request has been mentioned on Avogadro Discussion. There might be relevant details there: https://discuss.avogadro.cc/t/november-2024-live-updates/6446/1 |
Signed-off-by: Perminder <permindersingh089@gmail.com>
… variables Signed-off-by: Perminder <permindersingh089@gmail.com>
Signed-off-by: Perminder <permindersingh089@gmail.com>
Signed-off-by: Perminder <permindersingh089@gmail.com>
|
Hi everyone, Here's the results after the implementation of flying-edges. requesting @ghutchis for the review, still needs some cleanups in the code, removal of unnecessary codeblocks and variables. Marching cube takes 1.5 minutes to render the mesh
Flying edges takes only 3 seconds to render it. (could vary with different systems)
Also it produces a little smoother mesh (left->flying edges, right->marching cubes)
After Laplacian smoothing we have:
|
|
I'm happy to help do some performance tests - I'm not seeing the "Time taken" code at the moment in your pull request. I have a range of molecule sizes and resolutions to give this a good benchmark. Doing a rough stopwatch count, I'm seeing ~2-2.5x performance improvement, but that's including the whole thing, not just mesh generation. I can say that with caffeine as an example, the flying edges version appears instantly on my MacBook even at "Very High" resolution. With a larger molecule, it's going from ~10s to display the VdW mesh to ~4.5s. If you want to send me the patch you used for timing, I'd be happy to perform some real benchmarks tonight in release mode. |
Thanks @cryos for your insights. Really helps a lot and also thanks @ghutchis for the help in performance testing. If I could say what code I used for checking the time, I used the chrono library https://en.cppreference.com/w/cpp/chrono/high_resolution_clock/now If you include header #include <chrono> // at the top (header file)
// Rest code remains the same
// Replace the run function to test
void MeshGenerator::run() // you can directly copy the run() function and paste it with the original ones
{
if (!m_cube || !m_mesh) {
qDebug() << "No mesh or cube set - nothing to find isosurface of…";
return;
}
m_mesh->setStable(false);
m_mesh->clear();
auto start = std::chrono::high_resolution_clock::now();
FlyingEdgesAlgorithmPass1();
FlyingEdgesAlgorithmPass2();
FlyingEdgesAlgorithmPass3();
FlyingEdgesAlgorithmPass4();
auto end = std::chrono::high_resolution_clock::now();
std::chrono::duration<double> elapsed = end - start;
qDebug() << "Flying Edges Time: " << elapsed.count() << " s";
m_mesh->setVertices(points);
m_mesh->setNormals(normals);
m_mesh->setTriangles(tris);
m_mesh->smooth(m_passes);
m_mesh->setStable(true);
points.resize(0);
normals.resize(0);
tris.resize(0);
}
also, if you want to test and compare the time with marching cubes. here how you can check the time taken for marching cubes as well. OLD CODE#include <chrono> // similarly include this
// just a little changes in the run function
void MeshGenerator::run()
{
// rest code remains the same.
auto start = std::chrono::high_resolution_clock::now(); // add this
for (int i = 0; i < m_dim.x() - 1; ++i) {
for (int j = 0; j < m_dim.y() - 1; ++j) {
for (int k = 0; k < m_dim.z() - 1; ++k) {
marchingCube(Vector3i(i, j, k));
}
}
if (m_vertices.capacity() < m_vertices.size() + m_dim.y() * m_dim.x() * 3) {
m_vertices.reserve(m_vertices.capacity() * 2);
m_normals.reserve(m_normals.capacity() * 2);
}
emit progressValueChanged(i);
}
// add this
auto end = std::chrono::high_resolution_clock::now();
std::chrono::duration<double> elapsed = end - start;
qDebug() << "Marching Cube Timing: " << elapsed.count() << " s";
// rest code remains the same
}
|
Also, after the merge of this, I can handle the merge conflicts (if any) in this PR #1692. If we add a progressive refinement, it won't take even 4.5 seconds to render and will generate the mesh in no time. Do let me know if everything looks fine in the results, I could do many cleanups in the code (added too many debugging peices, unnecessary functions, variables haha). Still needs some formatting and cleanups. |
Signed-off-by: Perminder <permindersingh089@gmail.com>
|
Btw @ghutchis I just noticed one bug (when you click on the calculate button twice for the same mesh generation it takes more time). It's because we were not clearing the memory. In the above commit I have fixed that as well. Sorry for the oversight. |
|
I'm seeing ~2x - 2.2x speedup for mesh generation on a range of 7 molecules with 0.05Å resolution. It depends a lot on the size of the molecule and the shape (e.g., smaller molecules and more spherical / cubic see the greatest speedup). I'll do a bit of profiling, but of course this is also the single-core implementation (vs. parallel) |
|
Correct! I looked at the results mentioned on the paper. They also have 2-2.2x speedups. I'll do some cleanups in the code tomorrow. |
|
I also checked the number of vertices for marching cubes vs. flying edges. It's getting exactly 6.0 times fewer vertices, so the mesh is much smaller at the same resolution (and thus faster to render, less memory, etc.) Even on a really big DNA chunk, I'm seeing ~0.1s for mesh creation with the "automatic" resolution. (And yes, the progressive refinement could be a help here too.) |
Signed-off-by: Perminder <permindersingh089@gmail.com>
|
|
||
| float Cube::getData(int i, int j, int k) const | ||
| { | ||
| int nx = m_points.x(); |
Check notice
Code scanning / CodeQL
Unused local variable
|
Here are the build results |
ghutchis
left a comment
ghutchis
left a comment
There was a problem hiding this comment.
Mostly some small things with the comments and documentation consistency.
| std::array<float, 3> computeGradient(int i, int j, int k) const; | ||
|
|
||
| /** | ||
| * Get the values of the eight corners of a cube cell. |
There was a problem hiding this comment.
I'm not quite sure I understand from the docs here. I've got some index (i, j, k) into the cube. What's the "cell" of the cube? Is it just going to get me the corners of the entire cube? If so, why do I need i, j, k parameters?
| float getData(int i, int j, int k) const; | ||
|
|
||
| /** | ||
| * Get the positions of the eight corners of a cube cell. |
There was a problem hiding this comment.
Again, what's the "cell" here? Are these the min, max of the corners of the cube? If so, why do you need the i, j, k?
| Core::Array<Vector3f> m_vertices; | ||
| Core::Array<Vector3f> m_normals; | ||
| Core::Array<Color3f> m_colors; | ||
| Core::Array<Vector3f> tris; |
There was a problem hiding this comment.
Please use m_triangles to be consistent with the other variables.
| */ | ||
| void progressValueChanged(int); | ||
|
|
||
| protected: |
There was a problem hiding this comment.
Can we keep this? Sometimes it's helpful to get normals for arbitrary points.
| */ | ||
| Vector3f normal(const Vector3f& pos); | ||
|
|
||
| unsigned long duplicate(const Vector3i& c, const Vector3f& pos); |
| std::vector<unsigned char> cubeCases; //size ((m_dim.x() - 1) * (m_dim.y() - 1) * (m_dim.z() - 1)) | ||
| std::vector<int> triCounter; // size ((m_dim.y() - 1) * (m_dim.z() - 1)) | ||
| std::vector<unsigned char> edgeCases; // size ((m_dim.x() - 1) * (m_dim.y()) * (m_dim.z())) | ||
| Core::Array<Vector3f> tris; // triangles of a mesh |
There was a problem hiding this comment.
Please remove any un-used variables (from Marching Cubes)
Please also use m_triangles (or similar) to keep consistent with member variables through the rest of the code.
| static const unsigned char numTris[256]; | ||
| static const bool isCut[256][12]; | ||
| static const char caseTriangles[256][16]; | ||
| static const unsigned char edgeVertices[12][2]; |
There was a problem hiding this comment.
The static analysis says these class members are never used?
If they're used, please keep to m_edgeVertices etc. for consistency.
Signed-off-by: Perminder <permindersingh089@gmail.com>
|
Here are the build results |
1 similar comment
|
Here are the build results |
|
|
||
| std::array<std::array<float, 3>, 8> pos; | ||
|
|
||
| float xpos = m_min.x() + (i * m_spacing.x()); |
There was a problem hiding this comment.
I'm sorry - I still don't understand what this method is supposed to do.
xpos, ypos, zpos are the real-space versions of i, j, k indices
.. so the method gives me a cube that's one step in each of the x, y, z directions?
If so, shouldn't that be the doc string?
| /** | ||
| * @class Cube cube.h <avogadro/core/cube.h> | ||
| * @brief Provide a data structure for regularly spaced 3D grids. | ||
| * @author Marcus D. Hanwell |
There was a problem hiding this comment.
@author Marcus D. Hanwell
@author Perminder Singh
| */ | ||
| const Core::Array<Vector3f>& normals() const; | ||
|
|
||
| /** |
There was a problem hiding this comment.
I still don't understand why you're removing numNormals() which seems like a useful method.
There was a problem hiding this comment.
But i was unable to find where we were using numnormals() in our actual code?
There was a problem hiding this comment.
We're not, currently. But since this is a core class, it's also API for future development.
ghutchis
left a comment
ghutchis
left a comment
There was a problem hiding this comment.
Mostly resolved - please note the other comments, thanks.
|
Here are the build results |
1 similar comment
|
Here are the build results |
Signed-off-by: Geoff Hutchison <geoff.hutchison@gmail.com>
ghutchis
left a comment
ghutchis
left a comment
There was a problem hiding this comment.
Sorry, I missed these last time. I think this fixes the compile on Windows.
| ge2.xstart += isCutCase[4]; | ||
| ge2.ystart += isCutCase[7]; | ||
| } | ||
| if(isXEnd and isYEnd) |
There was a problem hiding this comment.
&& not and - this is the build failure on Windows
| { | ||
| ge1.zstart += isCutCase[11]; | ||
| } | ||
| if(isXEnd and isZEnd) |
| { | ||
| ge2.ystart += isCutCase[5]; | ||
| } | ||
| if(isYEnd and isZEnd) |
| if(isCutCase[11]) | ||
| { | ||
| int idx = ge1.zstart + z1counter; | ||
| if(isXEnd and isYEnd) |
| if(isCutCase[5]) | ||
| { | ||
| int idx = ge2.ystart + y2counter; | ||
| if(isXEnd and isZEnd) |
| if(isCutCase[6]) | ||
| { | ||
| int idx = ge3.xstart + x3counter; | ||
| if(isYEnd and isZEnd) |
Signed-off-by: Perminder Singh <127239756+perminder-17@users.noreply.github.com>
|
Done @ghutchis |
|
Here are the build results |
|
Here are the build results |
6 participants
Resolves issue : #901
Developer Certificate of Origin
Version 1.1
Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129
Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.