Initial refactor - compiles but needs fixing etc.

[adamkewley]

This is a work-in-progress PR - it does not currently work.

Changes up to now:

• Added in-source explanation of what the "new" GeometryPath means here
• Reorganized GeometryPath methods such that related members are together (e.g.1, e.g.2)
• Added basic documentation to PointBasedPath here
• Copied standard license header onto FunctionBasedPath here
• Added basic documentation for FunctionBasedPath here
• Moved Interpolate into the cpp file for FunctionBasedPath and hidden FunctionBasedPath's implementation behind an opaque OpenSim::FunctionBasedPath::Impl struct that no other compilation units can see (header, impl)
• Made getLength and getLengtheningSpeed use the underlying CacheVariable infrastructure here
• Made printContent use an ostream rather than an ofstream, so that it can be used to also print to (e.g.) memory. Also refactored the implementation to use char delimibers, which the compiler can more easily optimize around than strings. See here
• Made id, an int that implicitly names+finds the associated data file, into data_path, which explicitly holds a path to the data file. This is so that data files can later be moved to other locations, shared between models, etc. Change here
• Removed the FunctionBasedPath constructors that take an id or a PointBasedPath to construct an instance of FunctionBasedPath. Understanding the intention (of passing an int) is unclear from the type signature alone. Those constructors are now factory functions called:
  • FunctionBasedPath::fromPointBasedPath i.e., create a FunctionBasedPath from an existing PointBasedPath (here)
  • FunctionBasedPath::fromDataFile i.e. create a FunctionBasedPath from an existing data file (here)
• Moved the "from data file" constructor into the factory function (explained above), but also refactored its implementation and added a comment-based specification of the serialized data file. This format needs additional development, or refactoring into one of OpenSim's sto abstractions. See change here
• Cleaned up fromPointBasedPath implementation here
• Added more explanation to the FunctionBasedPathModelTool here
• Changed FunctionBasedPathModelTool to take two args, so the user can specify an output name here
• Removed any uses of UNIX filesystem APIs: these might not compile on non-UNIX OSes (e.g. Windows)

Outstanding issues:

• FunctionBasedPath in the implementation doesn't seem to have a way of loading data files where id != 0. Probably wouldn't be able to load an arbitrary osim containing FunctionBasedPaths and expect it to work
• File loading seems to pass an empty vector of Coordinate*s into Interpolate, but Interpolate seems to rely on it being non-empty at runtime (there are asserts, but these would be disabled in release builds)
• The base GeometryPath class still contains a lot of point-centric methods
• Methods like setLength do not seem to make sense in either FunctionBasedPath or PointBasedPath - should probably be private. Same for setLengtheningSpeed
• The base class still holds data it shouldn't hold (e.g. cache variables that should probably be held by derived classes)

We'll meet tomorrow about some of these

[adamkewley]

The latest refactor:

• Tries to move file loading into extendFinalizeFromProperties here
• Makes the tool map a PBP to a PathActuator, rather than re-casting here
• Moves all the Interpolate implementation in-line in the Interpolate class, because it's now a private class in the CPP file
• Performs basic cleanups (e.g.)
• Refactors some methods to try and make their intent clearer (e.g.)

Outstanding work (laundry list - will be discussed separately):

• Find a suitable place to store the coordinates that affect the FunctionBasedPath, so that loading the osim/data file correctly re-links to the correct Coordinates in the model at runtime. This probably needs to be added to extendFinalizeConnections
• Heavily clean up the Interpolate implementation. At the moment, it's unclear what each datastructure is doing at runtime because the algs are very very "math-ey" and still look fairly experimental. The Interpolate data invariants are unclear from a basic reading of the class structure, and currently require a fair amount of concentrated reading of the source code to understand. Unlikely to be accepted by external maintenance devs (imho). The impl should ideally be a clearly-explained implementation of a standard alg (refactoring this should be easier to do now, because Interpolate is only in one CPP file)
• Establish how the file loading should play out. What errors should be thrown when data files are missing, etc.
• Establish where to save the interpolation data files
• Establish now relative paths are handled for data_path. Relative to the model? Lookup locations? etc.
• Hook up the tool to the general tooling infrastructure, or add a relevant build target for building the PBP2FPB conversion command line tool
• Test FBPs actually work with basic models (e.g. a single elbow, bouncing block, whatever)
• Test that loading an osim file containing an FBP correctly works via the C++ api
• Verify that the loaded file finalizes and can visualize a model that contains FPBs
• Get this changeset building in all OSes
• Get this changeset passing all existing tests in all OSes (incl. the bizzare segfault)
• Take basic performance metrics of the implementation after it is robustly implemented, integrated, tested, etc.
• Performance grind the implementation down so that it's faster than a PBP
• Discuss GeometryPath's new design (which contains point-based APIs still etc.) - this is a V2 discussion

[joris997]

I'll merge and fix/test things during the weekend. Then if anything breaks we can go back to the pre-merge