Fix string serialization for broadphase multisap

[Vrixyz]

• Context: Basic serialization example bevy_rapier#560.

Without that, serialization to string fails. Interesting to note bincode works without that.

error log:

thread 'main' panicked at bevy_rapier3d/examples/serialization.rs:30:51:
Unable to serialize `RapierContext`: Error("key must be a string", line: 0, column: 0)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Encountered a panic in system `serialization::print_physics`!
Encountered a panic in system `bevy_app::main_schedule::Main::run_main`!

After more testing, I realize this crash doesn't happen every time 😮 ; around once in 10 runs 🤔.

The solution for this issue is more involved than I thought, serde json doesn't support non-string keys for Hashmap: serde-rs/json#887 ; I'm not sure what's the path forward.

[sebcrozet]

This doesn’t look correct. serialize_hashmap_capacity serializes the hashmap as a single value (its capacity) but doesn’t actually serialize its content. So deserialization will result in an emptied collidiers_proxy_id which is invalid.

I’m surprised this breaks string serialization though. Is it failing to serialize ColliderHandle or BroadPhaseProxyIndex?

[Vrixyz]

It was failing on the key ; I'll dig more ; I updated the PR description

[Vrixyz]

I believe that's the root issue: serde-rs/json#887. I'm surprised there is not a workaround provided in the issue :(

I imagine we could serialize to Vec, and deserialize to HashMap ? But I'm not sure how that would get implemented. And where, because we should keep the same hashmap serialization logic for bincode.

[sebcrozet]

Mmh, yeah, I’m surprised no alternative was suggested in the serde issue.

I imagine we could serialize to Vec, and deserialize to HashMap ?

That sounds reasonable. Implementing this would be somewhat similar to de/serialize_hashmap_capacity. The serialization functions would look something like that:

/// Serializes a hash-map as a `Vec<(K, V)>`.
#[cfg(feature = "serde-serialize")]
pub fn serialize_hashmap_as_vec<S: serde::Serializer, K, V, H: std::hash::BuildHasher>(
    map: &StdHashMap<K, V, H>,
    s: S,
) -> Result<S::Ok, S::Error> {
    let hashmap_as_vec = /* convert the hashmap to a vec */;
    hashmap_as_vec.serialize(s);
}

/// Deserializes a hash-map from a `Vec<(K, V)>`.
#[cfg(feature = "serde-serialize")]
pub fn deserialize_hashmap_from_vec<
    'de,
    D: serde::Deserializer<'de>,
    K,
    V,
    H: std::hash::BuildHasher + Default,
>(
    d: D,
) -> Result<StdHashMap<K, V, H>, D::Error> {
    let hashmap_as_vec: Vec<(K, V)> = Deserialize::deserialize(deserializer)?;
    /* Convert the hashmap_as_vec to a hashmap */
}

because we should keep the same hashmap serialization logic for bincode

It’s fine if we don’t keep the same logic for bincode.

[sebcrozet]

Looking good, thanks!

[sebcrozet]

Not needed since the pub mod serde is already feature-gated.

[Vrixyz]

I was conflicted between gating the whole mod or each functions ; gating the whole mod might give users a bit of a pain for their "use" statements, but it's not a huge annoyance, and it feels cleaner to gate the whole mod