Improves testing for quest::STEPReader for surface orientation#1818
Merged
Improves testing for quest::STEPReader for surface orientation#1818
quest::STEPReader for surface orientation#1818Conversation
jcs15c
changed the base branch from
develop
to
feature/spainhour/fast_gwn_triangles
jcs15c
force-pushed
the
feature/spainhour/rigorous_step_orientation
branch
from
d3046a8 to
947c562
Compare
jcs15c
requested review from
Arlie-Capps,
BradWhitlock,
bmhan12,
kennyweiss and
rhornung67
Base automatically changed from
feature/spainhour/fast_gwn_triangles
to
develop
March 23, 2026 19:44
kennyweiss
approved these changes
Mar 30, 2026
Comment on lines
+889
to
+890
| BRepTools_WireExplorer edgeExp(wire, TopoDS::Face(faceExp.Current())); | ||
| for(; edgeExp.More(); edgeExp.Next(), ++edgeIndex) |
Member
There was a problem hiding this comment.
Suggestion variable name (minor):
Since this is a WireExplorer, should the variable be wireExp instead of edgeExp?
Contributor
Author
There was a problem hiding this comment.
Maybe, but wireExp is already defined above as an object of type TopExp_Explorer since that iterates over the topology, producing wires. Then the object of type BRepTools_WireExplorer iterates over the wires, producing edges. It's a little complicated, but I think there is logic to it. Wire explorers explore wires, and edge explorers explore edges.
jcs15c
force-pushed
the
feature/spainhour/rigorous_step_orientation
branch
from
7d23c50 to
58f664d
Compare
rhornung67
approved these changes
Apr 1, 2026
Member
rhornung67
left a comment
rhornung67
left a comment
There was a problem hiding this comment.
One minor request to use Axom macro for unused method args.
jcs15c
force-pushed
the
feature/spainhour/rigorous_step_orientation
branch
from
13097be to
39a5d3e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
quest::STEPReaderto handle additional edge cases stemming from different canonical representations of OpenCascade STEP files andprimal's trimmed NURBS primitives. In general, OCC stores surface orientation through both an internal flag and the orientation of loops of trimming curves, whileprimalencodes orientation information only through patch control points and requires CCW trimming curve loops.An issue was identified and partially fixed in the closed PR #1765, but since then additional edge cases have popped up. To better test for these issues as the reader is exposed to more in-the-wild meshes, a dedicated test has been added to
quest. This test is supported by additional STEP files added to Axom's data repo.Below is a table which shows which tested files fail for each configuration of the STEP reader. Files marked with * are new to
axom_data.Additionally, this PR addresses an incorrect overload in
winding_number.hppfor 2D NURBS curves.