{"action":"created","comment":{"url":"https://api.github.com/repos/opensim-org/opensim-core/pulls/comments/39214594","id":39214594,"diff_hunk":"@@ -53,28 +53,15 @@ class OSIMSIMULATION_API PhysicalOffsetFrame : public OffsetFrame<PhysicalFrame>\n     //--------------------------------------------------------------------------\n     // CONSTRUCTION\n     //--------------------------------------------------------------------------\n-    /** By default, the frame is not connected to any parent frame,\n-     * and its transform is an identity transform.\n-     */\n-    PhysicalOffsetFrame();\n+    /** Constructors are defined by the OffsetFrame base class */\n+    using OffsetFrame<PhysicalFrame>::OffsetFrame;\n \n-    virtual ~PhysicalOffsetFrame() {}\n-\n-    /**\n-    A convenience constructor that initializes the parent connection and\n-    offset property of this PhysicalOffsetFrame.\n-\n-    @param[in] parent   The parent PhysicalOffsetFrame.\n-    @param[in] offset   The offset transform between this frame and its parent\n-    */\n-    PhysicalOffsetFrame(const PhysicalFrame& parent,\n-                        const SimTK::Transform& offset);\n+    ~PhysicalOffsetFrame() final {}\n \n protected:\n     /** Extend Component interface for adding the PhysicalOffsetFrame to the \n         underlying multibody system */\n-    void extendAddToSystem(SimTK::MultibodySystem& system) const override;\n-\n+    void extendAddToSystem(SimTK::MultibodySystem& system) const override final;","path":"OpenSim/Simulation/Model/PhysicalOffsetFrame.h","position":29,"original_position":29,"commit_id":"96c67fbf69cc1a5d0decc422afad123d85100cbb","original_commit_id":"96c67fbf69cc1a5d0decc422afad123d85100cbb","user":{"login":"aymanhab","id":3972208,"avatar_url":"https://avatars.githubusercontent.com/u/3972208?v=3","gravatar_id":"","url":"https://api.github.com/users/aymanhab","html_url":"https://github.com/aymanhab","followers_url":"https://api.github.com/users/aymanhab/followers","following_url":"https://api.github.com/users/aymanhab/following{/other_user}","gists_url":"https://api.github.com/users/aymanhab/gists{/gist_id}","starred_url":"https://api.github.com/users/aymanhab/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/aymanhab/subscriptions","organizations_url":"https://api.github.com/users/aymanhab/orgs","repos_url":"https://api.github.com/users/aymanhab/repos","events_url":"https://api.github.com/users/aymanhab/events{/privacy}","received_events_url":"https://api.github.com/users/aymanhab/received_events","type":"User","site_admin":false},"body":"I agree with @sherm1 particularly that the error message users get if they try to extend will likely be cryptic which is more harm than good in this case.","created_at":"2015-09-10T21:13:09Z","updated_at":"2015-09-10T21:13:09Z","html_url":"https://github.com/opensim-org/opensim-core/pull/589#discussion_r39214594","pull_request_url":"https://api.github.com/repos/opensim-org/opensim-core/pulls/589","_links":{"self":{"href":"https://api.github.com/repos/opensim-org/opensim-core/pulls/comments/39214594"},"html":{"href":"https://github.com/opensim-org/opensim-core/pull/589#discussion_r39214594"},"pull_request":{"href":"https://api.github.com/repos/opensim-org/opensim-core/pulls/589"}}},"pull_request":{"url":"https://api.github.com/repos/opensim-org/opensim-core/pulls/589","id":43586399,"html_url":"https://github.com/opensim-org/opensim-core/pull/589","diff_url":"https://github.com/opensim-org/opensim-core/pull/589.diff","patch_url":"https://github.com/opensim-org/opensim-core/pull/589.patch","issue_url":"https://api.github.com/repos/opensim-org/opensim-core/issues/589","number":589,"state":"open","locked":false,"title":"Joints connect to PhysicalFrames","user":{"login":"aseth1","id":6494122,"avatar_url":"https://avatars.githubusercontent.com/u/6494122?v=3","gravatar_id":"","url":"https://api.github.com/users/aseth1","html_url":"https://github.com/aseth1","followers_url":"https://api.github.com/users/aseth1/followers","following_url":"https://api.github.com/users/aseth1/following{/other_user}","gists_url":"https://api.github.com/users/aseth1/gists{/gist_id}","starred_url":"https://api.github.com/users/aseth1/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/aseth1/subscriptions","organizations_url":"https://api.github.com/users/aseth1/orgs","repos_url":"https://api.github.com/users/aseth1/repos","events_url":"https://api.github.com/users/aseth1/events{/privacy}","received_events_url":"https://api.github.com/users/aseth1/received_events","type":"User","site_admin":false},"body":"Update to the Joint and its derived classes, which includes:\r\n\r\n1. Connecting to any PhysicalFrame and not just Body (which is a PhysicalFrame)\r\n2. Removal of parent and child Joint frames maintained by location and orientation properties.\r\n3. Joint automatically creates PhysicalOffsetFrames from old Joint definitions (with location and orientation properties) but these constructors are now deprecated.\r\n4. All derived Joints use the base constructors (thus deprecated constructors are isolated to the base Joint)\r\n5. Using a non static initializer (`Joint::constructCoordinate()`) for derived Joints to specify their Coordinates and their MotionType.  The initializer automatically adds the Coordinate to the Joint's CoordinateSet (property).  This was required, because derived joints use the base constructor and thus we cannot have each joint construct its Coordinates in their own constructor. This was temporarily delayed until `finalizeFromProperties()` but that proved unsatisfactory (requiring `finalizeFromProperties to be called everywhere a Joint's coordinate was being accessed). Now only CustomJoint (which requires its SpatialTransform to be specified) has its own constructor and its own constructCoordinates() since the number of dofs cannot be known apriori.\r\n6. Address the fallout from the above modifications, which include updating from older model file versions, different number of components now present in a Model, different drawing order, etc ...\r\n7. I rerouted `Component::getComponent<C>(name)` through the old Component::findComponent() so I could find Frames before calling `Model::connect()` which is when the Component traversal tree gets built. This is kind of a mess, but I would like to address that as a completely separate PR.\r\n\r\nHow to review this PR (in order of priority):\r\n\r\n1. Look at Joint.h and the subsequent concrete Joint classes. Are you happy with the Joint(s) API?\r\n2. Verify that the Joint::updateFromXML (and helper code in XMLDocument) is implemented correctly. I had introduced bugs earlier by calling upon the API to help with the update. @chrisdembia correctly noted that this could be dangerous! Updates should be restricted to XML editing only. If you see otherwise please flag it!\r\n3. `Model::extendFinalizeFromProperties()`'s Multibody tree building is based on bodies and I do this by tunneling through the OffsetFrames to the base. I think this right, but a sanity check here would be appreciated.\r\n4. Finally the smaller accommodating changes and test case updates. \r\n\r\nHappy reviewing. Thanks!\n\n<!-- Reviewable:start -->\n[<img src=\"https://reviewable.io/review_button.png\" height=40 alt=\"Review on Reviewable\"/>](https://reviewable.io/reviews/opensim-org/opensim-core/589)\n<!-- Reviewable:end -->\n","created_at":"2015-08-28T01:54:07Z","updated_at":"2015-09-10T21:13:09Z","closed_at":null,"merged_at":null,"merge_commit_sha":"17dd89bd9a596528338c9233bb7b89c7609012a7","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/opensim-org/opensim-core/pulls/589/commits","review_comments_url":"https://api.github.com/repos/opensim-org/opensim-core/pulls/589/comments","review_comment_url":"https://api.github.com/repos/opensim-org/opensim-core/pulls/comments{/number}","comments_url":"https://api.github.com/repos/opensim-org/opensim-core/issues/589/comments","statuses_url":"https://api.github.com/repos/opensim-org/opensim-core/statuses/96c67fbf69cc1a5d0decc422afad123d85100cbb","head":{"label":"opensim-org:JointsConnectToPhysicalFrames","ref":"JointsConnectToPhysicalFrames","sha":"96c67fbf69cc1a5d0decc422afad123d85100cbb","user":{"login":"opensim-org","id":4370782,"avatar_url":"https://avatars.githubusercontent.com/u/4370782?v=3","gravatar_id":"","url":"https://api.github.com/users/opensim-org","html_url":"https://github.com/opensim-org","followers_url":"https://api.github.com/users/opensim-org/followers","following_url":"https://api.github.com/users/opensim-org/following{/other_user}","gists_url":"https://api.github.com/users/opensim-org/gists{/gist_id}","starred_url":"https://api.github.com/users/opensim-org/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/opensim-org/subscriptions","organizations_url":"https://api.github.com/users/opensim-org/orgs","repos_url":"https://api.github.com/users/opensim-org/repos","events_url":"https://api.github.com/users/opensim-org/events{/privacy}","received_events_url":"https://api.github.com/users/opensim-org/received_events","type":"Organization","site_admin":false},"repo":{"id":20775600,"name":"opensim-core","full_name":"opensim-org/opensim-core","owner":{"login":"opensim-org","id":4370782,"avatar_url":"https://avatars.githubusercontent.com/u/4370782?v=3","gravatar_id":"","url":"https://api.github.com/users/opensim-org","html_url":"https://github.com/opensim-org","followers_url":"https://api.github.com/users/opensim-org/followers","following_url":"https://api.github.com/users/opensim-org/following{/other_user}","gists_url":"https://api.github.com/users/opensim-org/gists{/gist_id}","starred_url":"https://api.github.com/users/opensim-org/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/opensim-org/subscriptions","organizations_url":"https://api.github.com/users/opensim-org/orgs","repos_url":"https://api.github.com/users/opensim-org/repos","events_url":"https://api.github.com/users/opensim-org/events{/privacy}","received_events_url":"https://api.github.com/users/opensim-org/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/opensim-org/opensim-core","description":"SimTK OpenSim 4.0 C++ libraries and command-line applications, and Java/Python wrapping.","fork":false,"url":"https://api.github.com/repos/opensim-org/opensim-core","forks_url":"https://api.github.com/repos/opensim-org/opensim-core/forks","keys_url":"https://api.github.com/repos/opensim-org/opensim-core/keys{/key_id}","collaborators_url":"https://api.github.com/repos/opensim-org/opensim-core/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/opensim-org/opensim-core/teams","hooks_url":"https://api.github.com/repos/opensim-org/opensim-core/hooks","issue_events_url":"https://api.github.com/repos/opensim-org/opensim-core/issues/events{/number}","events_url":"https://api.github.com/repos/opensim-org/opensim-core/events","assignees_url":"https://api.github.com/repos/opensim-org/opensim-core/assignees{/user}","branches_url":"https://api.github.com/repos/opensim-org/opensim-core/branches{/branch}","tags_url":"https://api.github.com/repos/opensim-org/opensim-core/tags","blobs_url":"https://api.github.com/repos/opensim-org/opensim-core/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/opensim-org/opensim-core/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/opensim-org/opensim-core/git/refs{/sha}","trees_url":"https://api.github.com/repos/opensim-org/opensim-core/git/trees{/sha}","statuses_url":"https://api.github.com/repos/opensim-org/opensim-core/statuses/{sha}","languages_url":"https://api.github.com/repos/opensim-org/opensim-core/languages","stargazers_url":"https://api.github.com/repos/opensim-org/opensim-core/stargazers","contributors_url":"https://api.github.com/repos/opensim-org/opensim-core/contributors","subscribers_url":"https://api.github.com/repos/opensim-org/opensim-core/subscribers","subscription_url":"https://api.github.com/repos/opensim-org/opensim-core/subscription","commits_url":"https://api.github.com/repos/opensim-org/opensim-core/commits{/sha}","git_commits_url":"https://api.github.com/repos/opensim-org/opensim-core/git/commits{/sha}","comments_url":"https://api.github.com/repos/opensim-org/opensim-core/comments{/number}","issue_comment_url":"https://api.github.com/repos/opensim-org/opensim-core/issues/comments{/number}","contents_url":"https://api.github.com/repos/opensim-org/opensim-core/contents/{+path}","compare_url":"https://api.github.com/repos/opensim-org/opensim-core/compare/{base}...{head}","merges_url":"https://api.github.com/repos/opensim-org/opensim-core/merges","archive_url":"https://api.github.com/repos/opensim-org/opensim-core/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/opensim-org/opensim-core/downloads","issues_url":"https://api.github.com/repos/opensim-org/opensim-core/issues{/number}","pulls_url":"https://api.github.com/repos/opensim-org/opensim-core/pulls{/number}","milestones_url":"https://api.github.com/repos/opensim-org/opensim-core/milestones{/number}","notifications_url":"https://api.github.com/repos/opensim-org/opensim-core/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/opensim-org/opensim-core/labels{/name}","releases_url":"https://api.github.com/repos/opensim-org/opensim-core/releases{/id}","created_at":"2014-06-12T16:57:56Z","updated_at":"2015-08-28T09:57:36Z","pushed_at":"2015-09-10T06:15:07Z","git_url":"git://github.com/opensim-org/opensim-core.git","ssh_url":"git@github.com:opensim-org/opensim-core.git","clone_url":"https://github.com/opensim-org/opensim-core.git","svn_url":"https://github.com/opensim-org/opensim-core","homepage":"https://opensim.stanford.edu","size":114987,"stargazers_count":17,"watchers_count":17,"language":"C++","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":16,"mirror_url":null,"open_issues_count":237,"forks":16,"open_issues":237,"watchers":17,"default_branch":"master"}},"base":{"label":"opensim-org:master","ref":"master","sha":"613516248da33a04ce195dfea8f1e53b900724b9","user":{"login":"opensim-org","id":4370782,"avatar_url":"https://avatars.githubusercontent.com/u/4370782?v=3","gravatar_id":"","url":"https://api.github.com/users/opensim-org","html_url":"https://github.com/opensim-org","followers_url":"https://api.github.com/users/opensim-org/followers","following_url":"https://api.github.com/users/opensim-org/following{/other_user}","gists_url":"https://api.github.com/users/opensim-org/gists{/gist_id}","starred_url":"https://api.github.com/users/opensim-org/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/opensim-org/subscriptions","organizations_url":"https://api.github.com/users/opensim-org/orgs","repos_url":"https://api.github.com/users/opensim-org/repos","events_url":"https://api.github.com/users/opensim-org/events{/privacy}","received_events_url":"https://api.github.com/users/opensim-org/received_events","type":"Organization","site_admin":false},"repo":{"id":20775600,"name":"opensim-core","full_name":"opensim-org/opensim-core","owner":{"login":"opensim-org","id":4370782,"avatar_url":"https://avatars.githubusercontent.com/u/4370782?v=3","gravatar_id":"","url":"https://api.github.com/users/opensim-org","html_url":"https://github.com/opensim-org","followers_url":"https://api.github.com/users/opensim-org/followers","following_url":"https://api.github.com/users/opensim-org/following{/other_user}","gists_url":"https://api.github.com/users/opensim-org/gists{/gist_id}","starred_url":"https://api.github.com/users/opensim-org/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/opensim-org/subscriptions","organizations_url":"https://api.github.com/users/opensim-org/orgs","repos_url":"https://api.github.com/users/opensim-org/repos","events_url":"https://api.github.com/users/opensim-org/events{/privacy}","received_events_url":"https://api.github.com/users/opensim-org/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/opensim-org/opensim-core","description":"SimTK OpenSim 4.0 C++ libraries and command-line applications, and Java/Python wrapping.","fork":false,"url":"https://api.github.com/repos/opensim-org/opensim-core","forks_url":"https://api.github.com/repos/opensim-org/opensim-core/forks","keys_url":"https://api.github.com/repos/opensim-org/opensim-core/keys{/key_id}","collaborators_url":"https://api.github.com/repos/opensim-org/opensim-core/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/opensim-org/opensim-core/teams","hooks_url":"https://api.github.com/repos/opensim-org/opensim-core/hooks","issue_events_url":"https://api.github.com/repos/opensim-org/opensim-core/issues/events{/number}","events_url":"https://api.github.com/repos/opensim-org/opensim-core/events","assignees_url":"https://api.github.com/repos/opensim-org/opensim-core/assignees{/user}","branches_url":"https://api.github.com/repos/opensim-org/opensim-core/branches{/branch}","tags_url":"https://api.github.com/repos/opensim-org/opensim-core/tags","blobs_url":"https://api.github.com/repos/opensim-org/opensim-core/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/opensim-org/opensim-core/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/opensim-org/opensim-core/git/refs{/sha}","trees_url":"https://api.github.com/repos/opensim-org/opensim-core/git/trees{/sha}","statuses_url":"https://api.github.com/repos/opensim-org/opensim-core/statuses/{sha}","languages_url":"https://api.github.com/repos/opensim-org/opensim-core/languages","stargazers_url":"https://api.github.com/repos/opensim-org/opensim-core/stargazers","contributors_url":"https://api.github.com/repos/opensim-org/opensim-core/contributors","subscribers_url":"https://api.github.com/repos/opensim-org/opensim-core/subscribers","subscription_url":"https://api.github.com/repos/opensim-org/opensim-core/subscription","commits_url":"https://api.github.com/repos/opensim-org/opensim-core/commits{/sha}","git_commits_url":"https://api.github.com/repos/opensim-org/opensim-core/git/commits{/sha}","comments_url":"https://api.github.com/repos/opensim-org/opensim-core/comments{/number}","issue_comment_url":"https://api.github.com/repos/opensim-org/opensim-core/issues/comments{/number}","contents_url":"https://api.github.com/repos/opensim-org/opensim-core/contents/{+path}","compare_url":"https://api.github.com/repos/opensim-org/opensim-core/compare/{base}...{head}","merges_url":"https://api.github.com/repos/opensim-org/opensim-core/merges","archive_url":"https://api.github.com/repos/opensim-org/opensim-core/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/opensim-org/opensim-core/downloads","issues_url":"https://api.github.com/repos/opensim-org/opensim-core/issues{/number}","pulls_url":"https://api.github.com/repos/opensim-org/opensim-core/pulls{/number}","milestones_url":"https://api.github.com/repos/opensim-org/opensim-core/milestones{/number}","notifications_url":"https://api.github.com/repos/opensim-org/opensim-core/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/opensim-org/opensim-core/labels{/name}","releases_url":"https://api.github.com/repos/opensim-org/opensim-core/releases{/id}","created_at":"2014-06-12T16:57:56Z","updated_at":"2015-08-28T09:57:36Z","pushed_at":"2015-09-10T06:15:07Z","git_url":"git://github.com/opensim-org/opensim-core.git","ssh_url":"git@github.com:opensim-org/opensim-core.git","clone_url":"https://github.com/opensim-org/opensim-core.git","svn_url":"https://github.com/opensim-org/opensim-core","homepage":"https://opensim.stanford.edu","size":114987,"stargazers_count":17,"watchers_count":17,"language":"C++","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":16,"mirror_url":null,"open_issues_count":237,"forks":16,"open_issues":237,"watchers":17,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/opensim-org/opensim-core/pulls/589"},"html":{"href":"https://github.com/opensim-org/opensim-core/pull/589"},"issue":{"href":"https://api.github.com/repos/opensim-org/opensim-core/issues/589"},"comments":{"href":"https://api.github.com/repos/opensim-org/opensim-core/issues/589/comments"},"review_comments":{"href":"https://api.github.com/repos/opensim-org/opensim-core/pulls/589/comments"},"review_comment":{"href":"https://api.github.com/repos/opensim-org/opensim-core/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/opensim-org/opensim-core/pulls/589/commits"},"statuses":{"href":"https://api.github.com/repos/opensim-org/opensim-core/statuses/96c67fbf69cc1a5d0decc422afad123d85100cbb"}}}}