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.

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

Tree height and width are f32 but deserialised as Option<f32> #86

Closed
lwiklendt opened this issue Sep 29, 2024 · 7 comments · Fixed by #87
Closed

Tree height and width are f32 but deserialised as Option<f32> #86

lwiklendt opened this issue Sep 29, 2024 · 7 comments · Fixed by #87

Comments

@lwiklendt
Copy link

Tree::height and Tree::width are f32 but are trying to be deserialised as Option<f32> resulting in an error

egui_tiles/src/tree.rs

Lines 57 to 62 in 3e7f324

fn deserialize_f32_null_as_infinity<'de, D: serde::Deserializer<'de>>(
des: D,
) -> Result<f32, D::Error> {
use serde::Deserialize;
Ok(Option::<f32>::deserialize(des)?.unwrap_or(f32::INFINITY))
}

@kgv
Copy link

kgv commented Sep 29, 2024

relevant: emilk/egui#5176

@emilk
Copy link
Member

emilk commented Sep 30, 2024

I think we need to revert

@s-nie
Copy link

s-nie commented Sep 30, 2024

Either revert it or also handle the serialization the same way:

serde(serialize_with = "serialize_f32_infinity_as_null") for the fields and then:

#[cfg(feature = "serde")]
fn serialize_f32_infinity_as_null<S: serde::Serializer>(
    t: &f32,
    serializer: S,
) -> Result<S::Ok, S::Error> {
    if t.is_infinite() {
        serializer.serialize_none()
    } else {
        serializer.serialize_f32(*t)
    }
}

Edit based on @lwiklendt's suggestion:

#[cfg(feature = "serde")]
fn serialize_f32_as_infinity_as_null<S: serde::Serializer>(
    t: &f32,
    serializer: S,
) -> Result<S::Ok, S::Error> {
    if t.is_infinite() {
        serializer.serialize_none()
    } else {
        serializer.serialize_some(t)
    }
}

@lwiklendt
Copy link
Author

handle the serialization the same way

The deserialiser is expecting an Option<f32>, so you would need to serialise it as an Option<f32>. That is, you would need to use something like serializer.serialize_option(t) instead of serializer.serialize_f32(*t). However, this would result in a surprising incoherence between the struct fields as f32 and their serialisation as Option<f32>.

@emilk
Copy link
Member

emilk commented Sep 30, 2024

Can JSON just add support for +/-inf already 🙄

@lwiklendt
Copy link
Author

Wow, TIL JSON doesn't have inf and NaN support.
I'm using RON in my projects, so I was blissfully ignorant of the JSON problem.

@emilk
Copy link
Member

emilk commented Oct 1, 2024

@emilk emilk closed this as completed Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants