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

Update gui scripts #908

Merged
merged 23 commits into from
Aug 31, 2018
Merged

Update gui scripts #908

merged 23 commits into from
Aug 31, 2018

Conversation

jimmyDunne
Copy link
Member

Fixes issue #736

Brief summary of changes

Change getInstallDir() -> getResourcesDir()

Testing I've completed

Tested each script locally

CHANGELOG.md (choose one)

@aymanhab
Copy link
Member

When this is done we need to make sure we upgrade "ResourceVersion" so that installer prompts to installResources. @chrisdembia do you think this can be automated using CMake/Ant?

@aymanhab
Copy link
Member

aymanhab commented Aug 2, 2018

@jimmyDunne the fix to browse for folder has been on merged a few days back, please test and close/move as needed. Just making sure it's not blocking your progress on GUI scripts. Thank you.

@chrisdembia
Copy link
Member

When this is done we need to make sure we upgrade "ResourceVersion" so that installer prompts to installResources. @chrisdembia do you think this can be automated using CMake/Ant?

As discussed elsewhere, I think we should just always prompt to install resources.

@jimmyDunne
Copy link
Member Author

@ayman, folder picker seems to be working on Mac— thanks!

All scripts have been tested and are operational.

@jenhicks
Copy link
Member

jenhicks commented Aug 8, 2018

@jimmyDunne What are we waiting for on this one? Someone to verify on Windows?

@jimmyDunne
Copy link
Member Author

Yes, someone to test on Windows.

@chrisdembia chrisdembia self-assigned this Aug 9, 2018
@chrisdembia
Copy link
Member

I started verifying on Windows. Running runTutorialTwo.py causes the GUI to crash after some amount of time, likely during garbage collection.

@aseth1
Copy link
Member

aseth1 commented Aug 11, 2018

Here are the issues I ran into:

  1. install resources did not update my OpenSimResourcesDir. I had to update that manually to the folder that was created by installResources()
  2. runTutorialTwo crashed consistently after plotting.
  3. runTutorialThree runs, but is unknown where the ID results are written to.
  4. alterTendonSlackLength.py crashes the GUI
    hs_err_pid3880.log
  5. plotMuscleFiberLengthAgainstFile.py did not plot. Could not find "subject01_walk1_ik.mot". Comments says it loads BothLegs.osim but in fact loads gait2392_simbody.osim. Cannot run IK on the model, because it has not markers, either.
  6. runMultipleIKTrials.py load leg39 without bones. There is no IK motion files loaded, but scripting window confirms IK was run on two ,trc files
  7. strengthenModel.py creates strengthened model, but does not load it. Have to go to folder to see that a new model was created.
  8. testShowModelSummary.py works but window is tiny an appears in top right corner of screen no where near the application window in my case.

@aseth1
Copy link
Member

aseth1 commented Aug 13, 2018

It might be good to break this issue for the sprint so that each script is its own fix and verification task.

@nickbianco
Copy link
Member

@aseth1 I've run alterTendonSlackLength.py on three different models without the GUI crashing. Do you remember which model you tried it on?

@aymanhab
Copy link
Member

It would be good when testing these scripts to check first if the resources directory has been set. You can tell what the GUI expects by typing getResourcesDir() in the scripting shell.

@nickbianco
Copy link
Member

nickbianco commented Aug 14, 2018

Test report for plotMuscleFiberLengthAgainstFile.py

I was able to run the script and get a plot. I then get the dialog box below after the plot is created. Also, under the directory <OpenSimResourcesDir>/Code/GUI/testData, instead of one file name cvs_export.sto, there are two additional files cvs_export_1.sto and cvs_export.s_2to (yes I typed that last file correctly, something weird is happening for that file only).

image

Artifact: OpenSim 3762598-2018-08-14
Windows 10

@nickbianco
Copy link
Member

Test report for runTutorialTwo.py.

The first time I ran this script, the GUI crashed like @aseth1 mentioned. However, after rerunning the script multiple times, including after re-installing the build, it hasn't crashed for me again. It seems to be working properly, all the plots and models are generated according to the script, so I'm not sure what happened that first time.

Artifact: OpenSim 3762598-2018-08-14
Windows 10

@aymanhab
Copy link
Member

aymanhab commented Aug 15, 2018

@nickbianco do bones visualize if drag/drop the model file in GUI but not thru scripts? They are supposed to use the same code, and it's possibly a model version/update issue (was it saved in 3.3 or in an earlier version)?

Crash logs are usually written to install folder but if the crash happens in native/api side while doing directory change maneuvers maybe found in whatever folder is assumed current by the API at that point (e.g. with model or script or setup file)

@aymanhab
Copy link
Member

Just a headsup, calling installResources() from the shell as of now does NOT set the resources directory, so calls to getResourcesDir() will still return the value set by whatever last installer that installed these resources. I'll submit a PR to fix that but in the meantime please make sure to type getResourcesDir() in the scripting shell, and if you get unexpected result let me know and I'll suggest a workaround. This issue only affects Windows users where preferences are persistant but not OSX users if they clear preferences on fresh install.

@chrisdembia
Copy link
Member

Just a headsup, calling installResources() from the shell as of now does NOT set the resources directory, so calls to getResourcesDir() will still return the value set by whatever last installer that installed these resources. I'll submit a PR to fix that but in the meantime please make sure to type getResourcesDir() in the scripting shell, and if you get unexpected result let me know and I'll suggest a workaround. This issue only affects Windows users where preferences are persistant but not OSX users if they clear preferences on fresh install.

@aymanhab I just had this issue on Mac where getResourcesDir() returns None and there is no preference for the resources dir. I think you fixed this in #958 (merged), so I've merged master into this PR.

@aseth1
Copy link
Member

aseth1 commented Aug 17, 2018

I did some digging and the leg39.osim does not appear to be a legitimate 20303 version model file but a hand-modified version. It appears it was introduced as part of a series of changes to SWIG wrapping for the GUI on Dec-19-2014:

SHA-1: dd75f64706e82623f6e7b2d2205a4e6aac54bece

* Add current version of SWIG output to track changes, eventually these will be removed and jar file copied from opensim-core build when the process is streamlined.

This file should be removed and have the runMultipleIKFiles.py script use a model from the "/Models/" or replaced by a leg39.osim file generated by a version of OpenSim, either 2.4 or 3.3.

@aymanhab
Copy link
Member

Please merge branch https://github.com/opensim-org/opensim-gui/tree/fix_plot_exportdata for export issues fix

@nickbianco
Copy link
Member

nickbianco commented Aug 21, 2018

@jimmyDunne I ran your runTutorialTwo.py fix a handful of times on Windows without the GUI crashing. To be fair, it only crashed on me once previously, so not sure if it's worth another test on Windows by someone else (probably @aseth1 since you had the most trouble with this).

Artifact: OpenSim f1e93ab-2018-08-21 (AppVeyor build 1.0.1741)
Windows 10

@nickbianco
Copy link
Member

nickbianco commented Aug 21, 2018

Test report for testShowModelSummary.py.

I get nearly the same result as reported by @aseth1: the window is tiny and appears on a separate screen from the application window. However, mine appeared in the top left.

Artifact: OpenSim f1e93ab-2018-08-21 (AppVeyor build 1.0.1741)
Windows 10

Copy link
Member

@chrisdembia chrisdembia left a comment

Choose a reason for hiding this comment

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

@jimmyDunne I left some comments related to runTutorialTwo.py.

ECUPostpathPoint = myModel.getMuscles().get("ECU_post-surgery").getGeometryPath().getPathPointSet().get(i)
# Replace path point from ECU_pre-surgery to ECU_post-surgery
myModel.getMuscles().get("ECU_pre-surgery").getGeometryPath().replacePathPoint(myState,ECUPrepathPoint,ECUPostpathPoint)
myModel.getMuscles().get("ECU_pre-surgery").getGeometryPath().updPathPointSet().clearAndDestroy();
Copy link
Member

Choose a reason for hiding this comment

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

No need for ; in python/jython.

myModel.getMuscles().get("ECU_pre-surgery").getGeometryPath().updPathPointSet().clearAndDestroy();
pps = myModel.getMuscles().get("ECU_pre-surgery").getGeometryPath().updPathPointSet();
for i in range(myModel.getMuscles().get("ECU_post-surgery").getGeometryPath().getPathPointSet().getSize()):
pps.cloneAndAppend(myModel.getMuscles().get("ECU_post-surgery").getGeometryPath().getPathPointSet().get(i));
Copy link
Member

Choose a reason for hiding this comment

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

It might be nice to use a local variable for the ECU_post-surgery PathPointSet so that these lines are shorter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, Chris. I incorporated your suggestions

Copy link
Member

Choose a reason for hiding this comment

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

Looks good.

@jimmyDunne
Copy link
Member Author

Test report for plotMuscleFiberLengthAgainstFile.py

I was able to run the script and get a plot. I then get the dialog box below after the plot is created. Also, under the directory /Code/GUI/testData, instead of one file name cvs_export.sto, there are two additional files cvs_export_1.sto and cvs_export.s_2to (yes I typed that last file correctly, something weird is happening for that file only).

This seems to be an issue on the GUI side, here. @aymanhab, I am not particularly sure how to approach this one— Is there a way of just plotting what is in the plotter?

@aymanhab
Copy link
Member

@jimmyDunne I had a fix for that on a branch "fix_plot_exportdata" that I thought you merged into your environment earlier. I went ahead and merged it to master so if you merge from master you should be good. Let me know how it goes.

@aymanhab
Copy link
Member

@jimmyDunne this branch is way behind master and that may explain some issues that you're running into. Please let me know if it's ok to merge latest master and retry so you're not hitting issues that we resolved already.

@aymanhab
Copy link
Member

I merged master into this branch, if anybody is working on these scripts please "pull" to avoid downstream issues.

@jimmyDunne
Copy link
Member Author

@aymanhab, would it be possible for you to pull in the latest version of opensim-models? There is a leg39 model that should now be included in the distribution (which is relevant to the GUI scripts). If there is something I can do, please let me know?

@jimmyDunne
Copy link
Member Author

jimmyDunne commented Aug 30, 2018

@jimmyDunne this branch is way behind master and that may explain some issues that you're running into. Please let me know if it's ok to merge latest master and retry so you're not hitting issues that we resolved already.

@aymanhab I am using the latest version from sourceforge (4.0-2018-08-30-a33ffc9) and I am getting this same issue. Running plotMuscleFiberLengthAgainstFile.py results in multiple outputs and a warning that 'Exported curves have different domains. Creating 2 files'. Has the 'fix_plot_exportdata' been incorporated into master? If it is only in this branch then I can't test it because I don't (can't) build the GUI locally.

@aymanhab
Copy link
Member

aymanhab commented Aug 30, 2018

@jimmyDunne the fix changes the three files, one with bad name, into 2 files, if you remove the Constant function which is only added to show off rather than for useful reason then you should get one file. Please let me know if that works for you.

@aymanhab
Copy link
Member

@jimmyDunne I updated the models repo and remerged, you should be good to go with latest fixes and models

@jimmyDunne
Copy link
Member Author

the fix changes the three files, one with bad name, into 2 files,

Cool, I can see this behavior. 🎉

@jimmyDunne jimmyDunne merged commit 0edd5bd into master Aug 31, 2018
@jimmyDunne jimmyDunne changed the title {WIP} Update gui scripts Update gui scripts Aug 31, 2018
@jimmyDunne jimmyDunne deleted the update_gui_scripts branch August 31, 2018 14:25
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.

6 participants