Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

initial commit of Smith2018ContactMesh and Smith2018ArticularContactForce #2658

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

clnsmith
Copy link
Contributor

@clnsmith clnsmith commented Dec 29, 2019

Brief summary of changes

This PR introduces a new ContactGeometry and Force component to represent articular contact using triangulated surfaces meshes based on the method introduced in this paper:

Smith, C. R., Won Choi, K., Negrut, D., & Thelen, D. G. (2018). Efficient computation of cartilage contact pressures within dynamic simulations of movement. Computer Methods in Biomechanics and Biomedical Engineering: Imaging & Visualization, 6(5), 491-498.

https://www.tandfonline.com/doi/abs/10.1080/21681163.2016.1172346

Testing I've completed

Testing is a work in progress. Currently I added a test to testForces that builds a model using these components and performs a simple forward simulation. I will add more testing soon to make sure results are correct.

I tried to add to testComponents, but without a mesh file (.stl etc) the Smith2018ContactMesh fails.

Looking for feedback on...

Overall layout of components etc. This is a pretty good sized chunk of code, so it may take a while to review.

CHANGELOG.md (choose one)

  • updated to introduce new components

The Doxygen for this PR can be viewed at http://myosin.sourceforge.net/?C=N;O=D; click the folder whose name is this PR's number.


This change is Reviewable

@chrisdembia
Copy link
Member

AppVeyor (Windows testing) generates warnings like the following:

C:\projects\opensim-core\OpenSim\Simulation\Model\Smith2018ArticularContactForce.cpp(349): warning C4244: 'argument': conversion from 'SimTK::Real' to 'int', possible loss of data

Could you fix these issues? Once the tests pass, we should create a separate PR to merge these components into the camsknee branch without review.

In parallel, we can review these changes for being merged into the master branch.

@chrisdembia
Copy link
Member

The Travis-CI build fails due to the use of tabs instead of spaces.

changed SimTK::Vector that contains int to std::vector<int> to address compiler warnings
rewrote initialization and get function of neighboring triangles to address this change ^
@clnsmith
Copy link
Contributor Author

I just pushed a commit that should address both of these issues.

Apparently visual studio edit->advanced->untabify selected lines doesn't actually remove all tabs.

@chrisdembia
Copy link
Member

I quickly skimmed this PR. I think it will take a while to review.

@clnsmith
Copy link
Contributor Author

I quickly skimmed this PR. I think it will take a while to review.

Yeah agreed. Feel free to just make some high level comments to begin with. I will keep cleaning up the code as I address these.

The only thing that I think is important to try to finalize before the workshop would be the user interface (ie properties and component types) so the representation in the .osim model file is stable.

Copy link
Member

@chrisdembia chrisdembia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some initial disorganized comments.

I have not finished thinking about the user interface.

I made a PR into your fork: clnsmith#3

Reviewed 4 of 12 files at r1, 1 of 1 files at r3.
Reviewable status: 5 of 12 files reviewed, 36 unresolved discussions (waiting on @clnsmith)


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 40 at r3 (raw file):

 interpenetrate and the local overlap depth (proximity) is calculated for 
 each triangle. The contact pressure on each triangle face is then calculated 
 based on the overlap depth (see below).

Do you think it would make sense to bring over some figures from the paper (if the copyright permits it)?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 40 at r3 (raw file):

 interpenetrate and the local overlap depth (proximity) is calculated for 
 each triangle. The contact pressure on each triangle face is then calculated 
 based on the overlap depth (see below).

I love how detailed this documentation is.

Most users will want to know "should I use this contact model or a different contact model?" and "how do I use this contact model?". I think the documentation could be organized a bit more to answer these two questions before delving into the implementation.

One question I have is what makes this contact model specific to articulations versus foot-ground contact? Might be good to define "articular", too. Can this model be used only for articulations?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 49 at r3 (raw file):

 method to efficiently detect contact between triangular meshes using object 
 Oriented Bounding Boxes (OBB, a common approach in computer graphics) and 
 several additional speed ups that take advantage of the constrained nature of 

What are the constraints? Does this mean the surfaces must intersect?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 68 at r3 (raw file):

 generally small. Thus, after reposing the meshes, (i.e. realizePosition) each 
 triangle in the casting mesh is tested against the contacting target triangle 
 from the previous pose. Additional speed up is gained by casting the normal 

Is the case of "no previous pose" handled gracefully?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 128 at r3 (raw file):

 to accomodate variable properties. The use_lumped_contact_model property 
 controls whether the constant property or variable property formulation is 
 used.

