-
Notifications
You must be signed in to change notification settings - Fork 32
Adds 3D winding number example to quest #1768
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
Conversation
| SLIC_ASSERT_MSG( | ||
| isValidParameter_u(u, 1e-5), | ||
| axom::fmt::format("Requested u-parameter {} is outside valid range [{},{}] with tolerance {}", | ||
| u, | ||
| getMinKnot_u(), | ||
| getMaxKnot_u(), | ||
| 1e-5)); | ||
| SLIC_ASSERT_MSG( | ||
| isValidParameter_v(v, 1e-5), | ||
| axom::fmt::format("Requested v-parameter {} is outside valid range [{},{}] with tolerance {}", | ||
| v, | ||
| getMinKnot_v(), | ||
| getMaxKnot_v(), | ||
| 1e-5)); |
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.
@jcs15c and I spoke about this -- this is triggered called when evaluating the normal for a trimmed patch, and was failing for some files due to trimming curves that are slightly outside the parametric bounding box. Since the user has no control over this, we should probably preprocess/fix this before processing the NURBS patches,
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.
To further keep record of this conversation, at this point it's unrealistic to clip or clamp the trimming curves to the correct parameter space for a given patch. Instead, the methods in NURBSPatch (which already clamp the inputs to the parameter space) should probably give a one-time warning if out-of-bounds input is recorded, then move on. Since #1765 is mostly concerned with the STEPReader, I'll make those changes here instead.
4d0dd2d to
59746cf
Compare
| "NURBSPatchGWNCache::getKnots_v() must match NURBSPatch signature"); | ||
| static_assert(std::is_same_v<decltype(std::declval<const Cache&>().getTrimmingCurves()), | ||
| decltype(std::declval<const Patch&>().getTrimmingCurves())>, | ||
| "NURBSPatchGWNCache::getTrimmingCurves() must match NURBSPatch signature"); |
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.
These are great tests, thank you! It's worth keeping in mind that a future implementation of NURBSPatchGWNCache might have trimming curves of type NURBSCurveGWNCache instead of NURBSCurve, since that seems easier than templating the type of NURBSPatch on trimming curve type.
jcs15c
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.
Thanks for the optimizations of winding_number(NURBSPatch)! It all looks great.
* Allows user to omit bounding box and will use input bounding box instead. * Checks for consistency in query grid bounds and resolution (min/max/res) * Adds a few timers and annotations
Scales query bounding box when user does not supply one.
…or u- and v- The function already clamps the parametric coordinates to the knot-span, so there's no real loss here. In this case, the function was called when computing the patch normal of the trimmed patch. We should probably preprocess the trimming curves to be withing the parametric bounding box since the user of the 3D winding numbers has no control over the trimming curves.
We can now choose not to provide a query_mesh bounding box, and it will instead use the input mesh's bounding box (slightly inflated). We also print the query times and rates.
I tracked down the changes in containment determination exclusively to samples on/near the boundary. atan2 is more stable, robust and performant than acos for the linear_winding_number calculations.
| * \return a*b+c computed using an fma operation | ||
| */ | ||
| template <typename T> | ||
| inline AXOM_HOST_DEVICE T fma(const T& a, const T& b, const T& c) |
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.
I can think of a bunch of places to use this!
BradWhitlock
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.
Nice optimizations!
8716c0a to
c63ef54
Compare
Summary
quest::STEPReader#1765 (all the changes in this PR toSTEPReader.cpp), so that should be merged firstNURBSPatch.hppto accommodate the fact that many in-the-wild CAD examples have trimming curves which extend beyond the range of the patch's parameter space. In the future, these could be handled by the methods described in Add slic macros that only log the first time they're called #1685, which would alert the user without cluttering output.linear_winding_numberis now based onatan2instead ofacos. This was the bottleneck in 2D WN computations and shaves about 25% of runtimesaxom::utilities::fmafunction (with the expectation that this will improve robustness)