overlap_check: make the ray direction independent of the compiler - #996
Open
cyb3ralbert wants to merge 1 commit into
Open
overlap_check: make the ray direction independent of the compiler#996cyb3ralbert wants to merge 1 commit into
cyb3ralbert wants to merge 1 commit into
Conversation
The order in which function arguments are evaluated is unspecified, so CartVect dir(rand(), rand(), rand()) assigns the three values to components in a compiler-dependent order: GCC evaluates right to left and Clang left to right. The same geometry could therefore be checked along a different ray depending on which compiler built the tool, and both compilers are in the CI matrix. Evaluate the three calls in a defined order instead. On Clang the direction is unchanged; on GCC it now matches Clang.
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.
check_instance_for_overlapspicks the ray direction like this:CartVect dir(rand(), rand(), rand());The order in which function arguments are evaluated is unspecified in C++, so which
rand()result lands in which component is left to the compiler. GCC evaluates right to left and Clang left to right, so the same source ends up checking along a different ray depending on which compiler built it:(0.644860, 0.324763, 0.691871)(0.691871, 0.324763, 0.644860)(measured in the CI container, ubuntu-22.04 / glibc)
Both compilers are in the
linux_build_test.ymlmatrix, so two supported builds of the same tool can check the same model along different rays. This patch just evaluates the three calls in a defined order: on Clang the direction is unchanged, and on GCC it becomes the same as Clang's.I kept the change as small as I could. While looking at this I also tried replacing
rand()with a uniform sample over the sphere, but that turned out to be a regression, so I'm not proposing it here.diris used both as the bump offset and as the ray direction, and having all three components positive is what makes one of the two probes (+dir/-dir) land inside an axis-aligned convex corner. With a uniformly sampled direction whose components have mixed signs, neither probe enters the corner, and an overlap at a cube corner that the current code finds gets missed. So the positive-octant behaviour seems worth keeping, and I've left it alone — this also corrects the framing of #995, where I described the octant as a weakness. Sorry for the noise there.Tested on ubuntu-22.04 / gcc 11 / MOAB 5.5.1:
ctestpasses 11/11 includingoverlap_check_test, andoverlap_checkgives the same results as before on the shipped models and on a few small ones I built by hand.Happy to adjust the approach or drop this if you'd rather handle it differently.