Skip to content
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.

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

Beefup markersreference testcase #3230

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aymanhab
Copy link
Member

@aymanhab aymanhab commented Jun 20, 2022

Fixes issue #3226

Brief summary of changes

Exercise workflow in issue 3226

Testing I've completed

Looking for feedback on...

CHANGELOG.md (choose one)

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

This change is Reviewable

@aymanhab
Copy link
Member Author

@cvhammond this PR exercises the code you have in #3226, I will not merge it since it tests nothing now but it doesn't crash. If you agree, this points to memory management issues/early garbage collection in Matlab rather than an actual API bug, I will propose ideas/suggestions if that proves to be the case.

@cvhammond
Copy link
Member

@aymanhab that appears to be the correct C++ implementation of the MATLAB test case that I posted. Looking forward to advice on adjustments to deal with MATLAB's memory management

@aymanhab
Copy link
Member Author

Some ideas for troubleshooting:

  1. print the version (opensimCommonJNI.GetVersionAndDate()) to make sure the wiring is to the latest build of 4.4.
  2. Create objects and keep references to them in variables rather than on the stack e.g. SimTKArrayCoordinateReference(), otherwise they could be immediately garbage collected.
  3. Where does the crash happen, if in a loop is it at a specific index?
  4. If there's a stacktrace that would be extremely helpful to pinpoint the issue source.
  5. I understand this is a simplified version, which is best but I don't know if it actually crashes for you or if the crash happens only in more complicated setting where the three sets (Model-makers vs. weights vs. markers-in-trc-file) are not identical.
  6. I don't have a working Matlab environment but can get that setup over the next day or two. In that case I'd like to know your Matlab version.

Let me know what you find out or if you get more clues on what's going wrong.

@cvhammond
Copy link
Member

cvhammond commented Jun 21, 2022

I believe I have found the issue and fixed it. It was a combination of the MarkersReference object requiring a specific signature and the MarkerReference's MarkerTable returning double sometimes and java.lang.double other times for MarkerTable.getNearestRowIndexForTime(). The suggestion to make sure I was using the HEAD (opensimCommonJNI.GetVersionAndDate()) was important as well. I'm learning more about interacting with the C++ API than I originally thought I would.

All of my tests now pass as they did on 4.0.

@aymanhab
Copy link
Member Author

Outstanding! thanks for the update. The issue with double vs. java.lang.double was reported by another user earlier and we couldn't reproduce. If you can provide a minimal example that would be awesome. Thanks for your help 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants