Bug Fix in the Component API

fcanderson commented 3 weeks ago

Fixes issue #3803

This PR addresses an assertion failure that was occurring in Debug mode and fixes bugs in Component::setModelingOption() and Component::getModelingOption().

In Debug mode, an assertion fails upon initialization of ssIndex, dvIndex, and moIndex (lines 4091, 4094, 4180, 4183 in Component.h). Originally, these indexes were initialized to -1, much as one would initialize a pointer to null. However, an assertion in SimTK::State fails (only in Debug mode) and interrupts execution. As an alternative solution, initializing these indexes to an unreasonably large number would avoid an assertion failure. However, upon digging deeper, I discovered that initialization is altogether unnecessary. Simbody initializes these indices under the covers at the moment of creation.
The wrong _namedModelingOptionInfo member variable was being consulted. The owner's map should have been consulted, as opposed to the calling Component's map. This bug went unidentified by testComponentInterface.cpp because the map finds were all successful.

Brief summary of changes

ssIndex, dvIndex, and moIndex are now left uninitialized in OpenSim::Component because, under the covers in Simbody, indexes like these are initialized to static const SimTK::InvalidIndex.
Component::_namedModelingOptionInfo of the owner is now being queried instead of the Component on which the get or set was called.
Added a Catch2 test to testComponentInterface.cpp that triggers a VariableNotFound exception, which would have caught this bug even in Release mode.

Testing I've completed

I re-ran testComponentInterface in both Release and Debug modes. All tests are passing.

CHANGELOG.md (choose one)

No need to update because a new feature was not added and because there is no change to the API.

This change is

fcanderson commented 3 weeks ago

@nickbianco and @aymanhab. Do either of you have time to review my bug fixes and merge this PR? There are just a few tweaks to existing code and a few added lines.

Thanks, -Clay

aymanhab commented 3 weeks ago

Thanks @fcanderson I'll try to review today.

fcanderson commented 1 week ago

@aymanhab @nickbianco I'm working to identify the reason for the C-I failure for the "Windows" build. testPrescribedForce is the specific test that is failing.

All tests pass on my desktop for both Visual Studio 2019 and Visual Studio 2022 builds.

My fork/branch is up-to-date with opensim-org:main (i.e., I have synched).
I'm running the same version of Simbody (GIT_TAG d98f05fa1899633b544757e6c179562940f0849d).
I'm running the latest versions of Visual Studio Community 2019 (16.11.37) and Visual Studio Community 2022 (17.10.3).
I'm running on Windows 10. (I do not have a computer running Windows 11).

At this point I'm wondering if the difference in behavior between my desktop and C-I has to do with either the version of Visual Studio or the version of Windows.

In the past I have seen differences in the standard C++ libraries on Windows 10 vs. Windows 11. I had to program around them in Simbody. See https://github.com/simbody/simbody/issues/780.

So, for C-I, I'm specifically wondering:

What version of Windows is running (10 or 11)?
What version of Visual Studio Community 2019 is running?

I'm assuming I need to get to the bottom of this before the bug fixes in this PR are merged? Merger of these bug fixes is a prerequisite for the PR I will submit for the new StatesDocument class.

Thanks. I know you guys are busy.

nickbianco commented 1 week ago

@fcanderson the testPrescribedForce failure is unrelated to your changes here. We've been running into this issue in other PRs and not sure what the root cause is yet.

Sorry if you went down the rabbit hole on this! You can ignore that failure if it continues to persist (although sometimes re-running the CI will get it to pass).

fcanderson commented 1 week ago

@nickbianco @aymanhab

Thanks for the response, Nick. I didn't go too far down the rabbit hole.

Given that the failure is unrelated to my changes, would one of you merge this PR? Or, is a review from another person needed?

aymanhab commented 1 week ago

Merged, thanks @fcanderson

fcanderson commented 1 week ago

@aymanhab @nickbianco Awesome. Thanks, Ayman.

opensim-org / opensim-core