This paragraph is a bit hard for me to follow.


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 137 at r3 (raw file):

 \f[
   P_\mathrm{casting} = f(E,\nu,h,d_\mathrm{casting})\:
   (linear or non-linear formulation above)

This is not formatted well when generating the HTML documentation:

image.png


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 167 at r3 (raw file):

 system of equations is solved using a numerical solver. 

 The min_proximity and max_proximity properties limit the search region for a 

These are distances in meters along the ray?

How does "proximity" differ from "penetration depth" or "overlap depth"?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 172 at r3 (raw file):

 from being found. This is particularly important for situations with highly
 curved meshes where a ray cast from the casting_mesh may intersect the 
 target_mesh multiple times, or at infeasible contact locations. An example

Can you explain? This description hints that, if the proximity properties are not set properly, one could get invalid results. That does not seem desirable.


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 176 at r3 (raw file):

 cartilage mesh can interesect the backsides of the femoral condyles, or the 
 the sides of the trochealar groove multiple times. The min_proximity can be 
 set to a negative value if you would like distance maps for the out of 

What is a distance map?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 182 at r3 (raw file):

 mapped.

 This component has some outputs such as tri_proximity, tri_pressure, and 

Would it be possible to use the full word triangle everywhere?

Relatedly, should these be named mesh_proximity, etc.?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 191 at r3 (raw file):

 summary metrics in a subspace of the mesh. The entries in the 
 1 x 6 SimTK::Vector correspond to the subset of 
 triangles whose center is located in the half space [+x, -x, +y, -y, +z, -z].

I don't completely follow how these subspaces are defined.


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 197 at r3 (raw file):

 medial-lateral axis, points medially, and the origin is located between the 
 femoral condyles, then the regional outputs corresponding to +z and -z will
 enable comparisons of the loading in the medial and lateral compartments. 

These outputs seem difficult to post-process. What do you think about including a Matlab or Python example for post-processing these outputs to create a 3-D visualization? Would this be a lot of work?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 241 at r3 (raw file):

    OpenSim_DECLARE_PROPERTY(elastic_foundation_formulation, std::string,
        "Formulation for depth-pressure relationship: "
        "'linear' or 'nonlinear'")

These comments should mention the default value.


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 260 at r3 (raw file):

        "Target mesh for collision detection.")
    OpenSim_DECLARE_SOCKET(casting_mesh, Smith2018ContactMesh,
        "Ray casting mesh for collision detection.")

