ultimate-research / ssbh_lib

Reading and writing SSBH file formats in Rust
MIT License
8 stars 3 forks source link

Improve ergonomics of SsbhEnum<T> type #110

Closed ScanMountGoat closed 2 years ago

ScanMountGoat commented 2 years ago

It should be possible to avoid including the data type as a field. The current implementation requires initializing the data type manually, which is tedious and error prone. This will also simplify the JSON output.

Current:

pub struct SsbhEnum64<T: BinRead<Args = (u64,)> + SsbhWrite> {
    pub data: RelPtr64<T>,
    pub data_type: u64,
}

New:

pub struct SsbhEnum64<T: BinRead<Args = (u64,)> + SsbhWrite + DataType> {
    pub data: RelPtr64<T>,
}

trait DataType {
    fn data_type(&self) -> u64;
}
ScanMountGoat commented 2 years ago

For the new struct definition, the contained type T should implement some sort of trait that describes the data type for a given T. This could just be a method fn data_type(&self) -> u64. For enums, this will depend on the value. This method will be used to determine what value to write when exporting. For implementing parsing, this may require additional annotations through some sort of proc macro.

ScanMountGoat commented 2 years ago

The main issue to be resolved in order for this to work is determining the data type when the relative offset is 0. In the current implementation, a null offset is encoded as None in Rust. If the user specifies the value as SsbhEnum64(None), there's no way to determine what data_type to write.

ScanMountGoat commented 2 years ago

A potential solution for code generation. This won't work well if the type needs to derive additional traits or have documentation.

macro_rules! ssbh_enum {
    ($enum:ident, $($tag:literal => $variant:ident($field:tt)),*) => {
        #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
        #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
        #[derive(Debug, BinRead, SsbhWrite)]
        #[br(import(data_type: u64))]
        pub enum $enum {
            $(
                #[br(pre_assert(data_type == $tag))]
                $variant($field),
            )*
        }

        impl DataType for $enum {
            fn data_type(&self) -> u64 {
                match self {
                    $(
                        Self::$variant(_) => $tag,
                    )*
                }
            }
        }
    }
}

ssbh_enum!(TestData, 1 => Float(f32), 2 => Unsigned(u32));
ScanMountGoat commented 2 years ago

This could also be done as a derive macro for Data type.

ScanMountGoat commented 2 years ago

Exposing the data type as a field also creates issues with fuzz testing since the data type is a u64 instead of an enum.