-
Notifications
You must be signed in to change notification settings - Fork 63
Bugfix: Curved geometries #2073
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2073 +/- ##
=======================================
Coverage 77.05% 77.05%
=======================================
Files 112 112
Lines 18959 18959
=======================================
Hits 14608 14608
Misses 4351 4351 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lenaploetzke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First part of review
| } | ||
| } | ||
|
|
||
| /* Lastly, we put the parameters into our output array. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok i think overall this function works.
| /* Convert reference parameters to OCCT point */ | ||
| gp_Pnt2d reference_point (reference_face_params.value ()[0], reference_face_params.value ()[1]); | ||
|
|
||
| /* If the edge is a seam we have to get both points and check which one is closer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
points? parameters maybe?
| We iterate over all edges of the face and check if the edges are the same as the | ||
| input edge. Then we check if the converted parameters are closer to the | ||
| already computed parameters. */ | ||
| bool first_point = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edge?
| if (edge.IsSame (current_edge)) { | ||
| Handle_Geom2d_Curve curve_on_surface = BRep_Tool::CurveOnSurface (edge, face, first, last); | ||
| /* If it is the first seam we compute the parameters. */ | ||
| if (first_point) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can also again solve this with uv as optional can't you?
| } | ||
| /* Now we can check if every node lies on the surface and retrieve its parameters */ | ||
| if (surface_index) { | ||
| int all_nodes_on_surface = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use maybe a bool? 😅
Typos Co-authored-by: lenaploetzke <70579874+lenaploetzke@users.noreply.github.com>
lenaploetzke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also merge main
| #include <ranges> | ||
| #include <type_traits> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check if you need these two includes?
| if (dim == 2) { | ||
| face_nodes[i_face_node] = tree_nodes[i_face_node]; | ||
| face_nodes.push_back (tree_nodes[i_face_node]); | ||
| T8_ASSERT (num_faces == 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? Only chance that num_faces is set is here
switch (dim) {
case 0:
num_faces = 0;
break;
case 1:
num_faces = 0;
break;
case 2:
num_faces = 1;
break;
case 3:
num_faces = t8_eclass_num_faces[eclass];
break;
default:
SC_ABORTF ("Invalid dimension of tree. Dimension: %i\n", dim);
}
| for (auto &face_node : face_nodes) { | ||
| switch (face_node.entity_dim) { | ||
| case 2: | ||
| /* Nothing to do. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont have to convert because the face node is directly on surface right?
| * If the edge is locked for edges on surfaces we have to skip this edge */ | ||
| else if (edge_geometry_dim == 2 && edge_geometries[i_tree_edges + num_edges] >= 0) { | ||
|
|
||
| /* We also skip this exge if the edge is on a plane. Planes have no curvature and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /* We also skip this exge if the edge is on a plane. Planes have no curvature and | |
| /* We also skip this edge if the edge is on a plane. Planes have no curvature and |
Closes #2074
Describe your changes here:
I am so sorry for the diff count. But a huge part of this PR is documentation.
Fixes a bug in our curved mesh algorithms in which in some edge cases geometries were either not linked to tree faces or they were linked with wrong parameters. This can be observed by executing the curved mesh tutorial for tetrahedra on main and on this branch.
All these boxes must be checked by the AUTHOR before requesting review:
Documentation:,Bugfix:,Feature:,Improvement:orOther:.All these boxes must be checked by the REVIEWERS before merging the pull request:
As a reviewer please read through all the code lines and make sure that the code is fully understood, bug free, well-documented and well-structured.
General
Tests
If the Pull request introduces code that is not covered by the github action (for example coupling with a new library):
Scripts and Wiki
script/find_all_source_files.scpto check the indentation of these files.License
doc/(or already has one).