Skip to content

Modeling Algorithms - BRepOffset_MakeOffset visits arc-join offset faces in the order they were bound - #1600

Open
gsdali wants to merge 1 commit into
Open-Cascade-SAS:IRfrom
SecondMouseAU:fix/3003-offset-arc-join-root-order
Open

gsdali wants to merge 1 commit into
Open-Cascade-SAS:IRfrom
SecondMouseAU:fix/3003-offset-arc-join-root-order

Conversation

@gsdali

@gsdali gsdali commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

BRepOffset_MakeOffset::BuildOffsetByArc keeps the offset of every face, convex edge (tube) and vertex (sphere) in

NCollection_DataMap<TopoDS_Shape, BRepOffset_Offset, TopTools_ShapeMapHasher> MapSF;

and iterates it to register each offset face as a root of myInitOffsetFace and myImageOffset. MakeShells passes myImageOffset.Roots() to BRepTools_Quilt, which keeps faces in the order it is given them, so the order of the roots is the order of the faces of the result.

std::hash<TopoDS_Shape> is the address of the TShape (TopoDS_Shape.hxx), so that order is decided by where the allocator put the shapes. It differs between processes and, as the heap evolves, between two builds in one process. Every arc-join offset (BRepOffsetAPI_MakeOffsetShape and BRepOffsetAPI_MakeThickSolid with GeomAbs_Arc) returns its faces in a different order from run to run, and the volume, which BRepGProp sums over the faces in the order the shape holds them, moves in its last digits. Nothing is parallel (one thread, BOPAlgo_Options::GetParallelMode() is false) and nothing is read uninitialised.

  • Record the order the entries are bound in (the faces as MakeOffsetFaces binds them, that is BRepLib::SortFaces over myFaceComp and then the faces BRepOffset_Analyse added, then the tubes, then the spheres) in an NCollection_IndexedMap, and walk that, looking each entry up in MapSF, from which ToContext may have removed it. Which offset faces are built, and how, does not change.
  • Add BRepOffset_MakeOffsetTest.ArcJoin_FaceOrderDoesNotDependOnAddresses. It offsets a freshly made box 32 times with a different amount of heap held before each and compares the order of the faces and the volume with the first build. A fresh box matters: offsetting one box repeatedly does not show the defect, because MapSF is also keyed on the input's own sub-shapes, whose addresses then do not change.

MapSF is not changed to NCollection_OrderedDataMap because its type is a parameter of BRepOffset_Inter3d::ConnexIntByInt and ContextIntByInt, so that would change signatures in another header and translation unit.

Measurements

Fresh processes, macOS arm64, 60 per row, on 8.0.1 with its fixes. "Volumes" and "dumps" count the distinct volumes and the distinct hashes of the bit-exact BinTools dump of the result:

request volumes dumps
box 10, offset +1, GeomAbs_Arc 7 60
box 20, thick solid 2.0, top face open 3 60
box 20, thick solid 2.0, nothing open 6 60
box 10, offset -1, GeomAbs_Arc 1 55
cylinder, offset +1, GeomAbs_Arc 1 24
box 10, offset +1, GeomAbs_Intersection 1 1

Rows at one volume and several dumps return the same solid with its faces in another order, and sum to the same volume because their faces are symmetric enough to add exactly in any order. The last row does not go through BuildOffsetByArc and is already reproducible. With this change every row is 1 and 1. In one process, 32 builds of the first row with a different amount of heap held before each give between 11 and 32 different face orders without the change (20 processes) and 1 with it. Over 72 requests (nine shapes, eight requests each) in 20 processes, 29 returned more than one distinct result before and none after; in 26 of them only the order of the faces differed.

Test

Built from IR at 0089343 (Release, Foundation, Modeling Data and Modeling Algorithms modules, with GTests):

  • BRepOffset_MakeOffsetTest.ArcJoin_FaceOrderDoesNotDependOnAddresses fails without the source change (15 of 15 runs, 649 failed expectations in the first, the first from run 1) and passes with it (15 of 15 runs).
  • The other 24 tests in BRepOffset_MakeOffset_Test.cxx pass either way. The 258 tests matching Offset, ThickSolid, Fillet, MakePipeShell and Sewing pass.
  • The full GTest suite with the change: 8360 tests, 8353 passed, 7 skipped by the tests themselves, none failed.
  • clang-format with the repository .clang-format (22.1.8 locally) reports nothing on the added lines; its one warning is on a line this change does not touch. No changed file contains a non-ASCII character.

One input the change makes deterministic in the wrong direction

For an input whose success depends on the order the roots arrive in, the result is now the same every time, and it may be the failing one. The one I found is the fuse of two boxes that keeps its coplanar faces split: BRepOffsetAPI_MakeOffsetShape with GeomAbs_Arc and +1 returns a solid in 11 of 20 processes before the change and, in the others, reports IsDone() with a null shape. The order chosen here is one of the failing ones for that input, so it now fails 20 of 20. Roughly 42% of random root orders succeed on it, and the reverse of binding order does, so that dependence belongs to the intersection stage that follows and is not addressed here. The other 69 of the 72 requests return the same outcome with and without the change. If you would rather not take a deterministic failure for that input in exchange, I can look at the intersection stage first.

…ces in the order they were bound

BRepOffset_MakeOffset::BuildOffsetByArc builds a BRepOffset_Offset for every face, every convex
edge (a tube) and every vertex (a sphere) and keeps them in

  NCollection_DataMap<TopoDS_Shape, BRepOffset_Offset, TopTools_ShapeMapHasher> MapSF;

It then iterates MapSF to register each offset face as a root of myInitOffsetFace and
myImageOffset. MakeShells passes myImageOffset.Roots() to BRepTools_Quilt in that order, and the
quilt keeps faces in the order it is given them, so the order of the roots is the order of the
faces of the result.

std::hash<TopoDS_Shape> is the address of the TShape (TopoDS_Shape.hxx), so the iteration order
is decided by where the allocator put the shapes. It differs between processes and, as the heap
evolves, between two builds in one process. Every arc-join offset (BRepOffsetAPI_MakeOffsetShape
and BRepOffsetAPI_MakeThickSolid with GeomAbs_Arc) returns its faces in a different order from
run to run, and the volume, which BRepGProp sums over the faces in the order the shape holds
them, moves in its last digits. Nothing is parallel (one thread, and
BOPAlgo_Options::GetParallelMode() is false) and nothing is read uninitialised.

- Record the order the entries are bound in (the faces as MakeOffsetFaces binds them, that is
  BRepLib::SortFaces over myFaceComp and then the faces BRepOffset_Analyse added, then the tubes,
  then the spheres) in an NCollection_IndexedMap, and walk that, looking each entry up in MapSF,
  from which ToContext may have removed it. Which offset faces are built, and how, does not change.
- Add BRepOffset_MakeOffsetTest.ArcJoin_FaceOrderDoesNotDependOnAddresses. It offsets a freshly
  made box 32 times with a different amount of heap held before each, and compares the order of
  the faces and the volume with the first build. A fresh box matters: offsetting one box
  repeatedly does not show the defect, because MapSF is also keyed on the input's own sub-shapes,
  whose addresses then do not change.

MapSF is not changed to NCollection_OrderedDataMap because its type is a parameter of
BRepOffset_Inter3d::ConnexIntByInt and ContextIntByInt, so that would change signatures in
another header and translation unit.
@trevin-lee

Copy link
Copy Markdown

I'm working on a project that requires bit-for-bit reproducible results across platforms from OCCT, so I ran an independent check of this change with a randomized stress test. The results are below.

Setup: 605 cases across 11 operation kinds: booleans of curved and rotated primitives, fillet, chamfer, MakeThickSolidByJoin, BRepOffsetAPI_MakeOffsetShape::PerformByJoin (default GeomAbs_Arc), revolve, B-spline prism, loft, pipe, and BRepMesh_IncrementalMesh. Inputs come from a seeded integer generator. Each result is compared bit for bit: volume, area, centre of mass, bounding box, vertex coordinates, and a hash of the BinTools dump. Failures are compared by exception type and message. OCCT 8.0.1 was built from source with USE_TBB=OFF and -ffp-contract=off, and with the platform libm replaced by a portable implementation so the two architectures could be compared. I applied this PR's source change to 8.0.1. The GTest file does not apply to 8.0.1, so I left it out.

Repeat runs, macOS arm64 (10 processes) macOS arm64 vs macOS x86_64 (Rosetta 2)
8.0.1 0 of 9 runs matched the first 573 of 605 cases identical
8.0.1 + this PR 9 of 9 runs matched the first 605 of 605 cases identical

Without the change, every difference was in the offset cases: only 22 to 23 of the 55 matched between runs. The other 10 operation kinds were identical in every run. With the change, the whole corpus is bit-identical across runs and across both architectures.

The same 47 cases fail with and without the change, and no case switched between success and failure. The input you described in "deterministic in the wrong direction" did not come up in this corpus.

I can post the harness source if it would be useful for review.

The stress harness and this investigation were done with AI assistance (Claude). The numbers above are from runs on my machines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

2 participants