OutpostUniverse / OP2Utility

C++ library for working with Outpost 2 related files and tasks.
MIT License
4 stars 0 forks source link

Extend Stream library to support non-trivially-copyable objects #277

Open DanRStevens opened 5 years ago

DanRStevens commented 5 years ago

I noticed there are a number of structs being used for serialization, but due to complex members, such as std::vector, they are not std::is_trivially_copyable. As such, they can not be automatically serialized.

To get around this, such objects often have a custom Write(Stream::Writer&) style method, which is manually called. I was thinking perhaps the Stream library could be updated to support auto calling such a member function on non-trivially-copyable objects. This would make serialization of such complex objects more consistent with traditional trivially-copyable objects.

Example:

struct ComplexObjectType {
  std::vector<int> nonTrivialField;  // Internally the vector may have pointers to external buffers

  Write(Stream::Writer& writer);
};

void f(Stream::Writer& writer) {
  ComplexObjectType object;
  writer.Write(object);  // Proposed upgrade would auto call: object.Write(writer)
}

On it's own, this small syntax adjustment doesn't buy much. It merely swaps the order of the writer and the object. However, combine this with a small update to vector writes, and we would be able to automatically serialize a std::vector<T> of non-trivial types. See the proposal in Issue #278.

DanRStevens commented 5 years ago

This article has some details about how to detect the presence of a given member: https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Member_Detector

I was hoping there would be a built in type_traits method, but I'm not seeing an exact match. The closest looking one I see is is_member_function_pointer. Maybe it can be used.

Brett208 commented 5 years ago

I generally support this and #278.

In map, we have both a WriteMap and WriteSavedGame. I'm guessing that class would not be a candidate for this improvement unless we separated into two classes, Map and SavedGame.

Also, for Bitmap, we have named the write function WriteIndexed. I like this because it explicitly tells the user they cannot write a non-indexed bitmap. This function could just be renamed to write, and a comment added about only supporting index bitmaps.

In C# you can decorate functions although I haven't researched the concept in C++. There might be a way to decorate a different named function to serve as the 'Write' function, although I don't know if we want to support it or not. It starts adding more complexity to the project since less people will be familiar with decorations.

The proposed syntax will certainly improve the readability when reading/writing.

-Brett

DanRStevens commented 5 years ago

That's a good point, yes. I suppose SavedGame could be made a subclass of Map. Though I'm not sure if that's something we want to do. The other thought is, the class itself knows if it has only map data, or full saved game data, so it can choose which to write. Though that doesn't play well with the idea of extracting a map from saved game data.

Another possibility is to allow specifying a serialization method. Example: an Image class could serialize as BMP, JPEG, PNG, TIFF, etc.

For Bitmap, maybe the class should be named IndexedBitmap? Or if there is a desire to extend it to support other bitmap types, to just name the methods accordingly, and throw a not supported error if used outside of its current design.

Sounds like this idea may need a bit of refinement.

DanRStevens commented 5 years ago

I noticed this today, and wanted to add a reference to it. Some of the techniques in that thread may be of use here for detecting a special Write method. I believe they can be adapted to look for a special signature, or to verify that a certain expression would compile. https://stackoverflow.com/questions/257288/is-it-possible-to-write-a-template-to-check-for-a-functions-existence

DanRStevens commented 5 years ago

I was doing a bit of research today. I took a second look at the link above, and also an additional link that seems to be useful: https://stackoverflow.com/questions/257288/is-it-possible-to-write-a-template-to-check-for-a-functions-existence https://stackoverflow.com/questions/23383599/how-to-find-out-if-a-type-has-member-function-with-any-return-type

In both threads, there are examples of the newer trailing return type syntax. It seems to play really nice with feature detection, in terms of deciding up front if a given line of code will compile. I may start to prefer that syntax over placing enable_if in front of the method. The example in the second thread seems much more readable than previous examples.

template <typename C, typename... Args>
struct is_call_possible {
private:
    template<typename T>
    static auto check(int)
        -> decltype( std::declval<T>().operator()(std::declval<Args>()...),
                     // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                     // overload is removed if this expression is ill-formed
           std::true_type() );

    template<typename>
    static std::false_type check(...);
public:
    static constexpr bool value = decltype(check<C>(0))::value;
};

In the check<C>(0) unevaluated call at the end, the 0 is to prefer the first overload check(int) (with return type true_type), as opposed to the second overload check(...) (with return type false_type).