search

hohav / peppi

Rust parser for Slippi SSBM replay files
MIT License
37 stars 9 forks source link

Add Platform enum for metadata #28

Closed NickCondron closed 1 year ago

NickCondron commented 1 year ago

EDIT: I dropped the parts of this PR related to implementing serialization on metadata directly due to the concerns raised below. I don't think the issues are insurmountable, but it's probably a low-priority issue and/or is blocked by issues with the spec.

hohav commented 1 year ago

This approach is unfortunately inconsistent with Peppi's round-trip goal. (I should really write down Peppi's goals somewhere.)

  1. Different Slippi platforms output different metadata, and AFAIK the spec makes no exhaustivity guarantees about the fields it lists for metadata.
  2. We must maintain order, which isn't generically possible with the current approach. It's possible that metadata is always written in the same order by current Slippi platforms, but again that's not guaranteed by the spec.
  3. There's also the possibility of inconsistencies with complex types like timestamps. I recall seeing slightly different timestamp formatting from different Slippi platforms.
NickCondron commented 1 year ago

The Spec can be really frustrating at times. Fizzi himself has said 'metadata was a mistake'. If the spec is bad and non-exhaustive, we should fix the spec. Figuring out which fields are undocumented wouldn't be too hard. Fields can be added/changed in later versions, but we already admit that future versions break our round-tripping guarantees. We could store ordering information and/or information about timestamp formats... Maybe it isn't worth the effort.

I'll think about it some more. There's still changes in this PR worth keeping otherwise.

hohav commented 1 year ago

I like the Platform enum and removing the defunct JMESPath code. Could you split them into their own PRs?

I don't like the metadata_raw renaming. When someone serializes a game to JSON (e.g. via peppi-slp), I want them to get the exact same metadata as what's in the file.

NickCondron commented 1 year ago

Closing in favor of two separate PRs as requested. Starting with #36. Platform will be added afterwards because it conflicts.