Fantastic! I'll start looking at this soon.

Unfortunately, std::is_null_pointer<T> is C++14, not C++11.

std::is_null_pointer is C++14, not C++11

Removed check for now. Should pass on Linux and OSX now; still one mysterious test failure on Windows.

I investigated the Windows failure. It appears to be a problem with the Visual Studio linker not correctly generating a single copy of each Value<T> class so that its static members are shared by all. Hence it tries to re-register the deserialization method for Value<double> and triggers an error message. It would be easy to remove the check and ignore subsequent registration (in which case all the tests will pass), but I am worried about what will happen on deserialization because dynamic_cast may not recognize the two Value<double> objects as being the same type. I'll leave it for now as a reminder that further investigation is needed; there may be a workaround that will ensure the linker works properly.

Ah I know very little about that stuff, but would extern help?

Ah I know very little about that stuff, but would extern help?

Can't be extern because Value<T> is templatized and thus has to be defined in headers. The linker then has a nasty job to perform to ensure that every compilation unit that has instantiated Value<SomeType> for the same template argument uses the same static data members -- I'm depending on that to ensure that each type is registered only once for deserialization.

Maybe it is desirable to prefer a free function (that is, template specialization of toXmlElement()) over a member function) if both exist.

No, as explained in the Unified Call Syntax doc you linked to, the member function is to be preferred because it was provided by the class designer.

Drawing a parallel to std::begin() and std::end() ...

Coincidentally that is also discussed in the Unified Call Syntax doc, where it was mostly described as a hack for range-for; it is definitely not a general principle and it didn't seem that they were particularly happy with it. (That's Stroustrup and Sutter so I'm inclined to take them seriously!)

One potential issue with preferring the member function over a free function: say today there is a type X that I want to serialize but I don't have control over it, so I write a free function. Then next year, the author of X decides to add a toXmlElement() member function to that class. Now, the functionality has silently changed.

Now, the functionality has silently changed.

Hmmm ... any time someone changes the code of a class you are using the behavior can change! Hopefully the author took seriously the need to serialize the class and now you won't have to. Otherwise I recommend sending a nasty email!

I suppose that's true.

@klshrinidhi seemed to be able to get rid of the third argument!

One way to get rid of the name toXmlElementHelper is to rename toXmlElement to toXmlElementImpl, to signify this is the implementation of a function that does the conversion, freeing the use of the name toXmlElement to the interface.

I have started working on a TestStateXml test for Simbody, and will open a PR for that eventually.

Separately, I gave my first shot at writing out an OpenSim Model's State to XML. I got the following error:

26: Test failed due to exception: SimTK Exception thrown at String.h:353:
26:   Error detected by Simbody method String::String(T): Type T=SBModelVars has no stream insertion operator<<(T) and there is no specialized String(T) constructor.
26:   (Required condition '!"no stream insertion operator"' was not met.)

I think this means (1) SBModelVars does not have a toXmlElement() yet, and (2) the error for when toXmlElement() is not implemented for T does not indicate that the issue was a missing toXmlElement(). Actually, I don't understand how this was a run-time error instead of a compile-time one.

• It's complaining about the stream insertion operator because that is used by default to serialize when there is no toXmlElement() available. Actually it is more contorted than that because it first falls back to writeUnformatted() which is how we get inf and nan to come out right among other things. It is writeUnformatted() that falls back to stream insertion. (Links are to the relevant code.)
• Not sure whether the above contortions are a good idea, but they do save writing a lot of redundant toXmlElement() methods. Better ideas?
• The reason it is a run time error is that there are many uses of Value<T> that don't require serializing and I didn't want to make people provide serialization methods if they don't intend to serialize. The original Value<T> did have this requirement (everyone had to provide operator<<()) and it turned out to be a huge inconvenience and resulted in many indecipherable compilation errors.

One way to get rid of the name toXmlElementHelper is to rename toXmlElement to toXmlElementImpl, to signify this is the implementation of a function that does the conversion, freeing the use of the name toXmlElement to the interface.

I don't understand. toXmlElement() is the name in the interface at the moment. Please clarify.

Not sure whether the above contortions are a good idea, but they do save writing a lot of redundant toXmlElement() methods. Better ideas?

Thanks for explaining. I like idea of piggybacking off writeUnformatted() and the stream operator. I'm also fine with the run-time message (over a compile-time message). What about just catching such exceptions in the toXmlElement() function that calls writeUnformatted() to provide a more informative message?

I don't understand. toXmlElement() is the name in the interface at the moment. Please clarify.

I mean the interface for the implementor of toXmlElement() methods. I'm thinking of the following: The interface for converting an object to an XML would be SimTK::toXmlElement(obj) (a free method). It may or may not work, depending if obj supports serialization. For some objects that users are more likely to want to serialize, we provide a convenience member function, e.g. State::toXmlElement(). To provide the ability to serialize an object, you either implement toXmlElementImpl(const T&) or T::toXmlElementImpl().

auto xmlDoc = SimTK::toXmlElement(SimTK::State()); // this would work and call SimTK::toXmlElementImpl
Xml::Element SimTK::State::toXmlElement() {
   return this->toXmlElementImpl();
}
Xml::Element SimTK::State::toXmlElementImpl() {
    ...
    elem.appendNode(toXmlElement(...));
    elem.appendNode(toXmlElement(...));
}

Let me know if I'm still not being clear. The gist is that if you want to obtain an Xml::Element, you call toXmlElement(). If you want to provide the ability to convert to Xml, you implement T::toXmlElementImpl(). The fuzzy area is member functions that you call to obtain an Xml::Element, since it's weird for a user to call a method toXmlElementImpl().

Alternatively, we could just rename toXmlElementHelper() to something that more clearly conveys the meaning findSomeWayToConvertThisThingToXmlForMe(). Like convertToXmlElement(), or createXmlElement() or formXmlElement().

I still like the idea of handling the potentially error-prone version and element name code in some central way. Here's a sketch of what I'm thinking:

class XmlSerialization {
public:
    XmlSerialization(std::string type, int version, std::string attrName="");
    template <typename T>
    void appendThing(std::string name, T value);
    Xml::Element getXmlElement() const;
};
...
Xml::Element Measure_Differentiate_Result::toXmlElement(const std::string& name) const {
        static const int version = 1;
        XmlSerialization xml("Measure_Differentiate_Result", version, name);
        xml.appendThing("operand", operand);
        xml.appendThing("operandDot", operandDot);
        xml.appendThing("derivIsGood", derivIsGood);
        return xml.getXmlElement();

I think there are a bunch of flaws with this proposal. For example, how would we require that people use the XmlSerialization class instead of writing their own serialization? Also, I haven't thought through how we would actually handle different versions, or how to combine this with the corresponding fromXmlElement() method. Also, perhaps the decentralized nature of the current serialization scheme is fine (where each class is in charge of doing it correctly). Anyway, it gets rid of some of the boilerplate.

It just sorta feels like the valuable information is "type name", "version", and a list of the members to serialize. It would be good if that were all one needed to specify.

Well, I'm more confused now! What I want to achieve is Xml::Element = state.toXmlElement() and state.fromXmlElement(element). Those are problematic because States contain AbstractValue objects, whose underlying Value<T> objects can be arbitrary existing types or user-defined types. That requires a user-written method for serializing type T that can be embedded in the serialization of Value<T>, and a corresponding user-written method that can be invoked when deserializing Value<T> objects. These should be member functions when possible (for general object-oriented code reasons), free functions otherwise.

I want those functions to be easy enough to write that there is a high probability of it being done successfully. Since user types will have members of arbitrary type (including other user types) the best way to do this would be recursively. So I'd like to be able to write a T.toXmlElement() method that invokes the TT.toXmlElement() methods of the data members of type TT. But that can't work exactly because we don't know if TT has a member, so I invoked the Helper there to figure out which method to invoke. @klshrinidhi's fix makes that more appealing since it gets rid of the mysterious third argument. It would be perfect I think if it could be named toXmlElement() so that a user writes the T.toXmlElement() method by calling a series of toXmlElement(TT) helper functions which map either to TT's member or a free function toXmlElement(TT). But I don't think that's possible because the helper and the free function would have the same name (is it?).

Anyway I could live with writing the member function or free function toXmlElement() in terms of toXmlElementHelper() with the final argument gone. I guess that's not so bad. Another alternative would to name the member function and helper function toXmlElement() but name the free functions toXmlElementFree() or something.

But I don't think that's possible because the helper and the free function would have the same name (is it?).

Even if it were possible, I think it might be confusing to use the same method name for so many things.

Anyway I could live with writing the member function or free function toXmlElement() in terms of toXmlElementHelper() with the final argument gone. I guess that's not so bad. Another alternative would to name the member function and helper function toXmlElement() but name the free functions toXmlElementFree() or something.

I think either of these are fine.

@sherm1 I am starting to add in the necessary toXmlElement() methods on other types. I was going to start working on SBInstanceVars but I'm not sure I actually know how to serialize this class. While some of the members seem like part of the state (mobilizerLockLevel, constraintIsDisabled), my gut says other members aren't actually part of the state, like bodyMassProperties.

class SBInstanceVars {
public:
    Array_<MassProperties,MobilizedBodyIndex>   bodyMassProperties;
    Array_<Transform,     MobilizedBodyIndex>   outboardMobilizerFrames;
    Array_<Transform,     MobilizedBodyIndex>   inboardMobilizerFrames;

    Array_<Motion::Level, MobilizedBodyIndex>   mobilizerLockLevel;
    Vector                                      lockedQs;
    Vector                                      lockedUs; // also used for udot

    Array_<bool,          MobilizedBodyIndex>   prescribedMotionIsDisabled;

    Vector                                      particleMasses;

    Array_<bool,ConstraintIndex>                constraintIsDisabled;

Could you tell me which of these members should be serialized?

(SBInstanceVars) Could you tell me which of these members should be serialized?

It's a variable, so everything in it should be serialized. Body mass properties are instance variables, but there aren't currently any methods for setting them so they just keep their default values.

I'm not sure that solution can work... sometimes, that macro is used inside a class definition, to create a nested type; the free function would end up in the body of the containing class.

Nevermind! You can define the friend function in the class body!

#define SimTK_DEFINE_AND_EXPORT_UNIQUE_LOCAL_INDEX_TYPE \
class EXPORT NAME { ... \
public: \
    friend std::istream& operator>>(std::istream& input, NAME& obj) { input >> obj.ix; }
};

Another question: is there a particular reason why AbstractValue does not have a fromXmlElement(), but instead has a createFromXmlElement()? Is createFromXmlElement() just for testing (it's used in TestXml)?

is there a particular reason why AbstractValue does not have a fromXmlElement(), but instead has a createFromXmlElement()?

Yes -- it's an abstract class (has pure virtuals) so you can't create one. Only concrete Value<T> objects can be created. So the thing.fromXmlElement(element) form can't work since there can't be a thing until after you've determined (from the element) what kind of concrete object to make.

@sherm1 For testing, I plan to add test cases to TestXml for types in SimTKcommon, and to create a TestSimbodyXml for types in Simbody. To test the ability to fully serialize and deserialize a State object and then use it meaningfully, I was thinking of a TestStateXml for SimTKcommon and a TestSimbodyStateXml for Simbody. Does this sound good?

I was thinking of a TestStateXml for SimTKcommon and a TestSimbodyStateXml for Simbody

Good idea! TestStateXml can just test the basic functionality while TestSimbodyStateXml checks that we managed to write the (de)serialization methods for all of the junk Simbody puts in the State.

It would be easy to remove the check and ignore subsequent registration (in which case all the tests will pass), but I am worried about what will happen on deserialization because dynamic_cast may not recognize the two Value objects as being the same type. I'll leave it for now as a reminder that further investigation is needed; there may be a workaround that will ensure the linker works properly.

Do you think we could solve this issue now? I'd like to start submitting PRs and it'd be good for the appveyor tests to pass. Let me know how I may be able to help here.

I was trying to learn more about static variables inside functions and came across this blog post that said dynamic initialization of static variables is not threadsafe in some compilers (non-GCC). The blog post is somewhat old, though; this page indicates that statics are threadsafe in MSVC 2015 ("magic statics"). This info doesn't seem relevant to the current issue with MSVC; I just thought it was worth sharing.

Thread safety of function-local statics was fixed in C++11 so all compliant compilers should behave properly. This stackoverflow answer points to the specification in the standard.

I have a question about if the registration mechanism for Value is sufficient. Is it possible for users to have an executable that deserializes an xml element that contains Value<T>s that are not registered in said executable? For example, OpenSim has an addToSystem() phase in which state and cache variables, etc. are created, and presumably that's when Value<T> objects will be first constructed, and thus registered. However, I may have a generic executable that simply deserializes state objects, independent of Models. In such a case I want to deserialize a state object before (or without) ever invoking Model::initSystem() (which invokes extendAddToSystem() on all components, and thus causes the registration).

We may not readily detect this issue in our tests, because the obvious way to write tests is to first serialize then deserialize. It seems like we need tests that just deserialize from an Xml file that is stored in the repository.

That's a good point - didn't occur to me. There would have to be a registration phase where AbstractValue::registerValueFactory() gets called for each interesting Value<T>::createFromXmlElement() method. We could do that automatically for known types, maybe on first deserialization call(?). That might even fix the Value<double> problem we've been seeing. There would have to be manual registration for user types T if they are to be deserialized prior to instantiating a Value<T>.

There has to be a State object prior to deserializing. If that has to be a State created by a System many of the Value<T> objects would likely have been instantiated already. But if an empty State can be used it could be the only thing in the program.

If that has to be a State created by a System many of the Value objects would likely have been instantiated already.

This is something I'd like to chat with you about in person at some point. I know you've mentioned before that we might need a System to properly deserialize a State. Right now, if we deserialize a state, about all we can do with it (without a System) is reserialize it. We can't even ask it for its time (b/c it's not realized to Time).

We could do that automatically for known types, maybe on first deserialization call(?).

I'm not sure I understand; how can we register upon deserialization?

I still like the idea of being able to (de)serialize with only a State around, but I could be talked out of it!

We can't even ask it for its time (b/c it's not realized to Time).

That's not right -- time and state are independent of anything else and don't require realization. If they aren't available something didn't deserialize correctly.

I'm not sure I understand; how can we register upon deserialization?

Someone calls state.fromXmlElement(). Eventually that tries to deserialize a Value. That method checks a flag to see if the bulk registration method has been called; if not it calls it.

Bulk registration could also be handled with a function static variable similar to what I did for individual registration of Value<T> objects.

Ah bulk registration. I don't know...my hunch is that we should treat all Ts equally. Otherwise, it seems like manual registration will create second-class citizens that won't be well supported.

I still like the idea of being able to (de)serialize with only a State around, but I could be talked out of it!

Yeah I like that idea too.

Things to discuss:

1. Naming for toXmlElementHelper/fromXmlElementHelper. The issue with naming the free function toXmlElementFree() is that it is then too easy to subvert the desired preference of member function over free function by an unsavory user who just specializes toXmlElement() instead. dispatcher.
2. How to register Value's, considering that they may not have been constructed in the program trying to deserialize.
3. If we like fromXmlElementHelperHelper, what should we name, it? no get rid of it.
4. Should there be a corresponding toXmlElementHelperHelper. no
5. Deserializing types that must obey certain restrictions (UnitVec3, Rotation), and the tolerance for obeying the restrictions. (I am also struggling with implementing fromXmlElement for UnitVec3.
6. Do we need a System to deserialize a State? Right now, OpenSim tries to validate a StatesTrajectory, which requires calling getTime() and isConsistent(), etc. Opinion about OpenSim classes: StatesTrajectory has to/from Xml, but then another class "Simulation" combines the StatesTrajectory and a Model into a file.
7. Wrapping another Xml Element around the writeUnformatted specialization (for consistency).
8. Symmetry of Inertia tensor does not work with Infinity (the comparison thinks Infinity != Infinity).
9. Fixing the MSVC static local issue.
10. The necessity of void_t. OK. common.h. reference the docs.
11. OpenSim's 5th Subsystem is just "Subsystem."

@chrisdembia: Ah I know very little about that stuff, but would extern help?

This was from way up this thread, referring to the problem where different instantiations of Value<double> got made in different compilation units causes a dynamic_cast failure. Now I think that extern template class Value<double>; in a header with an explicit Value<double> instantiation in a .cpp might successfully prevent the duplication.

I know this is paused at the moment but a similar problem came up in Drake so I thought I'd record that thought here.

Ah...interesting. I will try that out whenever I get back to this project. Thank you for sharing.