Changing motions from one with muscle states to without: color of muscles is carried over

[jenhicks]

This happened for me in the tutorial when I switched from the SO results to IK results

[tkuchida]

Related: Muscles are red when model is first opened. If IK is run on newly opened model, muscles remain red while IK is solving but then (correctly) become blue in playback mode. opensim4.0PreviewWin64_0719.zip

[aymanhab]

@tkuchida While running an analysis or playing back a motion, the muscles are colored by the state, otherwise the colors use their default colors from the file. This seems reasonable and allows for a mode where you can use display options to show various muscle groups etc. IK produces only coordinates so there's no specific color to apply to muscles. From there on we're using states hence the blue color. Does that make sense? Confusing? Open to suggestions.

[tkuchida]

I've never used the "mode where you can use display options to show various muscle groups etc." I think the playback should play back whatever was displayed during the computation. Presumably, the muscles should be blue during IK computation if the muscles are inactive.

[aymanhab]

This was folded into #283 where loading any motion now, we convert into States using formStateStorage. Please verify and close or report unexpected behavior. Thanks

[jimmyDunne]

I opened a model and two motions; one a states file from CMC, and the other is an RRA sto file from RRA. When I switch from the CMC states to the RRA kinematics, whatever the current muscle colors were during the CMC playback are persistent.

[jenhicks]

When you load coordinates, make full state with muscles to blue. @aymanhab will investigate

[aymanhab]

This actually is exactly how Model.formStateStorage() works which is correctly invoked in this case. For unspecifed states (e.g. muscle states) it keeps them at current value. If this is unexpected or a different behavior is expected pleaseopen an issue for opensim-core and when a solution is available at the API level we can wire it from the GUI. @chrisdembia please let me know if this is different from your expectations. Thanks.

[jimmyDunne]

Ayman, it would be great if you could pass on more details on how this feature works. It feels like the rest of the team aren't sure of how this is implemented and it would be awesome to get some further insight. What class/component draws the colors? What class/component holds/computes the data? It sounds like Model.formStateStorage() will compute some data that then gets passed to something that draws it on the screen, correct? When we have multiple loaded motions or states, does Model.formStateStorage() get computed each time you change the current motion or state or does it call it once when the data is loaded then store it somewhere for recall when a user changes the current motion or state? It would be really helpful for us to know the design.

We had discussed that when we switch between a loaded state with muscle activations to a motion with only coordinate values, the muscles should become blue— the recognized color for zero activation. Sorry if that was unclear from the meeting?

If this is an improvement that should be made to OpenSim-core to help the visualizer, it would be most helpful for you to create and write the issue. You are by far the most familiar with the GUI and the communication between the GUI and the API. I would find it daunting to write an issue on the this that captures the nuance of the problem.

[aymanhab]

@jimmyDunne nice writeup ;) My note was mainly intended for @jenhicks and @aseth1 since we had extended discussions in the past regarding display of motions and colors. Let me explain how this works for the benefit of everybody.

• Prior to 4.0, the GUI was deciding when to color muscles and how to color them.
• In 4.0 we agreed to move the functionality to the API side (for consistency and efficiency reasons) and because it is now capable of taking a motion and creating a trajectory of states that the GUI can push to the model without any interference.
• The function that performs the previous step (take motion and produce stateStorage) in the API is Model.formStateStorage() if your preference is for muscles to be colored blue if activations are unspecified then the function should be modified to take an option to either use current muscle states, use default activations or anything else.

Hope this helps.

[tkuchida]

if your preference is for muscles to be colored blue if activations are unspecified

Yes, I think this is essential. From the user's perspective, it would be very confusing for muscles to be colorful when playing a motion that is devoid of activation data (the colors would be conveying information that isn't there). Also, the color of a muscle when playing an animation of IK results, for example, should not depend on the arbitrary color of that muscle when some previously selected motion was paused. I would expect the internal state and external appearance of the visualizer to be identical when playing the same IK result, regardless of what else has been loaded or played previously.

[aymanhab]

I totally agree @tkuchida 👍 you make a very strong case. I just pointed out that the functionality is implemented in a different repository 😄

[tkuchida]

Ah, ok thanks @aymanhab. According to the doxygen for Model::formStateStorage(), it should "Use the default state value if a state is unspecified in the originalStorage." Perhaps this isn't happening when the original Storage is missing activations (e.g., IK results) and the GUI isn't given zeros to turn the muscles blue? I'll open an Issue on opensim-core :apple:

[jenhicks]

Thanks @tkuchida. I'm leaving this issue open on the GUI side and under Needs Verifying so that we don't forget to test.

[tkuchida]

@aymanhab: Please see @aseth1's analysis at https://github.com/opensim-org/opensim-core/issues/2152.

[aseth1]

I tracked it down further and it comes down to the following in MotionDisplayer.java on lines 813 and 823:

model.setStateVariableValues(context.getCurrentStateRef(), states.getAsVector());

where we (thanks to Java we can ignore the constness of the OpenSim::getCurrentStateRef()) write values to the internal working state of the Model. We should be providing the State to be rendered, not overwriting the internal state of the model! The const SimTK::State& signature of getCurrentStateRef() indicates that using the state for writing was never its intention.

This is not an API issue AFAICT.

[jimmyDunne]

Issue is still present

[aymanhab]

Trying to keep a copy of the state is doable but have wide ranging implications. since we need to explicitly Realize our copy to proper stage. E.g. testContext fail for this reason, and trying to load a model in the GUI with this change results in Error detected by Simbody method getCacheEntry: State Cache entry was out of date with respect to one of its explicit prerequisites. And this is just scratching the surface without knowing for sure that it fixes the issue in question.

I'll keep the branch for a future, cleaner fix, but suggest we use the more surgical fix in PR #804 for 4.0 purposes.

[aymanhab]

Thoughts @aseth1 @tkuchida @jenhicks

[aseth1]

'll keep the branch for a future, cleaner fix, but suggest we use the more surgical fix in PR #804 for 4.0 purposes.

I have to disagree. This will only perpetuate a bad design and make it harder to fix down the road. The issue here is twofold:

1. there is a flagrant violation of coding practices changing the value of a const method that is also a data member of a const object.
2. the argument of maintaining a copy of the State as being arduous is false. You are relying on the internal copy of the Model (which IMHO should not exist). That State is not magically realized to the correct stage. It follows a logical process. You are likely getting the error because you are trying to get positions to draw when the State has not been realized to the Position stage.

[jenhicks]

@aymanhab I will stop by your office this morning to discuss what we should do for the 4.0 beta we are finalizing today. @aseth1 If you have suggestions about what to do given that we need to post a beta today, please let me know.

[aseth1]

It should not be challenge to realize to the correct stage. I am not sure what is holding up the change.

[jenhicks]

@aseth1 We are not making the change at this point because the Model's internal copy of the state is used widely in the GUI. It seems too late in the game to change this over now.

[aymanhab]

Based on discussion with @jenhicks and considering the potential scope of the change, we agreed to merge the one-line surgical fix to MotionDisplayer and to leave the issue and the branch with the stateCopy intact for later consideration. This fix is easily rolled back if needed.

If you prefer to deprecate the get/updWorkingState methods you can start there and we'll adjust or we can discuss when you have a chance.

[aseth1]

This fix is easily rolled back if needed

I disagree. The Tools and GUI are filled with hacks that have never been rolled back because no-one can comprehend what they are doing. It will be the same in this case. The fact that a const method is being used to change a data member was difficult for me to track down. I doubt it will ever be fixed. It was not addressed almost three months ago when it was raised.

If you prefer to deprecate the get/updWorkingState methods you can start there and we'll adjust or we can discuss when you have a chance.

I'm not a fan of these methods, but that is not the issue here. You are using the working state as your internal data member when it is just a pointer to the model's. That is plain wrong.

[aymanhab]

I fixed the opensim-core branch with the copied state so that core test cases pass, and the models load in the GUI but other functionality broke (e.g. live playback while running tools). Will hold until I investigate these issues and we have cycles to test the far reaching effects of this change.