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

Adds Trimming Curve Support for primal::NURBSPatch #1503

Open
wants to merge 34 commits into
base: develop
Choose a base branch
from

Conversation

jcs15c
Copy link
Contributor

@jcs15c jcs15c commented Feb 17, 2025

Summary

  • This PR is a feature, enhancing the primal::NURBSPatch class to maintain and manipulate an unstructured array of primal::NURBSCurve objects to be used for trimming the parameter space of the original patch.
  • If a point in the parameter space is outside the collection of trimming curves, it is not visible. Containment within the array of trimming curves is computed using 2D generalized winding numbers and an EvenOdd rule on the resulting decimal value. These features are tested in the primal_trimming_curves.cpp file.
  • Trimmed surfaces are identified by a flag in the NURBS patch class, in order that trimmed patches can be defined with an empty array of trimming curves, i.e., the entire patch is trimmed out.
  • Additionally, this PR separates the implementation details of 2D and 3D generalized winding numbers to avoid circular dependencies resulting from visibility queries of trimmed surfaces.
  • Currently, splitting a trimmed NURBS patch is not supported. This will be addressed in a future PR.

@jcs15c jcs15c marked this pull request as ready for review February 19, 2025 20:10
@jcs15c jcs15c self-assigned this Feb 19, 2025
@jcs15c jcs15c added enhancement New feature or request Primal Issues related to Axom's 'primal component labels Feb 19, 2025
Copy link
Member

@kennyweiss kennyweiss left a comment

Choose a reason for hiding this comment

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

Thanks @jcs15c for another great PR!

Comment on lines 6 to 9
/*!
* \file primal_nurbs_patch.cpp
* \brief This file tests primal's NURBS patch functionality
*/
Copy link
Member

Choose a reason for hiding this comment

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

copy/paste: Please update this for primal_trimming_curves

Comment on lines 54 to 71
// Add simple trimming curves
nPatch.makeTriviallyTrimmed();
EXPECT_TRUE(nPatch.isTrimmed());
EXPECT_EQ(nPatch.getNumTrimmingCurves(), 4);

// Remove all trimming curves, while staying trimmed
nPatch.clearTrimmingCurves();
EXPECT_TRUE(nPatch.isTrimmed());
EXPECT_EQ(nPatch.getNumTrimmingCurves(), 0);

// Remove all trimming curves
nPatch.makeUntrimmed();
EXPECT_FALSE(nPatch.isTrimmed());

// Mark as trimmed, but empty vector of trimming curves
nPatch.makeTrimmed();
EXPECT_TRUE(nPatch.isTrimmed());
EXPECT_EQ(nPatch.getNumTrimmingCurves(), 0);
Copy link
Member

Choose a reason for hiding this comment

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

Do you think users might get tripped up by

  NURBSPatch::makeTriviallyTrimmed();  // adds trimming curves for boundaries
  NURBSPatch::makeTrimmed();           // does not add trimming curves; presumably entire surface is trimmed away

I'm not sure if I have a good suggestion other than maybe markAsTrimmed() instead of makeTrimmed()
(following your comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

markAsTrimmed() makes sense to me, sounds more intuitive than setTrimmed()

@@ -1000,6 +1000,7 @@ bool intersect(const Ray<T, 3>& ray,
axom::Array<T>& v,
double tol = 1e-8,
double EPS = 1e-8,
bool isTrimmed = true,
Copy link
Member

Choose a reason for hiding this comment

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

How does this parameter relate to NURBSPatch::isTrimmed()?

Copy link
Contributor Author

@jcs15c jcs15c Feb 22, 2025

Choose a reason for hiding this comment

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

There are some cases where a patch is trimmed, but you want to find intersections with the untrimmed patch, like recording "near-misses" with the visible portion. I should rename this variable to make that more clear, maybe to treatAsTrimmed or reverse the implementation and have it be treatAsUntrimmed/considerUntrimmed?

@@ -7,7 +7,7 @@
* \file winding_number.hpp
*
* \brief Consists of methods to compute the generalized winding number (GWN)
* for points with respect to various geometric objects.
* for points with respect to various 2D geometric objects.
Copy link
Member

Choose a reason for hiding this comment

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

Clarification: This file has both 2D and 3D functions, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! Fixed that.

Copy link
Member

@kennyweiss kennyweiss left a comment

Choose a reason for hiding this comment

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

Some additional minor comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Primal Issues related to Axom's 'primal component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants