Motion playback enforces coordinate range (clamped) not enforced during forward simulation

[aseth1]

Steps to reproduce

1. Load ..Models/BouncingBlock/bouncing_block.osim
2. Simulate for 1s.
3. Playback motion

Expected result

Contact sphere (foot) does not penetrate the floor on playback and joint angles are identical to the forward simulation.

Actual result

Note that the foot goes through the floor because pin2 does not flex enough. Instead of reaching -131 degrees the playback restricts the coordinate to -114.5 degrees (-2 rad). This is not an issue with the assembly.

Environment and GUI version

[tkuchida]

Possibly related to #534.

[aymanhab]

@aseth1 Loading the resulting states file from running FD tool and saving results to a file produced identical curve to the one shown above, Is there a better way to compare? Also running the FD tool as described produces the following message in the shell:

AssemblySolver::track() attempt Failed: SimTK Exception thrown at Assembler.cpp:973:
Method Assembler::track() failed because:
Unable to achieve required assembly error tolerance.
Assembly error tolerance achieved: 3.9279356989219139e-09 required: 1e-10.
Model unable to assemble: AssemblySolver::assemble() Failed: SimTK Exception thrown at Assembler.cpp:897:
Method Assembler::assemble() failed because:
Unable to achieve required assembly error tolerance.
Assembly error tolerance achieved: 4.5798533854224388e-09 required: 1e-10.

[aymanhab]

The GUI calls setStateVariableValue generically on all states, when the call is made with a Coordinate that is clamped, the clamping is enforced even if enforceConstraints is set to false. Would that explain? @aseth1 @tkuchida @chrisdembia Should we be using a different call that sets the Y vector directly? We know these are states at this point.

[aymanhab]

https://github.com/opensim-org/opensim-gui/blob/master/Gui/opensim/view/src/org/opensim/view/motions/MotionDisplayer.java#L793

[aseth1]

@aymanhab Coordinate::setValue() was "designed" to satisfy the GUIs need for the Coordinate slider. It may be incorrect for Component::setStateVariableValue() to delegate to this call and we could have it update the state directly with no check (easy to do). The only concern is that the two will be inconsistent with Coordinate::setValue() doing checks to make sure the state value is within range and Component::setStateVariableValue() writing directly to the State. If that's OK this can be easily fixed. We could also make the behavior consistent and not clamp in Coordinate::setValue(), in which case, clamp will be a purely GUI capability that limits the coordinate sliders. We could also put the clamping under the enforceConstraints flag in keeping that we will one day use unilateral constraints to enforce the clamp in forward simulations as well. What are some preferences: @tkuchida @chrisdembia @jenhicks?

[aseth1]

The bigger issue with MotionDisplayer is the fact that it wrangles values from various sources into a State at every time frame of data while it is playing. This seems grossly inefficient especially when combined with the fact that the code uses the absolute slowest way to set state values by name doing the lookup at every timeframe. I believe this is the main reason we don't have real-time playback.

A simpler/faster solution is to convert files and motions into states (StatesTrajectory) and then have it render at a given time rate (where changing a factor affects player speed).

[aymanhab]

Made a build where clamping is enforced only if enforceConstraints and the issue has been fixed, but no rush to put it in until we have a consensus.
@aseth1 Agree about the inefficiency, in the past and up to 3.3 we used setY, updY to set all states together, but the recommendation was to not to do that in 4.0 since names are more robust. Completely open to refactoring, modernizing the class to new 4.0 API as long as it meets the variety of use cases by actual users.

[aseth1]

Naming is more robust for a single transaction and that can be done once with getStateVariableNames() to get the correct order and then use setStateVariableValues() to set all states as a Vector. Furthermore, IMHO this should all be done prior to playback so that we are passing States to the motion player. This would boost playback performance. I think this explains why playback is choppy when playing at a speed factor of 1 or greater.

[chrisdembia]

I'm wary about using the clamped property to toggle unilateral constraints in forward simulations in the future. That sounds like a backwards-incompatible change.

I'm fine with either (a) only clamping in Coordinate::setValue() if enforceConstraints, or (b) never clamping in Coordinate::setValue().

[chrisdembia]

What I would really like is a way to easily directly write to State::updY().