Parse misc item info #53
Conversation
peppi/src/serde/de.rs
Outdated
| missile_type: item::MissileType(r.read_u8()?), | ||
| turnip_type: item::TurnipType(r.read_u8()?), | ||
| charge_launched: item::ChargeLaunched(r.read_u8()?), | ||
| charge_power: r.read_u8()?, |
There was a problem hiding this comment.
How sure are you that these fields are always/only used for these purposes?
There was a problem hiding this comment.
They aren't, but other potential uses are unknown or undocumented. I'm pretty sure the data these fields are written from is part of a big union that has different values for different items as shown in the decomp. That's why they are garbage when the item doesn't match.
https://github.com/doldecomp/melee/blob/master/src/melee/it/types.h#L655
https://github.com/project-slippi/slippi-ssbm-asm/blob/master/Recording/SendItemInfo.s#L118
In theory, the exact byte that describes the peach turnip type could be the exact byte that determines some property of some other item, but that would be hard to find out.
There was a problem hiding this comment.
They aren't, but other potential uses are unknown or undocumented. I'm pretty sure the data these fields are written from is part of a big union that has different values for different items as shown in the decomp. That's why they are garbage when the item doesn't match.
If the meanings of these fields vary based on context, I don't think it makes sense to give them semantic names even if there's a "typical" use.
In theory we could provide some way to interpret values contextually instead, but that seems overkill for four independent(?) bytes.
There was a problem hiding this comment.
I understand not giving them their "typical" name, but otherwise the best name would be something like item_0xd4 referring to the byte offset and then provide uses in documentation.
peppi/src/model/item.rs
Outdated
| pub struct MiscInfo { | ||
| pub missile_type: MissileType, | ||
| pub turnip_type: TurnipType, | ||
| pub charge_launched: ChargeLaunched, | ||
| pub charge_power: u8, | ||
| } |
There was a problem hiding this comment.
What advantage do you see in encapsulating these inside a MiscInfo struct, as opposed to splatting them into Item?
There was a problem hiding this comment.
Because the single Option<MiscInfo> in Item encodes the fact that you either have all four fields or none at all because they are all from the same Slippi version. They seem somewhat logically grouped to me too because they all describe specific properties of subsets of items. I wouldn't die on this hill though.
There was a problem hiding this comment.
From an user perspective, they're pretty distinct imo. They won't always know that they came from the same slippi version. The 2 charge values being lumped makes sense, but it's an odd place to look for missiles and turnips which are unrelated to charge shots and eachother.
Unfortunately these fields often have garbage values in them that we need to store in order to do round-tripping. Let me know if there's a better way to structure this while also protecting users from accidentally accessing garbage.