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

Polygon DEC #195

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Polygon DEC #195

wants to merge 21 commits into from

Conversation

nzfeng
Copy link
Collaborator

@nzfeng nzfeng commented Sep 17, 2024

Adds polygon Laplacian and other operators for general polygon meshes.

Summary of contributions:

  • implementation of polygon Laplacian and mass matrices (unlumped & lumped) from Polygon Laplacian Made Simple by Bunge et al. 2020.
  • implementation of polygon Laplacian, mass matrix, gradient, divergence, connection Laplacian, and DEC operators from Discrete Differential Operators on Polygonal Meshes by de Goes et al 2020.
  • A PolygonMeshHeatSolver that implements the Vector Heat Method, Unsigned Heat Method, and Signed Heat Method on polygon meshes, analogous to the PointCloudHeatSolver for point clouds.
  • Documentation & tests.

image

The only thing I'm not quite satisfied with is the naming convention for the operators from "Polygon Laplacian Made Simple": geometry.simplePolygonLaplacian, geometry.simplePolygonVertexLumpedMassMatrix, and geometry.simplePolygonVertexGalerkinMassMatrix. My concerns are

  • the names can be quite long;
  • perhaps the word "simple" will be mistaken as meaning "less sophisticated" and therefore "less accurate";
  • perhaps the operators will be mistaken as only applying to "simple polygons", which are non-self-intersecting polygons without holes, though they can indeed apply even when polygon faces are self-intersecting.

Suggestions & review are welcome!

@nzfeng
Copy link
Collaborator Author

nzfeng commented Sep 17, 2024

@MarkGillespie or @nmwsharp, adding this support for polygon meshes required making some changes for EmbeddedGeometryInterface --- mainly removing the asserts that assumed triangular faces in functions like computeCornerAngles() and computeFaceAreas().

I don't foresee that these changes will result in breaking behavior (especially since all tests have passed), but it might also be a good idea to have a geometry-central expert check the details in surface/embedded_geometry_interface.cpp!

@fire
Copy link

fire commented Sep 25, 2024

perhaps the word "simple" will be mistaken as meaning "less sophisticated" and therefore "less accurate";

What do you think of this argument?

geometry.basePolygonLaplacian, geometry.basePolygonVertexLumpedMassMatrix, and geometry.basePolygonVertexGalerkinMassMatrix.

Rename simple to base.

Like the base polygons are unrestricted, while for example manifold triangle polygon has numerous restrictions.

Edited:

See also general.

Copy link
Owner

@nmwsharp nmwsharp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!! I left a few comments, but nothing major. Looks great.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we define == with a hidden internal eps!?? If I did that it was a silly mistake... This is breaking change but a good one I think.

@@ -263,11 +263,11 @@ inline bool SurfacePoint::operator==(const SurfacePoint& other) const {
break;
}
case SurfacePointType::Edge: {
return edge == other.edge && abs(tEdge - other.tEdge) < eps;
return edge == other.edge && tEdge == other.tEdge;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you delete the unused eps variable above in this function?

DependentQuantityD<std::array<Eigen::SparseMatrix<double>*, 7>> polygonDECOperatorsQ;
virtual void computePolygonDECOperators();

// helper functions
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are a lot of helper functions. I'm worried this class/file is getting too busy.

Could we maybe move these helpers to a separate file, and invoke them as-needed? And maybe once moved to their own file they no-longer need to be virtual?

@@ -278,11 +288,11 @@ void EmbeddedGeometryInterface::computeCornerAngles() {
Vector3 pA = vertexPositions[he.vertex()];
he = he.next();
Vector3 pB = vertexPositions[he.vertex()];
he = he.next();
do {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function has quadratic complexity in the face degree, when it doesn't need to. It could be implemented with linear complexity by working one face at a time and maintaining a leading a trailing reference while orbiting the face.

[doesn't need to be fixed, just couldn't help but mention it :)]

@nmwsharp
Copy link
Owner

nmwsharp commented Oct 1, 2024

Re naming for simple polygon Laplacian (thanks for the ideas @fire!). Personally I think the name 'simple' is perfectly good. It matches the title of the paper it comes from. We also already use the term elsewhere in SimplePolygonMesh. Any confusion about the term will be quickly resolved by reading the docs.

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.

3 participants