Support for failure in basic_snapshot_loader and entt::snapshot

[bjadamson]

Hello,

I wanted to ask you if you thought it was feasible to report errors in basic_snapshot_loader and snapshot classes, or if I am missing something obvious. I use zppbits (https://github.com/eyalz800/zpp_bits) to serialize my actual entities and components, and the function for serialization/deserialization returns a std::errc indicating success/failure. To use the snapshot and snapshot loader classes, I implemented the operator() overload, and return the std::errc that zppbits gives back to me, but it seems in my calling code (load_archive) doesn't have a chance to inspect the errors.

ie:

entt/src/entt/entity/snapshot.hpp

Line 122 in f2c4174

const basic_snapshot &get(Archive &archive, It first, It last, const id_type id = type_hash<Type>::value()) const {

entt/src/entt/entity/snapshot.hpp

Line 87 in f2c4174

const basic_snapshot &get(Archive &archive, const id_type id = type_hash<Type>::value()) const {

entt/src/entt/entity/snapshot.hpp

Line 193 in f2c4174

basic_snapshot_loader &get(Archive &archive, const id_type id = type_hash<Type>::value()) {

Am I missing something obvious, is there a way to get a return value from my operator() overload?

If it helps here's my code:

using U8Buffer = std::vector<std::uint8_t>;
using U8Writer = zpp::bits::out<U8Buffer>;
using U8Reader = zpp::bits::in<std::span<std::uint8_t const> const>;

void
load_archive(auto& dest, auto& archive) noexcept
{
  // TODO: ask entt if it's possible to return serde failures
  dest
    .template get<entt::entity>(archive)
    .template get<Coord2D>(archive)

    .template get<TileType>(archive)
    .template get<IsHiddenTile>(archive)
    .template get<PassageNumber>(archive)
    .template get<RoomNumber>(archive)
    .template get<TrapType>(archive)
    .template get<TileSeenByPlayer>(archive)
    .template get<TileShroud>(archive)

    .template get<Item>(archive)
    .template get<TileItem>(archive)

    .template get<Inscription>(archive)
    .template get<AmuletData>(archive)
    .template get<ArmorData>(archive)
    .template get<FoodData>(archive)
    .template get<PotionData>(archive)
    .template get<RingData>(archive)
    .template get<ScrollData>(archive)
    .template get<StickData>(archive)
    .template get<WeaponData>(archive)
    .template get<DamageInfo>(archive)

    .template get<Monster>(archive)
    .template get<TileMonster>(archive)

    .template get<SlowState>(archive)
    .template get<MonsterDisguise>(archive)
    .template get<SeenByFarsight>(archive)
    .template get<HallucinationData>(archive)
  ;
}

class RegistryDeserializer final
{
  std::span<uint8_t const> const buffer_;
  U8Reader                       reader_;

public:
  explicit RegistryDeserializer(std::span<uint8_t const> const buffer) noexcept
    : buffer_{buffer}
    , reader_{{buffer_}}
  {}

public:
  template <typename T>
  [[nodiscard]] std::errc
  operator()(T& data) noexcept
  {
    return reader_(data);
  }

public:
  static auto
  deserialize(std::span<uint8_t const> const bytes) noexcept
  {
    Registry registry;
    {
      entt::basic_snapshot_loader loader{registry};
      RegistryDeserializer deserializer{bytes};
      load_archive(loader, deserializer);
      loader.orphans();
    }
    return registry;
  }
};

class RegistrySerializer final
{
  U8Buffer& buffer_;
  U8Writer  writer_;

public:
  explicit RegistrySerializer(U8Buffer& buffer) noexcept
    : buffer_{buffer}
    , writer_{{buffer_}}
  {}

  template <typename T>
  [[nodiscard]] std::errc
  operator()(T const& component) noexcept
  {
    return writer_(component);
  }

public:
  static auto
  serialize(Registry const& registry) noexcept
  {
    U8Buffer buffer;
    {
      entt::snapshot snapshot{registry};
      RegistrySerializer serializer{buffer};
      load_archive(snapshot, serializer);
    }
    return buffer;
  }
};

[bjadamson]

I wrote a rough pass at an implementation of an alternate method for get<> for snapshot_loader to show what I was thinking. It's based off the existing get<> template in the snapshot_loader class. Obviously it's not sufficient, but I thought it would provide something to help demonstrate what I was thinking:

template<typename Type, typename Archive>
std::errc my_get(Archive& archive, const id_type id = type_hash<Type>::value()) const {

  if (const auto* storage = reg->template storage<Type>(id); storage) {
    if (std::errc const ec = archive(static_cast<typename traits_type::entity_type>(storage->size())); ec != std::errc{})
    {
      return ec;
    }

    if constexpr (std::is_same_v<Type, entity_type>) {
      if (std::errc const ec = archive(static_cast<typename traits_type::entity_type>(storage->in_use())); ec != std::errc{})
      {
        return ec;
      }

      for (auto first = storage->data(), last = first + storage->size(); first != last; ++first) {
        if (std::errc const ec = archive(*first); ec != std::errc{})
        {
          return ec;
        }
      }
    }
    else {
      std::errc ec{};
      auto const fold = [&ec, &archive](auto&&... args) {
        if (ec != std::errc{})
          return;
        ec = (archive(std::forward<decltype(args)>(args)), ...);
      };
      for (auto elem : storage->reach()) {
        std::apply(fold, elem);
      }
      return ec;
    }
    return std::errc{};
  }
  else {
    return archive(typename traits_type::entity_type{});
  }
}

[skypjack]

The main problem with my_get is that you can't chain the calls as you did in the first snippet. Moreover, it works with std::errc but other users may want to work with different types. That is, this is very specific to your use case as far as I can tell.
Since the snapshot classes don't really fail and all errors are related to the archive, you could move the error handling there. Have you considered this option?
You can store the result of reader_(data) in the archive, then ask it if everything is ok (at the end or after each step if you like).
You achieve the same result while different users can have similar results while using different error types as needed.
Is there anything wrong that I can't see with this approach?

[bjadamson]

Thank you so much for the reply!

You can store the result of reader_(data) in the archive, then ask it if everything is ok (at the end or after each step if you like).

Maybe I'm not seeing it clearly, but because get contains 4 calls to the archive I don't think it's possible to handle error handling outside of the get method:

entt/src/entt/entity/snapshot.hpp

Line 126 in f2c4174

archive(static_cast<typename traits_type::entity_type>(std::distance(first, last)));

entt/src/entt/entity/snapshot.hpp

Line 130 in f2c4174

archive(entt);

entt/src/entt/entity/snapshot.hpp

Line 133 in f2c4174

archive(static_cast<entity_type>(null));

entt/src/entt/entity/snapshot.hpp

Line 137 in f2c4174

archive(typename traits_type::entity_type{});

What I'm looking for is when any of the calls to the archive returns "an error", I can stop immediately (print information about the error).

The main problem with my_get is that you can't chain the calls as you did in the first snippet.

I didn't mean to my_get to be more than a straight-forward example to demonstrate that at every call to the archive I want to check for an error, and exit early if one is discovered. I think the method chaining is nice syntax but it hides any error handling opportunities for someone who wants to abort serialization/deserialization when the first error is discovered. Method chaining is possible by using a reference parameter instead of a return value, but at every step of the serialization I want to check if serialization failed. With method chaining, it will call get for the entire chain, even if there's an error on the first item.

You achieve the same result while different users can have similar results while using different error types as needed.
Is there anything wrong that I can't see with this approach?

I guess I need to wait to see if I'm missing something obvious. I waited a couple days to respond because I wasn't sure if I was thinking enough about your response, but I don't see it.

[skypjack]

Bear with me, I'm just trying to brainstorm with you to see if it would work. 🙂
Imagine you have an archive that returns a boolean value. Then you can wrap it like this probably:

void
load_archive(auto& dest, auto& archive) noexcept {
    std::errc error{};
    auto error_aware_archive = [&error, &archive](auto &&... args) {
        if(error == std::errc{}) { error = archive(std::forward<decltype(args)>(args)...);
    };

    if(dest.template get<entt::entity>(error_aware_archive ); error != std::errc) {
        // do something to handle the error on the get<entt::entity> call
    }

    if(dest.template get<Coord2D>(error_aware_archive ); error != std::errc) {
        // do something to handle the error on the get<Coord2D> call
    }

    // and so on
}

This way you have the error available but we don't have to bind EnTT to std::errc (which isn't a good thing imho since it's very specific of your use case but could be just as wrong for other users).
Does it make sense or am I missing something in your reasoning that makes this approach unfeasible?

[Innokentiy-Alaytsev]

At very least, using std::error_code might be a bit much better than std::errc.

[bjadamson]

This is a really interesting idea for solving my issue, thank you for spending some time on it.

Tell me if I'm off here, but since the get<> method has multiple calls to archive(), I think a full implementation would need to look something more like this:

std::vector<std::errc>
load_archive(auto& dest, auto& archive) noexcept
{
  std::vector<std::errc> errors;
  auto const error_aware_archive = [&errors, &archive](auto &&... args)
  {
    std::errc const ec = archive(std::forward<decltype(args)>(args)...);
    if (std::errc{} != ec)
    {
      errors.push_back(ec);
    }
  };

  if (dest.template get<entt::entity>(error_aware_archive); !std::empty(errors)) {
    return errors;
  }
// .... repeat for each component
}

Does it make sense or am I missing something in your reasoning that makes this approach unfeasible?

This could totally work, and thank you again for spending time on this with me, it's just I don't think it's as good a solution as getting a return value from get<> directly.

1. I have to process the full component, even if the first call to the archive fails.
2. In the general case, I need to allocate space for N return values.

[bjadamson]

At very least, using std::error_code might be a bit much better than std::errc.

Yeah I think a solution that allows the user to specify whatever return value they want instead of binding it to std::errc or std::error_code is the way.

[skypjack]

Hi @bjadamson sorry for the loooong gap but I went on vacation and decided to take a full break from everything this time. 🙂
To address both your concerns, we can probably make archive optionally return a boolean value, then use it to early return from the serialization function on error. Does it make sense?
This way:

• You don't have to process the component if the first call fails
• You don't need to allocate for N return values since we return immediately on first error

Moreover, we have a couple of extra benefits imho:

• You can still use the error type that you prefer while EnTT only cares about the boolean (return) value
• We can make it optional and we have full backward compatibility for an archive that returns void

Do you see any weak points here? Would it work in your case too? I think so but a double check is welcome. 🙂

[bjadamson]

Happy new year!

To address both your concerns, we can probably make archive optionally return a boolean value, then use it to early return from the serialization function on error. Does it make sense?

This sounds great, but it's unclear (unless an in/out parameter is used) to me how this can be done. I'm very curious what you have in mind. The extra benefits sound wonderful. Please let me know if I can do anything to assist.

Do you see any weak points here? Would it work in your case too? I think so but a double check is welcome.

I believe so.

[skypjack]

but it's unclear (unless an in/out parameter is used) to me how this can be done

How is your archive function defined? I guess something like void archive(const T &value) { ... }, right?
What I mean is that EnTT could accept both this function type or bool archive(const T &value) { ... }.
In the second case (and only in the second case), the library is demanded to check the return value of the archive function after each call before proceeding.
In case of a false value, EnTT would return immediately. This way you have an early out during the serialization process.

Actually, we can even do better than this. We can support both void archive(...) and R archive(...), where R is a type that works in a boolean context.
EnTT can check it and return the value as-is if it converts to false. This makes it possible to use user defined error types and make error values flow from the archive function to the caller easily.
It would be transparent to the error type, it would work for all error types as long as they convert to bool and it would be easy to offer backward compatibility in case of void or true-lish return types.

Does it make sense? I think it would also work like a charm in your case.

[skypjack]

Ping @bjadamson 🙂

[bjadamson]

Sorry! I didn't notice you updated the thread.

How is your archive function defined? I guess something like void archive(const T &value) { ... }, right?

Their all defined mostly using this macro:

#define DEFINE_SERIALIZE_FN(...)                                                       \
static constexpr auto serialize(auto& archive, auto& self) noexcept -> zpp::bits::errc \
{                                                                                      \
  return archive(__VA_ARGS__);                                                         \
}

I agree I believe it would work as well, as errc (typedef for std::errc). The one thing that comes to mind is that for std::errc and std::error_code true implies an error, false is success. I assume then default thing you would want to do is assume true means success.

https://groups.google.com/a/isocpp.org/g/std-discussion/c/b3gMahfDxSw?pli=1
https://stackoverflow.com/a/48614155