Skip to content

Conversation

adamkewley
Copy link
Collaborator

General feature PR for implementing function-based paths.

This is WIP and is a manual merge of code from:

https://github.com/joris997/opensim-core/tree/interpolation

@adamkewley
Copy link
Collaborator Author

310abe1 separates GeometryPath from PointBasedPath as much as possible while ensuring the test suite, and any API calls used by opensim-gui, are still valid for any PointBasedPath instance.

Unlike the source implementation, this one rips out as many data members as possible and replaces almost the entire API of GeometryPath with abstract methods that downstream classes have to implement. The PointBasedPath just implements them exactly as pre-patch GeometryPath worked. The (to be merged) FunctionBasedPath will need to implement all methods (incl. the ones that handle path points) in such a way as to not destabilize implementations that relied on GeometryPath.

E.g. we can't just throw "Not implemented" or "not supported" on the API, because that would break the official OpenSim GUI, which uses parts of the API for the path-editing features (e.g. the ability to reorder wrapping elements). An alternate implementation has to at least return fake elements, or behave as believable as possible, to not destabilize the GUI (e.g. opening a FunctionBasedPath model in the editor and then editing a path shouldn't cause the GUI to explode).

@adamkewley
Copy link
Collaborator Author

The latest commit hardens the base class against deprecations. This was done so that downstream implementations that are not point-based (e.g. FunctionBasedPath) do not, themselves, have to redundantly handle deprecations. This will make it easier for users to create their own, custom, GeometryPath implementations (e.g. ConstantLengthPath or something).

The deprecations have been handled by providing a crash-resistant implementations that computationally work, but do not functionally do anything (e.g. they return valid pointers to valid memory locations, but those memory locations don't affect a simulation). This should mean that any legacy code that assumes the GeometryPath it's working with is sequence of points--but is, in reality, a FunctionBasedPath--shouldn't segfault. Instead, a global warning message will be printed to the log and the implementation will mostly limp along. This gives downstream developers breathing space to either remove point-dependent code or switch to ensuring it's dealing with PointBasedPaths.

The PointBasedPath has a fully legacy-compliant overload of the deprecated API. This means that all legacy code that assumes the GeometryPath it's working with is a sequence of points--and it actually is--will work as intended at runtime. However, downstream developers will still see a compile-time deprecation message if they are using PointBasedPaths via the GeometryPath API (which is now an invalid use). This should prompt them to eventually perform any necessary runtime downcast checks (e.g. dynamic_cast<PointBasedPath*>(&p) != nullptr), but their codebase will still mostly work with no changes.

Methods that are deprecated have been fully listed as so in the header (using the appropriate macro). This causes compilers to warn about the deprecation. Here is a list of deprecation warnings that are printed out when compiling opensim-core on Linux:

https://gist.github.com/adamkewley/fb1f1fa233d6a1c9ef03980c0d38bcf6

All existing OpenSim test suites etc. still pass, despite these deprecation warnings, because the runtime implementation that these tests use are PointBasedPaths. These are just compile-time warnings about API usage.

Those now-deprecated API uses should be fixed in-tree as part of the path chance. This is so that compiling OpenSim produces no deprecation warnings and so that it internally uses the new API correctly. However, the deprecated API, and stub methods, should then be left in-place for "a reasonable amount of time" (e.g. 3-8 years) so that downstream implementations have a chance to clean up.

@adamkewley
Copy link
Collaborator Author

34116ec just adds a basic not-yet-fleshed-out API for a FunctionBasedPath

The next step is to define a PathFunction class that has the necessary minimum API required for a FunctionBasedPath to... function

@adamkewley
Copy link
Collaborator Author

adamkewley commented Dec 1, 2021

5f84d1c tries to define what a PathFunction probably needs, api-wise, in order to work as a runtime-defined datastructure.

The next step is to wire up the FunctionBasedPath to use the relevant cache variables and PathFunction at runtime to determine its length, lengthening speed, etc.

Once wired up, I can write a mock implementation of a PathFunction that enables flags etc. when its members are called and confirm that the FunctionBasedPath is effectively just plumbing into the PathFunction.

Once the plumbing is tested, we'd then need to--separately--implement a (e.g.) JorisSplineFitPathFunction (or similar) in a test suite only (not declared in OpenSim - yet) and then we'd need to ensure that it meets minimum accuracy etc. requirements to satisfy the PathFunction interface.

Once that's done, the final step is to rip it out of the test suite and turn it into a standard API-visible OpenSim object with relevant properties, fields, etc. that can handle storage-time serialization (e.g. it has relevant properties for saving which Coordinates it was parameterized against, etc.)

@adamkewley
Copy link
Collaborator Author

d669e8d fleshes out the FunctionBasedPath layer of the problem and performs some basic unit testing that ensures the FunctionBasedPath is ultimately (with caching) using the PathFunction.

The next step is to copy Joris's implementation code into some standalone compilation unit that we can play with independently of the rest of OpenSim (e.g. a test compilation unit). In that, we'll implement + test the interpolation code independently of everything else.

I'll likely just hack it in initially so that we have something to play with.

@adamkewley
Copy link
Collaborator Author

3c25207 contains a cut-and-paste (give or take) port of Joris's FBP code into the test harness so that we can experiment with the code without having to heavily recompile OpenSim or anything.

@adamkewley
Copy link
Collaborator Author

To build the branch on with opensim-creator, checkout opensim-creator and:

#!/usr/bin/env bash

OSC_OPENSIM_REPO=https://github.com/ComputationalBiomechanicsLab/opensim-core OSC_OPENSIM_REPO_BRANCH=feature_function-based-paths ./scripts/build_windows.bat

Or, in cmd.exe:

set OSC_OPENSIM_REPO=https://github.com/ComputationalBiomechanicsLab/opensim-core && set OSC_OPENSIM_REPO_BRANCH=feature_function-based-paths && call ./scripts/build_windows.bat

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants