[BUG] Build error in ShipDesign.h

[Vezzra]

When I tried to build recent SVN revisions, I got a build error in ShipDesign.h at line 715.

The affected code section is:

Code: Select all

// template implementations
template <class Archive>
void PartType::serialize(Archive& ar, const unsigned int version)
{
    ar  & BOOST_SERIALIZATION_NVP(m_name)
        & BOOST_SERIALIZATION_NVP(m_description)
        & BOOST_SERIALIZATION_NVP(m_class)
        & BOOST_SERIALIZATION_NVP(m_stats)
        & BOOST_SERIALIZATION_NVP(m_production_cost)
        & BOOST_SERIALIZATION_NVP(m_production_time)
        & BOOST_SERIALIZATION_NVP(m_mountable_slot_types)
        & BOOST_SERIALIZATION_NVP(m_location)
        & BOOST_SERIALIZATION_NVP(m_effects)
        & BOOST_SERIALIZATION_NVP(m_graphic);
}

The error occurs in this line:

Code: Select all

        & BOOST_SERIALIZATION_NVP(m_icon);

The error message thrown is:

Code: Select all

'm_graphic' was not declared in this scope

I found out that there is no "m_graphic" in PartType, but "m_icon". I guess "m_graphic" was replaced with "m_icon" in commit 4642, but the serialization code wasn't modified accordingly. When I replaced "m_graphic" with "m_icon" in the code section above, FO build successfully.

[Geoff the Medio]

I'm guessing MSVC didn't produce a compile error from that bug due to that serialization function never actually being used, so no template instantiations of it were created.

That being the case, it's probably OK to just update the serialization function by replacing the m_graphic with m_icon.

If there had been any current or previous use of it, it would be necessary to have a test for the PartType class version and adjust the serialization accordingly, as you can't just rename variables without breaking the serialization of old saves that contain any of that class. See the changes the ShipDesign serialization and exporting in the version that made those changes.

[Vezzra]

...If there had been any current or previous use of it, it would be necessary to have a test for the PartType class version and adjust the serialization accordingly, as you can't just rename variables without breaking the serialization of old saves that contain any of that class. See the changes the ShipDesign serialization and exporting in the version that made those changes.

Hm, shouldn't these considerations also apply to the modifications of the HullType class? There "m_icon" was added, and the serialization code adjusted accordingly, but without any test for the HullType class version. And with "m_icon" not even added to PartType, but "just" being "m_graphic" renamed, I didn't think that class version checking would be required.

However, if we decide to be on the safe side and put in version checking for PartType, shouldn't be the same done for HullType?

[Geoff the Medio]

...with "m_icon" not even added to PartType, but "just" being "m_graphic" renamed, I didn't think that class version checking would be required.

All (or most?) FreeOrion serialization is done using name-value pairs, which as far as I can tell means that the data is stored as a name for the variable and its value together. That means that if a save game was created with an older version, it will have the older name of the variable that was renamed in a newer version. That will mean that the name the code is expecting won't match what's in the archive, and it will fail to deserialize. Having a version test prevents this problem, by avoiding attempting to serialize something with the wrong name, depending on the version of the archive being loaded.

However, if we decide to be on the safe side and put in version checking for PartType, shouldn't be the same done for HullType?

I don't intend to add version checking unless it's found to be necessary.

[Vezzra]

...I don't intend to add version checking unless it's found to be necessary.

Sounds reasonable.