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

Consider evaluating calc before seek_before #271

Open
roccodev opened this issue Jun 19, 2024 · 1 comment
Open

Consider evaluating calc before seek_before #271

roccodev opened this issue Jun 19, 2024 · 1 comment
Labels
confusing-api Oops! Confusion occurred

Comments

@roccodev
Copy link

roccodev commented Jun 19, 2024

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 csnover added the confusing-api Oops! Confusion occurred label Jun 25, 2024
@csnover
Copy link
Collaborator

csnover commented Jun 27, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confusing-api Oops! Confusion occurred
Projects
None yet
Development

No branches or pull requests

2 participants