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

fix alignment issues #9

Merged
merged 3 commits into from
Feb 22, 2024
Merged

fix alignment issues #9

merged 3 commits into from
Feb 22, 2024

Conversation

nil-vr
Copy link
Contributor

@nil-vr nil-vr commented Feb 21, 2024

  • A test is added for seeking backwards into the current page. This always worked because the page is flushed before seeking, effectively making the current page a previous page.
  • A test is added for performing a write that crosses from one written page into another written page, and the implementation is fixed such that the second written page is not zeroed. This requires that the output stream is seekable when writing more than one page, but writing e57 files already requires that the output stream be seekable so this shouldn't be a problem.
  • The PagedWriter no longer allocates any vectors.
  • The writer is aligned after writing images. I'm pretty sure the C++ implementation expects this.

@cry-inc
Copy link
Owner

cry-inc commented Feb 21, 2024

Thanks a ton for this PR! I will try to do a detailed review + merge + bugfix release in the next days. I am just not yet sure when exactly I will find the time to do so.

@cry-inc cry-inc merged commit 7a2d054 into cry-inc:master Feb 22, 2024
2 checks passed
@cry-inc
Copy link
Owner

cry-inc commented Feb 22, 2024

Today I merged your changes and released a new version of the crate. I added some additional changes as well. The important ones are:

  • Slightly refactored and extended the tests for the paged writer to cover more scenarios
  • Align writer to 4-byte boundary after every blob instead of after an image that could already contain multiple blobs

Thanks again for reporting this and also submitting a PR with fixes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants