Are both Model::updateMarkerSet() and replaceMarkerSet() necessary?

[aseth1]

Model::updateMarkerSet(newMarkerSet) takes the union of the existing marker set, which may not be what users intend/expect. During scaling, I was expecting that the provided <marker_set_file> replaces that of the model. The comment is ambiguous.

[aseth1]

At a minimum Model::updateMarkerSet(newMarkerSet) could be renamed to appendMarkerSet() with a comment clarifying in the case of collisions (marker with same name present in the Model) that the new definition replaces the existing.

[aseth1]

Actions from today's meeting:

1. eliminate Model::replaceMarkerSet(const SimTK::State& s, const MarkerSet& aMarkerSet) because editing the Model should not require a State and have a sideeffect of creation/initialization of the System. Also, the ScaleTool in the API uses updateMarkerSet() which causes an inconsistency with the GUI if it uses replaceMarkerSet().

2. rename Model::updateMarkerSet(MarkerSet& newMarkerSet) to Model::mergeMarkerSet(MarkerSet& markerSet) and update the comment on the ScaleTool that this takes the union of the Model's and provided marker sets, with the provided set taking precedence in the case of name collisions. This method is easily confused with updMarkerSet() , which returns a writable reference to the Model's MarkerSet.

I wanted to raise the issue that this treatment of the MarkerSet is inconsistent with the treatment of a ForceSet by the AbstractTool, which uses a _replaceForceSet flag/property. If true the model's force set is replaced with the provided ForceSet files. When false the supplied ForceSets are appended to the model's. Here, collisions are poorly handled. Duplicate names are disallowed by default but when a duplicate name is found in the provided set, the append just skips that force with no error/exception or warning.

From our discussion for MarkerSets, it seemed users may want to override an existing marker so it would be better for the provided marker set to take precedence over the Model's set. Then perhaps a distinct mergeMarkerSet() is better than trying to be consistent with the confusing ForceSet::append(ForceSet &aForceSet, bool aAllowDuplicateNames)) behavior.

[aseth1]

We should at least do 1. It looks like it is not being used by the GUI see #1903