Test common GUI workflows

scrum2a commented 5 years ago

We have made a bunch of changes to core and GUI since the last time we did user testing of the GUI. We should use the GUI artifact for the sets change. Download it by clicking "Details" next to "appveyor/pr" here: https://github.com/opensim-org/opensim-gui/pull/1005

[x] tutorials (5 pts, Carmichael successfully tested Tug of War with OpenSim 4.0-2018-10-01-76939822)
[x] tools that use ExternalLoads (5 pts)
[x] MarkerSets (loading/saving markers from files) (5 pts Ayman)
[x] loading RRA adjusted model crashes due to lingering ExternalLoads in model's ComponentSet (#1005) subject01_simbody_adjusted.osim
[x] Creating an ExternalLoads from scratch in the GUI (8 pts) (created ExternalLoads for inverse dynamics with OpenSim 4.0-2018-10-01-76939822)
[x] Double clicking Bodies, Muscles, ... nodes throws exception (likely because sets created on the fly for GUI use are not wired correctly to underlying model (they don't own their objects either)
[x] MarkerSet constructor from file still has wrong signature, doesn't throw if it fails.
[x] Deleting Markers does not work because it deletes from set not component tree

aymanhab commented 5 years ago

Trying to load leg6dof9musc model included with build, got message about knee_angle_pat_r wrong coordinate type. Not sure if this is specific to this build or master version of model is out of date as well.

aymanhab commented 5 years ago

@aseth1 @jimmyDunne I'm looking into MarkerSet issues now

aseth1 commented 5 years ago

Trying to load leg6dof9musc model included with build, got message about knee_angle_pat_r wrong coordinate type. Not sure if this is specific to this build or master version of model is out of date as well.

leg6dof9musc from models repo loads without error, but the one distributed with the build does. Seems specific to the build.

aymanhab commented 5 years ago

Saving MarkerSet to file works fine (though paths makes no sense on their own, not new ) but loading saved markers fails and throws exception

aseth1 commented 5 years ago

Double clicking Bodies, Muscles, ... nodes throws exception (likely because sets created on the fly for GUI use are not wired correctly to underlying model (they don't own their objects either)

I would not use ModelComponentSets for the display purposes in the GUI. If you just need a collection use Sets. ModelComponentsSet is an integral part of a Model not just a bag of pointers.

aseth1 commented 5 years ago

Previewing marker data fails. Seems to be the same failure for BodySet, ForceSet, etc... likely related to MarkerSet:

    at org.opensim.modeling.opensimSimulationJNI.ModelComponent_getModel(Native Method)
    at org.opensim.modeling.ModelComponent.getModel(ModelComponent.java:65)
    at org.opensim.view.SelectedObject.getOwnerModel(SelectedObject.java:68)
    at org.opensim.view.pub.ViewDB.markSelected(ViewDB.java:619)
    at org.opensim.view.pub.ViewDB.clearSelectedObjects(ViewDB.java:735)
    at org.opensim.view.pub.ViewDB.update(ViewDB.java:354)
    at java.util.Observable.notifyObservers(Observable.java:159)
    at org.opensim.view.pub.OpenSimDB.setCurrentModel(OpenSimDB.java:255)
    at org.opensim.view.pub.OpenSimDB.addModel(OpenSimDB.java:154)
    at org.opensim.view.pub.OpenSimDB.addModel(OpenSimDB.java:141)
    at org.opensim.view.motions.FileLoadDataAction.performAction(FileLoadDataAction.java:67)
    at org.openide.util.actions.CallableSystemAction$1.run(CallableSystemAction.java:130)
    at org.openide.util.actions.ActionInvoker$1.run(ActionInvoker.java:95)
    at org.openide.util.actions.ActionInvoker.doPerformAction(ActionInvoker.java:116)
    at org.openide.util.actions.ActionInvoker.invokeAction(ActionInvoker.java:99)
    at org.openide.util.actions.CallableSystemAction.actionPerformed(CallableSystemAction.java:127)
    at javax.swing.AbstractButton.fireActionPerformed(AbstractButton.java:2022)
    at javax.swing.AbstractButton$Handler.actionPerformed(AbstractButton.java:2348)
    at javax.swing.DefaultButtonModel.fireActionPerformed(DefaultButtonModel.java:402)
    at javax.swing.DefaultButtonModel.setPressed(DefaultButtonModel.java:259)
    at javax.swing.AbstractButton.doClick(AbstractButton.java:376)
    at javax.swing.plaf.basic.BasicMenuItemUI.doClick(BasicMenuItemUI.java:842)
    at javax.swing.plaf.basic.BasicMenuItemUI$Handler.mouseReleased(BasicMenuItemUI.java:886)
    at java.awt.Component.processMouseEvent(Component.java:6533)
    at javax.swing.JComponent.processMouseEvent(JComponent.java:3324)
    at java.awt.Component.processEvent(Component.java:6298)
    at java.awt.Container.processEvent(Container.java:2237)
    at java.awt.Component.dispatchEventImpl(Component.java:4889)
    at java.awt.Container.dispatchEventImpl(Container.java:2295)
    at java.awt.Component.dispatchEvent(Component.java:4711)
    at java.awt.LightweightDispatcher.retargetMouseEvent(Container.java:4889)
    at java.awt.LightweightDispatcher.processMouseEvent(Container.java:4526)
    at java.awt.LightweightDispatcher.dispatchEvent(Container.java:4467)
    at java.awt.Container.dispatchEventImpl(Container.java:2281)
    at java.awt.Window.dispatchEventImpl(Window.java:2746)
    at java.awt.Component.dispatchEvent(Component.java:4711)
    at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:760)
    at java.awt.EventQueue.access$500(EventQueue.java:97)
    at java.awt.EventQueue$3.run(EventQueue.java:709)
    at java.awt.EventQueue$3.run(EventQueue.java:703)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:80)
    at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:90)
    at java.awt.EventQueue$4.run(EventQueue.java:733)
    at java.awt.EventQueue$4.run(EventQueue.java:731)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:80)
    at java.awt.EventQueue.dispatchEvent(EventQueue.java:730)
    at org.netbeans.core.TimableEventQueue.dispatchEvent(TimableEventQueue.java:159)
[catch] at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:205)
    at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:116)
    at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:105)
    at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
    at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:93)
    at java.awt.EventDispatchThread.run(EventDispatchThread.java:82)

But oddly, associate motion data (e.g. trc file) works.

aymanhab commented 5 years ago

I would not use ModelComponentSets for the display purposes in the GUI. If you just need a collection use Sets. ModelComponentsSet is an integral part of a Model not just a bag of pointers.

Thanks @aseth1 I tried calling connectToModel on these sets and that seemed to resolve the problem, what would be the downside? Remember that templates do not pass through SWIG so if we want a different typed container we'll have to instantiate it for every type or get a generic container then safedowncast downstream.

aseth1 commented 5 years ago

Thanks @aseth1 I tried calling connectToModel on these sets and that seemed to resolve the problem, what would be the downside?

Ownership. ModelComponentSets are supposed to own their components and itself belong to a Model.
Traversal. Should getComponentList traverse these Sets? As a ModelComponent its components are included in the tree traversal.
Contribution to the System. Components of a ModelComponentSet have their component interface automatically invoked and it seems you could have addToSystem invoked twice if the same component exists in two lists. This is likely to throw before then.

Remember that templates do not pass through SWIG so if we want a different typed container we'll have to instantiate it for every type or get a generic container safedowncast downstream.

Its seems straightforward to do that, but do you even need a C++ Set for this? Could you not contain Component lists in Java containers?

aymanhab commented 5 years ago

@aseth1 The nodes in the Navigator are all backed exclusively by OpenSimObject so a generic Java container will not do the trick without major rework. Before this change all Sets and Groups derived from OpenSim::Object and changing this now would be quite disruptive.

aseth1 commented 5 years ago

I see. Thanks for clarifying. I think it is OK to SWIG wrap the specific Set instances that you need.

aymanhab commented 5 years ago

Looking further into the MarkerSet issues:

Removal from the Set uses the generic Set operation (since there's no ModelComponentSet remove), resulting in removal being broken, downstream effects of trying to load Marker with same name fails.

aymanhab commented 5 years ago

Trying to make PathPointSet into a plain Set to avoid extra 'pathpointset' on every pathpoint's path 😄 which leads to longer path and extra level of ../ still in progress.

scrum2a commented 5 years ago

@carmichaelong Muscle Tug of War

aymanhab commented 5 years ago

The change to PathPointSet appears to be working (since ownership was restored to previous state). Change was committed to branch.

aymanhab commented 5 years ago

MarkerSet issue exposes a bigger hole that's outside the scope of this issue/PR. Sets have built in methods to add/remove which are accessible as properties but the changes don't propagate to the component-tree. The GUI uses the sets exclusively to access Markers, same for the tools so changes to component-tree is irrelevant. I strongly recommend we document the caveat rather than try to significant changes at this point.

carmichaelong commented 5 years ago

Finished muscle tug of war successfully. Added note in the first post that it's completed.

jenhicks commented 5 years ago

@jimmyDunne Do you have updates on tutorials testing? Which ones are you testing?

@aseth1 Have you tested the tutorials? Any updates to report?

aseth1 commented 5 years ago

I tested creating and using ExternalLoads with the Tools and ran into only one issue that I reported in #1005:

[ ] crash due to RRA adjusted model containing ExternalLoads in ComponentSet. subject01_simbody_adjusted.osim.txt

jimmyDunne commented 5 years ago

I have tested all the introductory examples and they seem to be working. There are some minor issues, but they are things that have either previously existed (Choppy motion from dragging the motion scroller) or don't hinder the example (Messages that MuscleAnalysis() is unable to evaluate muscle dynamics when plotting muscle forces or generated moments)

Tutorial SImulation-Based Design to Prevent Ankle Injuries — When opening the Control Functions tree view, the boxes are unclickable and I cannot change any values, nor even view them.

Tutorial SImulation-Based Design for Metabolic cost — Model is missing bones. Download package needs to be updated or example files should be moved to models distribution.

Added probe to model using script. Unable to Enable metabolics probe, either through the navigation window or the properties window.

Dynamic Walker Challenge

/Code/Matlab/Dynamic_Walking_Challenge/ doesnt exist in the package.

aymanhab commented 5 years ago

@jimmyDunne can you attach or link to the file that you have problem editing so we can try in different builds? Thanks

jimmyDunne commented 5 years ago

I have gone through the examples and am now moving back to working on the master branch. Thanks.

scrum2a commented 5 years ago

@aseth1 can you make separate issues?

aymanhab commented 5 years ago

Deletion of Markers should be fixed due to PR #1023

opensim-org / opensim-gui

Test common GUI workflows #1008