Skip to content

Made triangle barycentric coordinates possible in 2D #1207

Merged
fsimonis merged 7 commits intoprecice:developfrom
boris-martin:barycentric_triangle
Mar 24, 2022
Merged

Made triangle barycentric coordinates possible in 2D #1207
fsimonis merged 7 commits intoprecice:developfrom
boris-martin:barycentric_triangle

Conversation

@boris-martin
Copy link
Copy Markdown
Contributor

@boris-martin boris-martin commented Mar 19, 2022 •
edited
Loading

Main changes of this PR

  • precice::math::barycenter::calcBarycentricCoordsForTriangle now works with triangles in a 2D mesh.
  • Added a unit test for correctness. (A copy of the existing one in 3D but with the Z-component removed, basically)
  • Removed the assertion dimensions == 3 in the Triangle constructor.

Motivation and additional information

Prerequisite to volume coupling.

Author's checklist

  • I added a changelog file with make changelog if there are user-observable changes since the last release.
  • I ran make format to ensure everything is formatted correctly.
  • I sticked to C++14 features.
  • I sticked to CMake version 3.10.
  • I squashed / am about to squash all commits that should be seen as one.

Reviewers' checklist

@fsimonis is a changelog needed here? I guess not but I didn't check whether there's somewhere written that you can't add a triangle in 2D where it should be changed.
The algorithm for 3D seemed hard to adapt in 2D so I designed a new one with pen & paper. Not sure it is optimal but I think it is cheap enough.
However, I'm not sure whether an if-else in the function is a better design choice than two separate functions or not.

  • Does the changelog entry make sense? Is it formatted correctly?
  • Do you understand the code changes?

@boris-martin boris-martin requested a review from fsimonis March 19, 2022 23:49
fsimonis
fsimonis reviewed Mar 21, 2022
View reviewed changes
Comment thread src/math/barycenter.cpp Outdated
uc = c - u;
ua = a - u;

scaleFactor = 1.0 / crossProduct2D(ab, ac);
Copy link
Copy Markdown
Member

@fsimonis fsimonis Mar 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This will crash for zero vectors among others. Can you assert this isn't 0?

Copy link
Copy Markdown
Contributor Author

@boris-martin boris-martin Mar 21, 2022

Choose a reason for hiding this comment

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

It wasn't checked in 3D either, so I just added assertions for both cases.

@boris-martin
Copy link
Copy Markdown
Contributor Author

boris-martin commented Mar 21, 2022

@fsimonis I just realized in (surface) NP there is a test case that uses a degenerate triangle, so my new assertion makes the test fail.

precice/src/mapping/tests/NearestProjectionMappingTest.cpp

Lines 343 to 349 in 5749690

Vertex &v1 = inMesh->createVertex(Eigen::Vector3d(0.0, 0.0, 0.0));
Vertex &v2 = inMesh->createVertex(Eigen::Vector3d(1.0, 1.0, 0.0));
Vertex &v3 = inMesh->createVertex(Eigen::Vector3d(2.0, 2.0, 0.0));
Edge & e12 = inMesh->createEdge(v1, v2);
Edge & e23 = inMesh->createEdge(v2, v3);
Edge & e31 = inMesh->createEdge(v3, v1);
inMesh->createTriangle(e12, e23, e31);

Is that intended?

@fsimonis
Copy link
Copy Markdown
Member

fsimonis commented Mar 21, 2022

The API currently allows such triangles, so we should we support them here as well.

Maybe we could/should extend the checks of the input mesh.

@boris-martin
Copy link
Copy Markdown
Contributor Author

boris-martin commented Mar 21, 2022

I don't get what you would do. Allow these triangles only when NP is not used? (Then the test case still divides by zero)
Add some hack such that interpolation in this triangle becomes an edge interpolation between two arbitrary vertices of that degenerate triangle?

@fsimonis fsimonis added this to the Version 2.4.0 milestone Mar 22, 2022
@fsimonis
Copy link
Copy Markdown
Member

fsimonis commented Mar 22, 2022

Ok, supporting degenerate triangles is nonsense here.

I propose to do the following in this PR:

  • Assert that the triangles are "valid" in the projection logic. (as you did)
  • Remove tests testing degenerate triangles.

In another PR:

  • Check the triangles to be valid in the SolverInterface and raise a PRECICE_ERROR. I'll do this asap.

@boris-martin
Copy link
Copy Markdown
Contributor Author

boris-martin commented Mar 22, 2022

I removed the degenerate test and all checks have passed :)

@fsimonis
Copy link
Copy Markdown
Member

fsimonis commented Mar 24, 2022

Perfect. I'll merge this and prepare additional checks in the API.

@fsimonis fsimonis merged commit 5dedf2a into precice:develop Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fsimonis fsimonis fsimonis left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Version 2.4.0

Development

Successfully merging this pull request may close these issues.

2 participants

@boris-martin @fsimonis