jam1garner / binrw

A Rust crate for helping parse and rebuild binary data using ✨macro magic✨.
https://binrw.rs
MIT License
545 stars 35 forks source link

Consider evaluating `calc` before `seek_before` #271

Open roccodev opened 1 month ago

roccodev commented 1 month ago

Consider evaluating calc/try_calc before seek_before, which would enable the following setup:

#[bw(stream = st)]
struct Main {
    // need to write textures offset at 0xC...

    // write other fields

    // ...but we don't know the offset until we're here.
    // It would be nice if we could get the texture offset this way:

    #[bw(try_calc = st.stream_position())]
    #[br(temp)]
    #[brw(seek_before = SeekFrom::Start(0xC), restore_position)]
    textures_offset: u32,

    // textures written here...
    textures: Textures,
}

Currently, try_calc is evaluated after seek_before, so the value of st.stream_position() is always 0xC.

I think it would make sense to switch the order of operations here, considering the argument to SeekFrom is either a constant or a field (it wouldn't make sense to use the old stream position, you'd use SeekFrom::Current instead), so it shouldn't break existing use cases.

csnover commented 3 weeks ago

Hi, thanks for your report!

This is related to the general problem of writing offsets, #4.

Other than actually fixing #4 so there is a simple blessed way to handle this case, I am not really sure what to do, and I am struggling to think through the ramifications of making a change here. From a logical perspective, the current order of operations is correct: value calculation replaces reading/writing, and reading/writing occurs after seeking in the file, so calculation also occurs after seeking. But then, to do what you are doing here, you have to get silly and use an extra temporary field.

I guess the place where this would break the most obviously is if someone was e.g. using SeekFrom::Current and then wanted to get the offset from there, but I can’t think of a format off the top of my head where it would make sense to do this.