Repository navigation
Conversation
initParameters validates five meshing parameters with tests of the form "value < bound". Every comparison with NaN is false, so a NaN parameter takes none of the five branches: the two tests that throw do not throw, the three that substitute a value do not substitute, and the NaN reaches the mesher. Write the five tests as !(value >= bound), which sends NaN to the refusing or substituting branch and leaves every ordered value on the branch it took before. Add GTests for a NaN deflection and angle, for NaN optional parameters being replaced, and for out-of-range values staying refused.
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
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.
Pre-Submission Checks
.github/CONTRIBUTING.md.Group - Summaryformat.Problem / Motivation
BRepMesh_IncrementalMesh::initParametersis the one place the kernel validates the meshing parameters, and it spells all five tests asvalue < bound:Every comparison with NaN is false. A NaN parameter therefore takes none of the five branches: the two tests that refuse do not refuse, the three that substitute a usable value do not substitute, and the NaN reaches the mesher. The two throws are literal
throwstatements, so they are live in a release build; an ordinary too-small value is refused promptly, and NaN is the one input that walks through.What it does depends on the parameter, measured on
BRepPrimAPI_MakeCylinder(10, 5)against 8.0.1, one process per case:Deflection: the tessellation did not return in 600 s. On a box, whose faces are all planar, it returned a mesh at no stated deflection (24 nodes); on a free circular edge it produced 22 216Poly_Polygon3Dnodes where a valid request produces 33.Angle, at linear deflection 10 so that the angle decides:IsDone()true,AngleandAngleInteriorNaN, 18 nodes against 254 for angle 0.05 to 0.2. The coarsest mesh the linear rule accepts, reported as done.Proposed Solution
!(value >= bound). An unordered comparison makes>=false, so NaN goes to the refusing or substituting branch. For every ordered value the two spellings are the same test, so no bound moves and no valid input changes behaviour. This is a NaN fix only.DeflectionandAngleare validated before the expressions that consume them ((std::min)(Deflection, DeflectionInterior)in theMinSizesubstitution,2.0 * Anglein theAngleInteriorone), so the substitutions cannot manufacture a NaN of their own from a NaN they were handed.DeflectionInterior,MinSize,AngleInterior) are changed for the invariant of the function rather than for a measured defect: a NaNAngleInteriorreaching it with a validAngledid not change the node or triangle count on either fixture I tried. They are included because a function whose contract is that nothing out of bounds reaches the mesher should not leave three of its five tests open to the one value that defeats all of them.Other users of the same pattern (
Prs3d::GetDeflection, theincmeshDraw command) are not touched: every value they pass on arrives here.Validation
Local build of current
IR(Release,BUILD_RELEASE_DISABLE_EXCEPTIONSon as CI builds it, macOS arm64, clang),OpenCascadeGTest. New tests inBRepMesh_IncrementalMesh_Test.cxx:IRwithout the source changeBRepMesh_IncrementalMeshTest.NaNDeflection_IsRefusedBRepMesh_IncrementalMeshTest.NaNAngle_IsRefusedBRepMesh_IncrementalMeshTest.NaNOptionalParameters_AreReplacedBRepMesh_IncrementalMeshTest.OutOfRangeParameters_StayRefused(control: 0 and negative values still throw, a valid request still meshes)The deflection test uses a box on purpose: on a cylinder the unpatched 8.0.1 kernel did not return for a NaN deflection.
With the change, the mesh-related suites (
*Mesh*,*Triangulation*,*Discret*,RWMesh*,*Tessell*): 141 tests, 141 passed.Checks performed:
clang-format 18.1.8 with the repository
.clang-format, the license check and the include cleanup of the formatting job report no change on the touched files, and none contains a non-ASCII character.Review Notes
The change is confined to the private inline
initParametersin the header, called fromBRepMesh_IncrementalMesh::Performonly.