The use of sockets allows a single mesh to be used with multiple forces, which is great.


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 267 at r3 (raw file):

    //=========================================================================
    //number of colliding triangles
    OpenSim_DECLARE_OUTPUT(target_total_n_contacting_tri, int,

Consider "num" instead of "n".


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 273 at r3 (raw file):

    //tri proximity
    OpenSim_DECLARE_OUTPUT(target_tri_proximity, SimTK::Vector,

What is the length of this Vector (and all other vectors)? Does it change with time/states? How does a user associate an element of this vector with the triangles in the mesh?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 424 at r3 (raw file):

    //number of contacting triangles
    int getTargetNContactingTri(const SimTK::State& state) const {

Consider "Num" instead of "N"


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 581 at r3 (raw file):

    }

    SimTK::Vector_<SimTK::Vec3> getTargetRegionalCenterOfProximity(

In what frame are these locations expressed?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 614 at r3 (raw file):

    //contact force
    SimTK::Vec3 getTargetContactForce(const SimTK::State& state) const {

In what frame are the forces expressed? To what point are they applied?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 656 at r3 (raw file):

    OpenSim::Array<double> getRecordValues(const SimTK::State& s) const;
    OpenSim::Array<std::string> getRecordLabels() const;

Could you document here what the labels are?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 664 at r3 (raw file):

    void computeForce(const SimTK::State& state,
        SimTK::Vector_<SimTK::SpatialVec>& bodyForces,
        SimTK::Vector& generalizedForces) const override;

This function and the one above could be public, I think?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 697 at r3 (raw file):

    void constructProperties();

    static void calcNonlinearPressureResid(

Consider avoiding abbreviations; "Resid" -> "Residual"


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 704 at r3 (raw file):

    //Member Variables
    //=========================================================================
    struct nonlinearContactParams {

Names of structs should start with a capital letter.


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 734 at r3 (raw file):

//              Smith2018ArticularContactForce :: CONTACT PARAMETERS
//=============================================================================
class OSIMSIMULATION_API Smith2018ArticularContactForce::ContactParameters :

Would you ever want different contact parameters for the same contact mesh? That is, if I have meshes A, B, and C, with contact force X between A-B and contact force Y between A-C, would I want A in X to have different contact parameters than A in Y?

Perhaps the contact parameters could be part of the Smith2016ContactMesh. It might make sense to use a different class name, in that case. I'm not sure.

We are not constrained to using the same class organization as for the other contact models.


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 574 at r3 (raw file):

            int ipvt[1];
            double qtf[1];
            double wa1[1], wa2[1], wa3[1], wa4[1];

It would be nice to create a helper function in this class so all this "noise" is removed from this function.


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 801 at r3 (raw file):

    force(0) = normal(0) * pressure * area;
    force(1) = normal(1) * pressure * area;
    force(2) = normal(2) * pressure * area;

No need to do this element-wise; force = normal * pressure * area; should work.


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 814 at r3 (raw file):

    force(0) = normal(0) * pressure * area;
    force(1) = normal(1) * pressure * area;
    force(2) = normal(2) * pressure * area;

No need to vectorize.


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 818 at r3 (raw file):

    moment(0) = force(2) * center(1) - force(1) * center(2);
    moment(1) = force(0) * center(2) - force(2) * center(0);
    moment(2) = force(1) * center(0) - force(0) * center(1);

Simbody has a function cross(), also accessible with the % operator.


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 107 at r3 (raw file):

        "Maximum thickness threshold for elastic layer [m] when calculating "
        "cartilage thickness for each triangle.")
    OpenSim_DECLARE_PROPERTY(scale_factors,SimTK::Vec3,

Add a space after the comma.


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 149 at r3 (raw file):

    int getNumFaces() const {
        return _mesh.getNumFaces();}

This placement of the brackets is not conventional in our codebase. One option is to place the closing bracket on a separate line.


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 191 at r3 (raw file):

        return _vertex_locations;}

    const OBBTreeNode& getObbTreeNode() const {

For consistency, the name of the function should be OBBTreeNode.


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 264 at r3 (raw file):

public:
    class OBBTreeNode {

Does this class need to be public?


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 266 at r3 (raw file):

    class OBBTreeNode {
        public:
            OBBTreeNode() : _child1(NULL), _child2(NULL), _numTriangles(0) {

With C++11, we use nullptr instead of NULL.


OpenSim/Simulation/Model/Smith2018ContactMesh.cpp, line 283 at r3 (raw file):

        _tri_center(i)(0) = (v1(0) + v2(0) + v3(0)) / 3.0;
        _tri_center(i)(1) = (v1(1) + v2(1) + v3(1)) / 3.0;
        _tri_center(i)(2) = (v1(2) + v2(2) + v3(2)) / 3.0;

No need to do this element-wise.


OpenSim/Simulation/Model/Smith2018ContactMesh.cpp, line 292 at r3 (raw file):

        double mag = sqrt(
            cross[0] * cross[0] + cross[1] * cross[1] + cross[2] * cross[2]);

Use .norm().


OpenSim/Simulation/Model/Smith2018ContactMesh.cpp, line 390 at r3 (raw file):

}

void Smith2018ContactMesh::computeVariableCartilageThickness() {

Is it necessary for this contact model to be specific to cartilage or joint contact? Or can this handle generic contact?

Copy link
Member

@chrisdembia chrisdembia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Colin, I looked through Smith2018ContactMesh a little bit. I think these two components might be our most complex components!

The next time we chat synchronously, perhaps we could discuss (a) whether Smith2018ContactMesh should derive from ContactGeometry and if Smith2018ContactMesh should have its own scale factors.

Reviewable status: 5 of 12 files reviewed, 57 unresolved discussions (waiting on @clnsmith)


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 268 at r5 (raw file):

    // OUTPUTS
    //=========================================================================
    //number of colliding triangles

Please always place a space after //.


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 511 at r5 (raw file):

                tri_pressure(i) = K * tri_proximity(i) / h;
                tri_energy(i) = 0.5 * tri_area(i) * K *
                    pow(tri_proximity(i), 2) / h;                

Use SimTK::square() instead.


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 537 at r5 (raw file):

            double depthC = kT / (kT + kC)*tri_proximity(i);

            double energyC = 0.5 * tri_area(i) * kC * pow(depthC, 2);

Use SimTK::square().


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 634 at r5 (raw file):

* @param flag2 A status flag
* @param ptr Pointer to data structure containing values for h1, h2, k1, k2, dc
*/

Doxygen comments should go in the header.


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 1015 at r5 (raw file):

    stats.center_of_proximity(0) = COPrx_x;
    stats.center_of_proximity(1) = COPrx_y;
    stats.center_of_proximity(2) = COPrx_z;

No need to do this elementwise.


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 1018 at r5 (raw file):

    //Center of Pressure
    SimTK::Vector Num = tri_pressure.elementwiseMultiply(tri_area);

Only the names of classes/structs should start with an upper-case letter.


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 1036 at r5 (raw file):

    stats.center_of_pressure(0) = COPx;
    stats.center_of_pressure(1) = COPy;
    stats.center_of_pressure(2) = COPz;

No need to do this elementwise.


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 1155 at r5 (raw file):

    constructProperty_poissons_ratio(0.0);
    constructProperty_thickness(0.0);
    constructProperty_use_variable_thickness(true);

These defaults should be functional rather than 0. If you want to force users to always provide a value, you could use NaN as the default and check in extendFinalizeFromProperties() that the user provided a value, but I prefer providing reasonable defaults.


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 34 at r5 (raw file):

//=============================================================================
//                       Smith2018ArticularContact
//=============================================================================

In general, this class description could be organized better so that the information relevant to all users come first. For example, most users will likely inherit meshes from others. So details about how to create a mesh could go farther down, and it would be nice to have a subheading such as "Choosing the coarseness of the mesh".


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 41 at r5 (raw file):

only the contact surface. The mesh does not need to be closed (ie water tight) 
and a smaller number of triangles in the mesh will lead to faster 
collision dection performance. The normal vectors of the mesh should be 

typo "dection"


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 43 at r5 (raw file):

collision dection performance. The normal vectors of the mesh should be 
pointing outwards from the articular surface towards the opposing contact 
mesh. Misdirected triangle normals is a common issue when constructing new 

What happens if the objects are facing the opposite direction from what's intended? For example, what happens if the tibia is above the femur? This specific scenario is not likely, but it's possible in general.


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 47 at r5 (raw file):

the triangle areas and normals, and the derivatives of these outputs with 
respect to joint coordinates are commonly calculated in integrators and 
optimizers, the best performance will be achieved with a smooth mesh whos 

whos -> whose


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 53 at r5 (raw file):

number of triangles in the mesh. For details on a convergence study based on 
triangle area see [1]. Note that the GPU implementation described in the paper 
are not implemented here.

are not -> is not


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 58 at r5 (raw file):

Force component to be in contact with another Smith2018ContactMesh. The 
collision detection algorithm is described in the Smith2018ArticularContact
doxygen. The Smith2018ContactMesh stores all geometric mesh data and also

"doxygen" -> "class description"


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 71 at r5 (raw file):

mesh_back_file mesh. If the calculated thickness is outside the range defined
by the min_thickness and max_thickness properties then it is set to the 
respective bound. 

Consider rewording as "We clip the thickness so it remains within the range defined by min_thickness and max_thickness."


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 81 at r5 (raw file):

scale_frame socket for both the femur and tibia Smith2018ContactMeshes would 
be connected to the femur frame to ensure both meshes are scaled by the femur 
scale factors.

I'm not sure this type of scaling should be done as part of the Scale Tool. It would be helpful to understand the use case.

What happens if the model is scaled multiple times? Does the mesh keep increasing?


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 86 at r5 (raw file):

class OSIMSIMULATION_API Smith2018ContactMesh : public ContactGeometry {

Do you use facilities provided by the ContactGeometry class?


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 116 at r5 (raw file):

    OpenSim_DECLARE_SOCKET(scale_frame, PhysicalFrame,
        "When using the ScaleTool, the scale factors from this frame will be "
        "used to scale the mesh.")

Could you use the frame to which the ContactGeometry is attached?


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 140 at r5 (raw file):

    //TODO This function must be overriden for because this component is 
    //derived from OpenSim::ContactGeometry. Is this the right way to do 
    // nothing?

Your implementation here seems to indicate that you shouldn't derive from ContactGeometry.


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 211 at r5 (raw file):

    const PhysicalFrame& getMeshFrame() const {
        return getComponent<PhysicalOffsetFrame>("mesh_frame");

How do you ensure this subcomponent exists?


OpenSim/Simulation/Model/Smith2018ContactMesh.cpp, line 139 at r5 (raw file):

    mesh_frame->set_orientation(get_orientation());

    adoptSubcomponent(mesh_frame);

It would be good to make sure that calling initSystem() on the model multiple times doesn't cause multiple subcomponents to be added.

Copy link
Member

@carmichaelong carmichaelong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just finished up a first pass of the code. Thanks for all the hard work on getting this ready. For such a huge class, it was mostly easy to follow along. Happy to chat about steps moving forward, but I think this your goal of a stable interface by the workshop is a good timeline.

Reviewable status: 5 of 12 files reviewed, 75 unresolved discussions (waiting on @clnsmith)


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 66 at r5 (raw file):

The major speed-up in the algorithm leverages the fact that changes in joint
coordinates and thus articular contact patterns between time steps are
generally small. Thus, after reposing the meshes, (i.e. realizePosition()) each

Is it guaranteed that "time steps are small" or should users only use this in cases when time steps are on? (e.g., is there anything stopping users from using this contact in a forward simulation with very loose tolerances or with fixed time steps?)


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 87 at r5 (raw file):

casting_mesh and target_mesh are switched, the simulation results will NOT be
exactly the same. For best performance, the casting_mesh should be set to the
mesh that contains the smaller number of triangles.

Comments like this that explain some of the implications for users are great!


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 156 at r5 (raw file):

force equilibrium, assuming that the force applied to a pair of contacting
triangles is equal and opposite. This formulation further assumes that the
triangles in contact have the same area. The fourth equation states that the

Does this mean that when using this class, one should make sure that triangles in contact have the same area always? How sensitive is the formulation when this assumption is not followed?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 236 at r5 (raw file):

    // PROPERTIES
    //=========================================================================
    OpenSim_DECLARE_PROPERTY(min_proximity, double, "The minimum overlap "

What happens if a user sets min_proximity > max_proximity?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 405 at r5 (raw file):

    Smith2018ArticularContactForce();

    Smith2018ArticularContactForce(const std::string& name,

When should a user use this constructor or another constructor? e.g., does this particular one use the parameters of the mesh passed in while the one below overrides the mesh with the passed in contact parameters?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.h, line 745 at r5 (raw file):

    // PROPERTIES
    //=========================================================================
        OpenSim_DECLARE_PROPERTY(elastic_modulus, double, 

What are the default values?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 135 at r5 (raw file):

    Super::extendAddToSystem(system);

    Smith2018ArticularContactForce::ContactParameters target_mesh_params = 

Are these two parameters used in this function?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 221 at r5 (raw file):

        casting_mesh_def_vec3, Stage::Dynamics);

    addCacheVariable<double>("target.contact_area",0, Stage::Dynamics);

Add a space between "," and "0"


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 433 at r5 (raw file):

    else {
        cache_mesh_name = "target";
        tContactParams = get_casting_mesh_contact_params();

The variable names look flipped here, but I'm assuming this is to account for some flip in which is the cached mesh? Maybe some comments here could help for maintainability?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 515 at r5 (raw file):

            }

            if (get_elastic_foundation_formulation() == "nonlinear") {

Is there a specific reason to use continue's here rather than an if/else if statement? What happens if get_elastic_foundation_formulation returns something other than "linear" or "nonlinear"?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 561 at r5 (raw file):

            //solution params
            double ftol = 1e-4, xtol = 1e-4, gtol = 0.0;

Should a user be able to adjust these parameters for their own model?


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 713 at r5 (raw file):

    auto stats = computeContactStats(casting_mesh, casting_tri_proximity,
        casting_tri_pressure,casting_faces);

space after comma


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 854 at r5 (raw file):

    for (int i = 0; i < 6; ++i) {
        auto stats = computeContactStats(casting_mesh, casting_tri_proximity,
        casting_tri_pressure, casting_region_tri_ind[i]);

extra indentation to improve readability


OpenSim/Simulation/Model/Smith2018ArticularContactForce.cpp, line 1062 at r5 (raw file):

    OpenSim::Array<std::string> labels("");

    labels.append(getName() + ".casting.contact_area");    

Would be great to add this list in the comments/doxygen of the .h file


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 55 at r5 (raw file):

are not implemented here.

This component can only be used with the Smith2018ArticularContact 

How is this enforced? If a user gives an ill-posed model (e.g., expecting this to contact another type of Force) what feedback will a user get?


OpenSim/Simulation/Model/Smith2018ContactMesh.h, line 125 at r5 (raw file):

    Smith2018ContactMesh();

    Smith2018ContactMesh(const std::string& name,

Like in the other class, which constructors should I use for what cases? What assumptions/defaults are used when using one constructor over the other?


OpenSim/Simulation/Model/Smith2018ContactMesh.cpp, line 229 at r5 (raw file):

    _mesh_is_cached = true;

    // Initialize Reused Variables

Looks like this section is no longer needed?


OpenSim/Simulation/Model/Smith2018ContactMesh.cpp, line 785 at r5 (raw file):

    //this should be adjusted for precision, a=0 when e1 and h are pependicular

    if (a > -0.00000001 && a < 0.00000001) {

Could make this a property with a default value for now to avoid hard-coding a constant? Would still keep the door open for some automated precision down the road with another optional boolean? (not sure what's best here to avoid the magic numbers)

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.

3 participants