Notes from the TopoNaming Project team

[bgbsww]

This issue is to capture knowledge and notes as the topological naming problem fixes are transferred from the Real Thunder branch to the main one.

Markdown format is preferred for convenience in a future collation of this information, but not required.

We are preserving author credit by waiting to merge comments. Each author is welcome to keep editing their comment, or use multiple comments as they see fit.

[bgbsww]

Overview of the work

Most of the work is contained within the Part module. Code from the RT branch is not just transferred, but modernized by updating the method names, cleaning up parameter definitions, removing legacy code, and using available c++ techniques. The primary method name change is from makE to makeElement for clarity - makEShell becomes makeElementShell for example. When possible, boolean flags are converted to enums to improve clarity when calling: doSomething(true,false,false,true) becomes doSomething(UseOptimization,NoThrowExceptions,NoRefine,BuildFaces)

Other concerns being addressed are any linter errors, in particular refactoring methods with high cognitive complexity. Also removal of single character variables, which can get I nt ense (bad dad joke to see if anyone is reading). We also want to absolutely minimize the use of preprocessor features when possible.

PR Technique

In order to maintain credit where deserved we're using a pattern for any transferred code: the first commit contains the code taken from the RT branch, with any simple changes like renaming the method applied. This PR is committed with --author="Zheng, Lei [email protected]" Subsequent commits contain tests, major edits, or anything else and do not get an author tag so the developer doing the work gets credited. Note that --amend, rebase -i and other standard git techniques can be used to get the right commits smushed together and credited.

An ideal PR contains a first commit with the RT code, and a second commit that resolves any remaining compile errors, completes all renames, reformats the code as needed, updates any code using legacy techniques, and add tests. That results in a minimal final commit log.

Individual commits should be prefixed with "Toponaming/Part: " as well as the overall PR so that the single line git logs are easy to read and understand.

Performance

We know by definition that when enabled this code will be slower than the existing code. As Chennes pointed out "we're doing more work, it's going to be slower". With that said, we believe that the effective use of cacheing and other performance improvements along the way should result in an acceptable result. The Realthunder fork is not unreasonable in its performance.

However, it would be useful to quantify how much slower it is, what types of files / operations trigger slowness, and where to look for optimizations. There is a proposal from Bgbsww to do this by providing a script that can download selected documents from the FreeCAD-library and run FreeCAD recomputes on them with measured performance. Perhaps even with profiling. There is also a thread with other potential example files.

Shape history/ancestry/info

The toponaming algorithm depends on keeping track of the history of how a shape was created, and using that history to form the internal name of the shape. The information required to create this history is available from OCCT, although it does not track it.

OCCT thinks that a shape is Generated if you create it from another shape and it has a higher dimensionality. E.G. a MakeFace from a Wire, for example, or Vertex -> Edge, Edge -> Face, Wire -> Shell, Face -> Solid, Shell(s) -> Compound.

OCCT sees a shape as Modified if you transform or translate it, so moves, rotates, etc. The dimensionality remains the same, but the shape is located somewhere else.

Initial Tests

The googletest "gtest" framework located at /tests in the source tree is used extensively by this project, as the plan is to implement unit tests that cover the behavior of the existing code, then transfer the new code and prove via the testing that nothing has changed.

The key difference in a makeElementX method like makeElementBoolean vs the makeBoolean form of the legacy code is that the TopoShapes passed in, and any created or returned are expected to have an ElementMap, a name associated with them that shows the history of how they were created and a Tag other than zero. These names get longer as more operations are performed, as they show the history of how a TopoShape came to be.

Attempting an operation ( compound, boolean, etc ) against TopoShape(s) without ElementMaps used to result in a segfault; that is now throws an exception. The fix is to use correct TopoShape(s) that have ElementMaps in all your tests. This has bitten the team at least three times. In particular, the third, empty form of the TopoShape constructor, TopoShape testshape(); is to be avoided; TopoShape testshape(1L) with a tag will keep you out of trouble. This may be mitigated by a subsequent change to resetElementMap, but ultimately you always need subshapes that are element aware.

There is a PartTestHelpers.cpp file established for generally useful methods that create geometry, interpret ElementMap vectors, and the like. This saves on boilerplate in various tests.

Gotchas

Some of the makeElementX methods return *this; a reference as a convenience to the user. However, that means that this typo: TopoShape y = z.makeElementX(...) does an implicit copy, and that copy doesn't have the mapped elements in it, so further code will fail. TopoShape& y = z.makeElementX(...) is correct, but the compiler doesn't enforce it.

The other makeElementX methods return a new TopoShape, and generally this is created with a Tag of 0, which means you won't have mapped elements. These are often defined right in TopoShape.h and call the reference version of the method. The rule of thumb here is tests should only use TopoShape& returns from makeElementX methods unless you are explicitly testing the empty element map version.

Sometimes the github builds ( particularly the MacOSX one ) fail with timeouts on perfectly good code. Check the fail logs, and you can request a rebuild from any maintainer. For self service, kickstart a new build with a trivial commit.

Most likely irrelevant by the time this is read: if you're working with a version of OCCT ahead of what the official builds use ( 7.8.0 ) as of this writing, be sure to test any failing builds against an official build version.

Particularly when splitting off a new source file, but in general, it is critical to check the PreCompiled.h file against any added #includes and ensure that they will get included. This does NOT get checked by anything, including the github builds, so it is possible to silently break something that will be discovered in a later local MSVC build by another unsuspecting developer. Note that PreCompiled.h is largely populated via including OpenCascadeAll.h.

Hints

gtests can be run with GTEST_CATCH_EXCEPTIONS=0 defined to make some issues more visible.

One notable issue is that using the --gtest_filter can cause shared libraries not to load in some cases. Both 'make test' and running 'ctest' show this behavior, as they run each test individually using this filter ( see ctest -VV but directly running a test program like Part_tests_run does not.

We're using #ifndef FC_USE_TNP_FIX around replacement code at the top level ( FeatureXX.cpp files ) during phase 2 to prevent having any of the new code called until we switch it on in phase 3. And then subsequently remove all the old code in the else side of these.

There is a TEST_WRITING_TUTORIAL.cpp file in draft that when submitted gives help on how the API works and how to write tests for it.

The earliest version of OCCT that still needs supporting is 7.6.3. Code ( often in an #ifdef ) from before that version can be discarded.

Changed Stuff

• The method names and some parameters as noted above
• elementMap is its own data structure, no longer part of ComplexGeoData, which changed some calls.

Future issues to address

• Order and organization of methods in / across source files. The final topoShape.h should be organized, documented, and readable though huge, and the cxx source files will want organization.
• Some variable names like tol as opposed to tolerance may want global replacement in the source code base.
• Some concepts like the boolean flag silent that extend beyond just the Part module may want to be addressed in targeted sweeping PRs.
• Debugging is a PITA because of the strings. This could help

Temporary things during the transfer process

The TNP branch code has a series of #ifdefs that contain the original code next to the changed code for TNP. Until we're ready to remove all the non-TNP code, transferred code will be protected by #ifdef FC_USE_TNP_FIX, which is intentionally different than the name in the TNP branch to allow detecting any copy/paste errors.

[CalligaroV]

Great work @bgbsww!

I'll contribute with my discoveries/modifications. By now I think we can add:

Changed Stuff

• The file TopoShapeEx.cpp from RT branch is now renamed TopoShapeExpansion and has the following modifications:
  • The function makESHAPE(const TopoDS_Shape &shape, ...) from RT branch is now renamed
    makeShapeWithElementMap(const TopoDS_Shape &shape, ...). makESHAPE is different from the other makEShape methods at it doesn't take BRep*API_* args as inputs.
  • Other makEShape(BRep*API_* &mkShape, ...) methods are / will be renamed makeElementShape(BRep*API_* &mkShape, ...) according to the convention described in the Overview of the work paragraph
  • Precompiler macros HANDLE_NULL_* and WARN_NULL_INPUT from TopoShapeEx.cpp have been removed and replaced with FC_THROWM and FC_WARN
  • Precompiler macro INIT_SHAPE_CACHE has been removed and replaced to calls to the method TopoShape::initCache. This method has been modified to not take into account the args const char *file and int line
  • method searchSubShape in RT branch has been renamed findSubShapesWithSharedVertex

Obviously there'll be other points that will be added as long as the project continues