Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 22, 2018
Merged

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

merged 4 commits into from
Feb 22, 2018

Conversation

chrisdembia
Copy link
Member

Fixes #2082

Brief summary of changes

Testing I've completed

  • Added a test case that failed without this PR. With the bug fix, the tests now pass.

Looking for feedback on...

  • Is the use of ComponentPath okay?

CHANGELOG.md (choose one)

  • no need to update because...this bug was introduced after 3.3.

@chrisdembia chrisdembia mentioned this pull request Feb 10, 2018
Merged
tkuchida
tkuchida reviewed Feb 10, 2018
View reviewed changes
Copy link
Member

@tkuchida tkuchida left a comment

Choose a reason for hiding this comment

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

One suggestion; otherwise, LGTM 👍

OpenSim/Common/Test/testComponentInterface.cpp Outdated
SimTK_TEST(b->getStateVariableValue(s, "../subState") == 20);
SimTK_TEST(b->getStateVariableValue(s, "../../internalSub/subState") == 10);

top.getStateVariableValue(s, "a/b/subState");
Copy link
Member

Choose a reason for hiding this comment

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

I don't think L1079 is necessary (see L1072).

@chrisdembia
Copy link
Member Author

Thanks @tkuchida

aseth1
aseth1 requested changes Feb 12, 2018
View reviewed changes
Copy link
Member

@aseth1 aseth1 left a comment

Choose a reason for hiding this comment

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

I like that the method now traverses directly to the Component (made possible by your changes to include the path in updateFromXMLNode in #1751).

Now, I think we should rename findStateVariable( name ) to traverseToStateVariable( pathName ) since there is no more "finding" that occurs.

@chrisdembia
Copy link
Member Author

@aseth1 thanks for your review. I renamed the function.

(made possible by your changes to include the path in updateFromXMLNode in #1751).

I'm not sure this is true. findStateVariable() could only be used after finalizeFromProperties(), during which I think all the socket paths are corrected (even without #1751), because of the logic here:

opensim-core/OpenSim/Common/Component.h

Lines 3004 to 3011 in 8d6db79

catch (const ComponentNotFoundOnSpecifiedPath& ex) {
if (Object::getDebugLevel() > 0) {
// TODO once we fix how connections are established when building
// models programmatically, we should show this warning even for
// debug level 0.
std::cout << ex.getMessage() << std::endl;
}
comp = root.template findComponent<C>(path.toString());

Ready for review again.

aseth1
aseth1 approved these changes Feb 13, 2018
View reviewed changes
Copy link
Member

@aseth1 aseth1 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

OpenSim/Common/Component.h
* branch of the Component tree (in such a case, the specified path might
* begin with "../").
* This returns nullptr if a StateVariable does not exist at the specified
* path or if the path is invalid.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@aseth1
Copy link
Member

aseth1 commented Feb 13, 2018

@chrisdembia the travis push build failure looks like a legit test failure.

@chrisdembia
Copy link
Member Author

Ready for review again.

@chrisdembia
Copy link
Member Author

This PR has 2 reviews and passes CI. Would either reviewer (@tkuchida or @aseth1) be interested in merging this?

@tkuchida
Copy link
Member

Re-reviewed and looks 👍; merging.

@tkuchida tkuchida merged commit 69c25d0 into master Feb 22, 2018
@tkuchida tkuchida deleted the setStateVariableValue_invalidpath branch February 22, 2018 21:09
@chrisdembia
Copy link
Member Author

Thanks @tkuchida

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkuchida tkuchida tkuchida approved these changes

@aseth1 aseth1 aseth1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Component::getStateVariableValue() silently accepts incorrect paths
3 participants
@chrisdembia @aseth1 @tkuchida