search

opensim-org / opensim-gui

SimTK OpenSim graphical user interface and distribution.
Apache License 2.0
55 stars 30 forks source link

Crashes due to Tools modifying models, their system or state #1044

Open aseth1 opened 5 years ago

aseth1 commented 5 years ago

Steps to reproduce

See #1035 where running runTutorialThree.py causes the GUI to crash because the Tool mangles the model that the GUI has a handle to and its underlying system (or state) is invalid.

Expected result

Running Tools in the scripting shell has not impact on models in the GUI.

Actual result

GUI crashes. (include screen grabs where appropriate)

Environment and GUI version

OpenSim 4.0-2018-10-29-06450ba8 on Windows

aseth1 commented 5 years ago

I have tested a fix where Tool::setModel(model) makes a copy of the model so that the Tool and the GUI do not have a handle to the same model. This seems to work in the API and Scripting just fine, but as @aymanhab mentioned in the scrum today, some Tools like IK can no longer animate since the Analyses the GUI adds are not accessible eventhough IK runs generates and a motion file, no results are presented in the GUI.

I was not able to get similar changes to AbstractTool to pass their tests so I held off the change . I am reluctant to add even more complexity into the mix to now have the option to clone or not in setModel. Instead we should have a well-defined and consistent interface between GUI, Tools and public scripting methods.

Users aren't really going to care for explanations for the crashes so putting out software with known exposure to potential crashes is not a great option. IMHO we should not allow users to have write access to the live model in the GUI. Giving users carte-blanche access to these methods and expecting safe behaviors is risky at best.

There are some sensible steps we can take in the interim:

  1. a. enforce constness of getCurrentModel() (even in Java) so users must clone() to modify. b. make loadModel() clone and initSystem() so the GUI never operates on a model that the user has a writable handle to, and prevent users from forgetting to call initSystem().
  2. forbid Tool::setModel() in GUI Scripting
  3. exclude problematic scripts from upcoming release until we have a robust solution

Some combination of these could be reasonable.

chrisdembia commented 5 years ago

I support taking runTutorialThree.py out of our 4.0 release.

aymanhab commented 5 years ago

I'm ok with removing runTutorialThree.py though my preference would be to fix it/document it particularly if it was included in earlier versions (3.3) and as it sounds per @jimmyDunne it is actually useful, so users can actually do it correctly.

I agree it looks bad indeed when a user can crash the application but scripting is assumed to be for advanced users so we need to weigh the flexibility vs. the potential misuse.

Also important to note that this exact behavior has been there since scripting was introduced to users years ago and was never reported by users as an issue. Thoughts @jenhicks @jimmyDunne

chrisdembia commented 5 years ago

I submitted #1049.

We should not close this issue when #1049 is merged, because #1049 is just a temporary workaround. We can simply move this issue out of the sprint.