Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix string serialization for broadphase multisap #675

Merged
merged 6 commits into from
Jul 15, 2024
Merged

Fix string serialization for broadphase multisap #675

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bfe0162
fix string serialization for broadphase multisap
Vrixyz Jul 9, 2024
b82fbb0
fix hashmap serialization to string
Vrixyz Jul 11, 2024
d29452c
fix clippy
Vrixyz Jul 12, 2024
39f1baf
Merge branch 'master' into serialize_broadphase_multisap
Vrixyz Jul 15, 2024
06a22ea
removed unused feature gate + added test clarifying comment
Vrixyz Jul 15, 2024
a21c84e
add changelog line
Vrixyz Jul 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/geometry/broad_phase_multi_sap/broad_phase_multi_sap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,13 @@ pub struct BroadPhaseMultiSap {
// Another alternative would be to remove ColliderProxyId and
// just use a Coarena. But this seems like it could use too
// much memory.
#[cfg_attr(
feature = "serde-serialize",
serde(
serialize_with = "parry::utils::hashmap::serialize_hashmap_capacity",
deserialize_with = "parry::utils::hashmap::deserialize_hashmap_capacity"
)
)]
colliders_proxy_ids: HashMap<ColliderHandle, BroadPhaseProxyIndex>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@Vrixyz Vrixyz Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@sebcrozet sebcrozet Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#[cfg_attr(feature = "serde-serialize", serde(skip))]
region_pool: SAPRegionPool, // To avoid repeated allocations.
Expand Down