Use relative paths in updateFromXMLNode() for socket connectees #1751

chrisdembia · 2017-06-07T08:19:09Z

This PR is a first pass at addressing part of #1609.

The "hackathon" warning is caused by two workflows:

(a) loading an old OSIM file, for which the conversion to socket connectee names performed in updateFromXMLNode() uses only names, not the correct relative paths; and
(b) programmatic model-building causes intermediate connectee name paths to be incorrect while components are orphans (before they are added to the model).

This PR shows how we can avoid warnings caused by (a). I am about halfway through editing the updateFromXMLNode() to avoid these warnings; I have to still tackle the Constraint classes that were modified to use sockets.

The most controversial aspect of this PR is probably that I have edited the updateFromXMLNode() code for previous XMLDocument versions, rather than creating a new XMLDocument version. Creating a new version would make the fix much more complex, and might actually be the wrong thing to do. If the perspective is that this is a bug fix, then it might be fine that we are editing the update code for a pre-existing XMLDocument version.

This commit attempts to correct the connectee name paths created in updateFromXMLNode() to avoid subsequent warnings when trying to connect a model that is loaded from an old XML file. [skip ci]

tkuchida · 2017-06-08T05:18:00Z

FWIW, I don't think it's safe to be modifying updateFromXMLNode() code until we have corresponding tests in place (#206).

chrisdembia · 2017-06-08T05:46:46Z

@tkuchida: @aseth1 and I were planning on discussing this issue during the dev meeting time, if you're interested in joining.

chrisdembia · 2017-06-08T17:44:22Z

FWIW, I don't think it's safe to be modifying updateFromXMLNode() code until we have corresponding tests in place (#206).

I might consider fixing the bouncing block issue as part of this work (opensim-org/opensim-gui#68).

chrisdembia · 2017-06-08T19:57:18Z

Thanks @aseth1 and @tkuchida for chatting about this issue earlier today. The plan will be to focus on fixing the updateFromXMLNode portion of the issue. I will try to locally test my changes using old models to protect against introducing new bugs: I will ensure that the master branch and my branch produce the same updated 4.0 model (except for the correction to connectee names).

As for addressing the fact that the warning also gets generated when building a model programmatically, I will update sockets to use the connectee pointer rather than the connectee name to establish the connection during finalizeConnections(), and deprecate the parts of the interface where users pass string names to specify connectees. I will also need to update print() to throw an exception if finalizeConnections()/finalizeProperties() is not yet called, or alternatively, we could make addComponent() call finalizeConnections(). It is not clear if I will be able to achieve this before we release 4.0.

tkuchida · 2017-06-08T20:13:18Z

Sounds good @chrisdembia.

I will also need to update print() to throw an exception if finalizeConnections()/finalizeProperties() is not yet called

Precedent for this approach has been established in Model::printBasicInfo().

Also, fix a bug in TwoFrameLinker::updateFromXMLNode() wherein the connectee name was not set correctly when offset frames were required.

…ateFromXMLNode_socket_paths

aseth1 · 2017-06-19T22:49:05Z

we could make addComponent() call finalizeConnections()

This should already by the case.

chrisdembia · 2017-06-20T06:18:13Z

This should already be the case.

I think addComponent() calls finailzeFromProperties(), but not finalizeConnections().

chrisdembia · 2017-06-20T06:22:46Z

This PR is now ready for review (no longer WIP).

This PR helps avoid generating a "could not find connectee" message (in the
future) caused by updating XML for old model files whose dependence on other
components now uses the Socket mechanism. This PR avoids such warnings by using
the relative path to the connectee, which we should know with certainty, given
that pre-4.0 models could not have an arbitrary organization of components.

Here's an example before-and-after comparison:

Before:

<PathPoint name="muscle2-point1">
    <!--Path to a Component that satisfies the Socket 'parent_frame' of type PhysicalFrame (description: The frame in which this path point is defined.).-->
    <socket_parent_frame_connectee_name>ground</socket_parent_frame_connectee_name>
    ...
</PathPoint>

After:

<PathPoint name="muscle2-point1">
    <!--Path to a Component that satisfies the Socket 'parent_frame' of type PhysicalFrame (description: The frame in which this path point is defined.).-->
    <socket_parent_frame_connectee_name>../../../ground</socket_parent_frame_connectee_name>
    ...
</PathPoint>

I ensured that the following classes will no longer cause the "could not find
connectee" when loaded from XML (all these classes previously generated such a
message):

AbstractPathPoint
ConditionalPathPoint
MovingPathPoint
ContactGeometry
PrescribedForce
TwoFrameLinker
ConstantDistanceConstraint
PointConstraint
PointOnLineConstraint
RollingOnSurfaceConstraint

I tested this by uncommenting the message in Socket::findAndConnect(), and
loading the following models (taken from throughout the repository):

arm26
BouncingBall_HuntCrossley
BouncingBallModelEF
BushingForceModel_300000
BushingForceOffsetModel_300000
gait10dof18musc_subject01
MovingPointCustomJointMomentArmTest
PushUpToesOnGroundExactConstraints
PushUpToesOnGroundLessPreciseConstraints
PushUpToesOnGroundWithMuscles
subject02_running_RRA_cycle02_sim02_07_v232
toyLeg
tugOfWar_model
tugOfWar_model_ThelenOnly

These models were selected because they contain the components listed above.

By default, the "could not find connectee" message is still not emitted
(emitting this message by default would cause lots of noise when building a
model programmatically). Now, however, the user can set the Object debug level
to a number greater than 0 to obtain these messages.

I also fixed the following unrelated bugs:

TwoFrameLinker correctly handles offset frames now (Fixes TwoFrameLinker updateFromXMLNode() does not handle offset frames correctly #1754).
BushingForceOffsetModel was introduce to test that this bug is fixed.
PhysicalFrame now preserves orientation of VisibleObjects from old models
(Fixes In updating XML of VisibleObject DisplayGeometry, orientation from transform not preserved #1759). I tested this with a modified version of tugOfWar_model (not in the
repo).

Here are some issues with this PR:

Sometimes, portions of a model exist in files other than the model file
(e.g., External Loads files, Force Set files, ControllerSet in the
AnalyzeTool). If/when these "external" components use sockets, the connectee
names would need to be relative paths in the model to which they are not yet a
part (in order to avoid the "could not find connectee" message). I think it's
somewhat awkward to need to specify a relative path to a component that's not
even in the same file.
I edited ExternalLoads::updateFromXMLNode() for an old XML version number
because this function directly uses PrescribedForce, and PrescribedForce's updated updateFromXMLNode()
code causes a change in behavior (this is why I think it is best not to use
actual Objects/Components in updateFromXMLNode()). This was a little bit like
rewriting history.
After we release 4.0, we will likely continue converting components to use
sockets. However, we will no longer be able to correctly assume a relative path
to the component (4.0 models can be organized in a more arbitrary fashion). So
I am not sure the solution we used in this PR will work as well after we
release 4.0.
Files in post-3.3 versions (e.g., 30505) will still generate the "could
not find connectee" warning if the connectee names are not relative paths (this
is why I updated two of the models in the repo that have post-3.3 versions).

chrisdembia · 2017-06-20T06:28:16Z

OpenSim/Simulation/Model/PhysicalFrame.cpp

 {
    SimTK::Xml::Element frameNode("PhysicalOffsetFrame");
    frameNode.setAttributeValue("name", frameName);
    stringstream ss;
    ss << localXform[3] << " " << localXform[4] << " " << localXform[5];
    SimTK::Xml::Element translationNode("translation", ss.str());
-    ss.clear();
+    ss = stringstream();


Without this, ss would still contain localXform[3], localXform[4], and localXform[5].

I don't think we have to wipeout the existing stream, just reset it to empty string, e.g. ss.str(""); and then ss.clear();

aseth1

LGTM I only have one comment that is optional to address (if you agree).

I am still concerned about our coverage of updateFromXML code which is some of the more complicated code with thin coverage. Beyond this PR, but lets continue to think of systematic tests to improve our coverage.

aseth1 · 2017-06-20T17:54:42Z

OpenSim/Simulation/Test/testForces.cpp

+    // It's necessary to correct the connectee names in the BushingForce, which
+    // we can do with finalizeConnections() (they are incorrect otherwise
+    // because `spring` is initially orphaned).
+    osimModel.finalizeConnections(osimModel);


I interpret this operation differently. If you "print()" a Component you get the existing values for its properties and connections. You call finalizeConnections before printing because you want the serialization to reflect the existing connections and want to flag any errors. You could print without calling finalizeConnections if you didn't care about the connections, for example, you are going to add more components when you reload.

I'm fine with your interpretation. This change was necessary to get the equality comparison to pass. I can investigate if alternate changes to the code can make the test pass. If not, then I will at least update the comment.

Can you update the comment to something like: "To print (serialize) the latest connections of the model, it is necessary to finalizeConnections() first." Or something to that effect? I think the code is fine.

@chrisdembia wish to update the comment?

Yes I will; thanks for your response. I've been meaning to update this PR but haven't gotten around to it yet.

- std::function needs <functional> header - std::stringstream copy constructor is deleted.

jimmyDunne · 2017-07-25T21:09:34Z

@aseth1 will look at again.

aseth1 · 2017-08-25T21:47:45Z

@tkuchida added you as second reviewer since you've been part of the discussion here. Hope you don't mind.

tkuchida

Looks fine to merge, but going forward I suggest we consider one or more of the following:

add a repository of representative models in various formats to improve model conversion testing (Add test case for model conversion #206)
release an XML schema for OSIM files (Need Schema for osim files #775)
update OSIM files one version at a time (updateFromXMLNode(): Connector/Socket updates can occur out of order #1588)

chrisdembia · 2017-08-28T06:30:20Z

Thanks for reviewing, @tkuchida .

chrisdembia · 2017-08-28T18:20:12Z

Thanks @aseth1. I'll try to submit a small followup PR soon to fix the comment in testForces.

aseth1 · 2017-08-28T18:33:52Z

@chrisdembia sure. Sounds good.

First pass at avoiding findAndConnect() exception.

89f1e0f

This commit attempts to correct the connectee name paths created in updateFromXMLNode() to avoid subsequent warnings when trying to connect a model that is loaded from an old XML file. [skip ci]

chrisdembia mentioned this pull request Jun 8, 2017

TwoFrameLinker updateFromXMLNode() does not handle offset frames correctly #1754

Closed

chrisdembia added 2 commits June 8, 2017 19:29

Fix updateFromXMLNode() addConnector() for TwoFrameLinker.

f46d149

Also, fix a bug in TwoFrameLinker::updateFromXMLNode() wherein the connectee name was not set correctly when offset frames were required.

Merge branch 'master' of github.com:opensim-org/opensim-core into upd…

433b128

…ateFromXMLNode_socket_paths

chrisdembia mentioned this pull request Jun 12, 2017

In updating XML of VisibleObject DisplayGeometry, orientation from transform not preserved #1759

Closed

Remove unnecessary ;.

a715923

Avoid "could not find connectee" when updating XML.

801c6f6

chrisdembia changed the title ~~[WIP] First pass at avoiding findAndConnect() exception.~~ First pass at avoiding findAndConnect() exception. Jun 20, 2017

chrisdembia changed the title ~~First pass at avoiding findAndConnect() exception.~~ Avoid "could not find connectee" warning when updating XML files Jun 20, 2017

chrisdembia changed the title ~~Avoid "could not find connectee" warning when updating XML files~~ Use relative paths in updateFromXMLNode() for socket connectees Jun 20, 2017

chrisdembia commented Jun 20, 2017

View reviewed changes

Improve comment in Model::updateFromXMLNode()

82a6dd7

aseth1 mentioned this pull request Jun 20, 2017

OffsetFrame should consistently use rotation and translation to desrcribe its Transfrom #1778

Closed

aseth1 approved these changes Jun 20, 2017

View reviewed changes

Fix compiler bugs

1fc5878

- std::function needs <functional> header - std::stringstream copy constructor is deleted.

aseth1 requested a review from tkuchida August 25, 2017 21:46

tkuchida approved these changes Aug 28, 2017

View reviewed changes

aseth1 merged commit d531a56 into master Aug 28, 2017

aseth1 deleted the updateFromXMLNode_socket_paths branch August 28, 2017 18:17

chrisdembia mentioned this pull request Aug 28, 2017

Update comment in testForces on finalizeConnections() #1899

Merged

chrisdembia mentioned this pull request Feb 13, 2018

Fix error cases with Component::findStateVariable(). #2083

Merged

Use relative paths in updateFromXMLNode() for socket connectees #1751

Use relative paths in updateFromXMLNode() for socket connectees #1751

Uh oh!

Conversation

chrisdembia commented Jun 7, 2017

Uh oh!

tkuchida commented Jun 8, 2017

Uh oh!

chrisdembia commented Jun 8, 2017

Uh oh!

chrisdembia commented Jun 8, 2017

Uh oh!

chrisdembia commented Jun 8, 2017

Uh oh!

tkuchida commented Jun 8, 2017

Uh oh!

aseth1 commented Jun 19, 2017

Uh oh!

chrisdembia commented Jun 20, 2017

Uh oh!

chrisdembia commented Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisdembia Jun 20, 2017

Choose a reason for hiding this comment

Uh oh!

aseth1 Aug 18, 2017

Choose a reason for hiding this comment

Uh oh!

aseth1 left a comment

Choose a reason for hiding this comment

Uh oh!

aseth1 Jun 20, 2017

Choose a reason for hiding this comment

Uh oh!

chrisdembia Jun 25, 2017

Choose a reason for hiding this comment

Uh oh!

aseth1 Aug 18, 2017

Choose a reason for hiding this comment

Uh oh!

aseth1 Aug 25, 2017

Choose a reason for hiding this comment

Uh oh!

chrisdembia Aug 25, 2017

Choose a reason for hiding this comment

Uh oh!

jimmyDunne commented Jul 25, 2017

Uh oh!

aseth1 commented Aug 25, 2017

Uh oh!

tkuchida left a comment

Choose a reason for hiding this comment

Uh oh!

chrisdembia commented Aug 28, 2017

Uh oh!

chrisdembia commented Aug 28, 2017

Uh oh!

aseth1 commented Aug 28, 2017

Uh oh!

Uh oh!

chrisdembia commented Jun 20, 2017 •

edited

Loading