Remove kinematics_for_external_loads property

[aymanhab]

Remove properties from from ExternalLoads object and remove usage in AbstractTool and DynamicsTool.

Fixes issue opensim-org/opensim-gui#348

Brief summary of changes

Testing I've completed

Looking for feedback on...

CHANGELOG.md (choose one)

• no need to update because...
• updated...

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 

[chrisdembia]

#2334

[chrisdembia]

Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @aymanhab)

---

OpenSim/Analyses/InducedAccelerations.cpp, line 981 at r2 (raw file):

                const Body &appliedToBody = _model->getBodySet().get(appliedToBodyIndex);
                const Frame* expressedInBody =
                        _model->findComponent<Frame>(

I do not think we should use findComponent(). I would prefer if we just have a special case for "ground".

Ideally, we would implement updateFromXMLNode() to change the body names into appropriate body paths.

---

OpenSim/Simulation/Model/ExternalLoads.cpp, line 417 at r2 (raw file):

                    if (transcoded.length()>0)
                std::cout << "Warn: external_loads_model_kinematics_file option is not supported anymore"
                        << " result could change, please inspect and update accordingly" << std::endl;

Does this mean a user might see multiple redundant warnings?

---

OpenSim/Simulation/Model/ExternalLoads.cpp, line 417 at r2 (raw file):

                    if (transcoded.length()>0)
                std::cout << "Warn: external_loads_model_kinematics_file option is not supported anymore"
                        << " result could change, please inspect and update accordingly" << std::endl;

Use log_warn().

---

OpenSim/Simulation/Model/ExternalLoads.cpp, line 488 at r2 (raw file):

    else 
        if (documentVersion < 40200) {
            // Warn on depreacted external_loads_kinematics_specification

This feature is not deprecated, it's removed.

---

OpenSim/Simulation/Model/ExternalLoads.cpp, line 499 at r2 (raw file):

                              << " result could change, please inspect and "
                                 "update accordingly"
                              << std::endl;

Can you use log_warn()? The second half of this comment is somewhat vague; I might simplify to "Results may change."

[aymanhab]

Thanks @chrisdembia This PR predates the change to logging so log_ functions were not available then. Will update accordingly. Generally we don't keep track if a warning is issued while upgrading and i don't think we will because the upgrade is per object not per file/document.

[aymanhab]

Reviewable status: 0 of 18 files reviewed, 5 unresolved discussions (waiting on @chrisdembia)

---

OpenSim/Analyses/InducedAccelerations.cpp, line 981 at r2 (raw file):

Previously, chrisdembia (Christopher Dembia) wrote…

I do not think we should use findComponent(). I would prefer if we just have a special case for "ground".

Ideally, we would implement updateFromXMLNode() to change the body names into appropriate body paths.

Changed to fix proposed/reviewed earlier and approved in another open PR. With only name available, we have no option other than findComponent unless we build in specific assumptions about Component tree layout.

---

OpenSim/Simulation/Model/ExternalLoads.cpp, line 417 at r2 (raw file):

Previously, chrisdembia (Christopher Dembia) wrote…

Does this mean a user might see multiple redundant warnings?

Yes as typical of all XML format changes, once per object

---

OpenSim/Simulation/Model/ExternalLoads.cpp, line 417 at r2 (raw file):

Previously, chrisdembia (Christopher Dembia) wrote…

Use log_warn().

Done.

---

OpenSim/Simulation/Model/ExternalLoads.cpp, line 488 at r2 (raw file):

Previously, chrisdembia (Christopher Dembia) wrote…

This feature is not deprecated, it's removed.

Done.

---

OpenSim/Simulation/Model/ExternalLoads.cpp, line 499 at r2 (raw file):

Previously, chrisdembia (Christopher Dembia) wrote…

Can you use log_warn()? The second half of this comment is somewhat vague; I might simplify to "Results may change."

Done.

[chrisdembia]

@aymanhab can you merge master into this branch to remove the excess changes in the diff?

Reviewed 1 of 8 files at r5.
Reviewable status: 1 of 18 files reviewed, 5 unresolved discussions (waiting on @aymanhab and @chrisdembia)

---

OpenSim/Analyses/InducedAccelerations.cpp, line 981 at r2 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Changed to fix proposed/reviewed earlier and approved in another open PR. With only name available, we have no option other than findComponent unless we build in specific assumptions about Component tree layout.

I advocated against using findComponent() because there could be multiple bodies with the same name, and the behavior of the code then depends on the order of components in the model file.

The ideal behavior is that files from before 4.0 are updated to use the only path that would have been valid /bodyset/<body-name>, and current files must provide an exact path.

Mimicking the behavior of other components is better.

One way to address the issue of using findComponent() is to document this "searching" behavior in the comment for the applied_to_body and expressed_in_body properties.

FYI there is now a more succinct syntax for exception messages:

                OPENSIM_THROW_IF_FRMOBJ(appliedToBody == nullptr, Exception,
                        "ExternalForce's appliedToBody {} not found.", exf->getAppliedToBodyName());

(No need to update here)

[aymanhab]

Reviewable status: 1 of 18 files reviewed, 5 unresolved discussions (waiting on @chrisdembia)

---

OpenSim/Analyses/InducedAccelerations.cpp, line 981 at r2 (raw file):

Previously, chrisdembia (Christopher Dembia) wrote…

I advocated against using findComponent() because there could be multiple bodies with the same name, and the behavior of the code then depends on the order of components in the model file.

The ideal behavior is that files from before 4.0 are updated to use the only path that would have been valid /bodyset/<body-name>, and current files must provide an exact path.

Mimicking the behavior of other components is better.

One way to address the issue of using findComponent() is to document this "searching" behavior in the comment for the applied_to_body and expressed_in_body properties.

FYI there is now a more succinct syntax for exception messages:

                OPENSIM_THROW_IF_FRMOBJ(appliedToBody == nullptr, Exception,
                        "ExternalForce's appliedToBody {} not found.", exf->getAppliedToBodyName());

(No need to update here)

@chrisdembia Can you clarify which "other components" do this? I believe this change was specific to Sockets which are used to serialize/deserialize/wire Paths, but it could have been done elsewhere. The files are already in 4.0 format so the assumption that Frames only exist under bodyset can't be guaranteed in general.

Thanks for the pointer regarding the syntax, will definitely use it.

[chrisdembia]

Reviewable status: 1 of 18 files reviewed, 5 unresolved discussions (waiting on @aymanhab and @chrisdembia)

---

OpenSim/Analyses/InducedAccelerations.cpp, line 981 at r2 (raw file):

Can you clarify which "other components" do this?

I meant that I think that your recent change using findComponent() (copied from another PR) is better than your original proposal in this PR; even though it's still using findComponent(), at least it's consistent with other code.

[aymanhab]

Reviewable status: 1 of 18 files reviewed, 5 unresolved discussions (waiting on @chrisdembia)

---

OpenSim/Analyses/InducedAccelerations.cpp, line 981 at r2 (raw file):

Previously, chrisdembia (Christopher Dembia) wrote…

Can you clarify which "other components" do this?

I meant that I think that your recent change using findComponent() (copied from another PR) is better than your original proposal in this PR; even though it's still using findComponent(), at least it's consistent with other code.

Done.

[chrisdembia]

@aymanhab I don't know why this keeps happening but the diff contains changes that are already in master.

[chrisdembia]

Reviewable status: 1 of 18 files reviewed, 5 unresolved discussions (waiting on @aymanhab and @chrisdembia)

---

OpenSim/Simulation/Model/ExternalLoads.cpp, line 417 at r2 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Yes as typical of all XML format changes, once per object

@aymanhab I mean that this warning is repeated in this function, so the user might see this twice per object.

[aymanhab]

@chrisdembia the two warnings are on two different control paths of if/else so they can't both be true AFAIK 😄

I'll look into the diffs issue.

[aymanhab]

Running IAA in 3.3 without kinematics for external loads gives identical results to branch with the fix as expected. I can post plots but visibly the totals are indistinguishable which is rather expected if we didn't introduce a bug elsewhere. Running IAA in 3.3 where same kinematics used for analysis is used for kinematics of external loads produce same result. File attached, graphs are indistinguishable
iaa_diffs_kel.pdf

[aymanhab]

Based on feedback and diffs above this seems ready to go (separate GUI PR will remove it from the application), main issue that I'd like feedback on is whether we upgrade file version number (corresponding to XML schema change). It's included here already but has the side effect that new files/models saved moving forward will not be backward compatible with 4.0 or 4.1, this is needed so that old external loads files are converted once but comes with this inconvenience. Thoughts @carmichaelong @jenhicks @nickbianco @aseth1
We can skip this and rely on the warning each time the file is opened and on documentation instead.

[aymanhab]

Reviewable status: 0 of 14 files reviewed, 2 unresolved discussions (waiting on @chrisdembia)

---

OpenSim/Simulation/Model/ExternalLoads.cpp, line 417 at r2 (raw file):

Previously, chrisdembia (Christopher Dembia) wrote…

@aymanhab I mean that this warning is repeated in this function, so the user might see this twice per object.

warnings are on two separate branches of if/else so users will never see both

[aymanhab]

@jenhicks @carmichaelong @aseth1 Any feedback on this so we can move on? Thank you

[jenhicks]

My initial inclination is to keep backwards compatibility, but I am open to other thoughts.

[aymanhab]

Thanks @jenhicks I tend to agree, particularly that ExternalLoads are saved to separate files typically so losing backward compatibility for model files feels like an overreach. I will undo the version change unless I hear otherwise from reviewers.

[aymanhab]

@carmichaelong one more look and will merge if no objections when you have a chance, thanks

[aymanhab]

Approved in dev meeting by @carmichaelong, Merging.