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

Timing points are cleared from beatmaps when parsed from JSON #15328

Open
smoogipoo opened this issue Oct 28, 2021 Discussed in #15327 · 6 comments · May be fixed by #25131
Open

Timing points are cleared from beatmaps when parsed from JSON #15328

smoogipoo opened this issue Oct 28, 2021 Discussed in #15327 · 6 comments · May be fixed by #25131
Labels
area:beatmap-parsing .osu file format parsing priority:1 Very important. Feels bad without fix. Affects the majority of users.

Comments

@smoogipoo
Copy link
Contributor

Discussed in #15327

Originally posted by khang06 October 28, 2021
i'm not really sure if this is worth fixing since it doesn't look like beatmaps parsed from json are actually used anywhere right now, but it was affecting some stuff i was doing with them. the timing points are serialized fine, but don't get deserialized at all.

i suspect that this is because the fields that contain them in ControlPointInfo inherit IReadOnlyList, so they can't be deserialized properly

@smoogipoo smoogipoo added area:beatmap-parsing .osu file format parsing priority:1 Very important. Feels bad without fix. Affects the majority of users. labels Oct 28, 2021
@khang06
Copy link
Contributor

khang06 commented Oct 28, 2021

trying to fix this locally, i ended up just having the inner private lists get serialized/unserialized. i'm not sure if it's the best solution, but it was the least complex one

//[JsonProperty]
[JsonIgnore]
public IReadOnlyList<TimingControlPoint> TimingPoints => timingPoints;

[JsonProperty]
private readonly SortedList<TimingControlPoint> timingPoints = // ...

unfortunately, this doesn't seem to work on groups since it doesn't look like BindableList<T> is supported for json serialization/deserialization

Newtonsoft.Json.JsonSerializationException : Cannot create and populate list type osu.Framework.Bindables.IBindableList`1[osu.Game.Beatmaps.ControlPoints.ControlPoint]. Path 'control_point_info.groups[0].control_points', line 76, position 27.

@bdach
Copy link
Collaborator

bdach commented Oct 28, 2021

since it doesn't look like BindableList<T> is supported for json serialization/deserialization

that's incorrect, the concrete type (de)serialises just fine, just checked.

2021-10-28-210034_1222x274_scrot

the issue is the interface spec, newtonsoft doesn't know which implementation of IBindableList<T> to choose when encountering the groups members. this presumably only works for built-in collection types by some sort of hardcoding on the newtonsoft side for convenience.

@khang06
Copy link
Contributor

khang06 commented Oct 28, 2021

yeah, that makes more sense. i just assumed BindableList<T> was the problem since serialization worked just fine on the bare IBindableList<T> and i didn't read the exception closely enough

@peppy
Copy link
Member

peppy commented Oct 29, 2021

Does the same solution not work for the groups case as well? ie. moving the JsonProperty specification to the non-interface private type.

@khang06
Copy link
Contributor

khang06 commented Oct 29, 2021

yeah, that solution works for groups. i just forgot to do the same to ControlPoints in ControlPointGroup in the original comment. after doing that, i get this:

Newtonsoft.Json.JsonSerializationException : Could not create an instance of type osu.Game.Beatmaps.ControlPoints.ControlPoint. Type is an interface or abstract class and cannot be instantiated. Path 'control_point_info.groups[0].control_points[0].time_signature_bindable', line 77, position 38.

doesn't look like applying [JsonConverter(typeof(TypedListConverter<ControlPoint>))] to controlPoints works either, unless i'm missing something here

Newtonsoft.Json.JsonSerializationException : Error setting value to 'controlPoints' on 'osu.Game.Beatmaps.ControlPoints.ControlPointGroup'.
  ----> System.ArgumentException : Object of type 'System.Collections.Generic.List`1[osu.Game.Beatmaps.ControlPoints.ControlPoint]' cannot be converted to type 'osu.Framework.Bindables.BindableList`1[osu.Game.Beatmaps.ControlPoints.ControlPoint]'.

@peppy
Copy link
Member

peppy commented Oct 29, 2021

It can probably be made to work with a bit of refactoring. I believe the remaining issue is that you are trying to apply it to a BindableList but the TypedListConverter doesn't immediately support this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:beatmap-parsing .osu file format parsing priority:1 Very important. Feels bad without fix. Affects the majority of users.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants