Skip to content
This repository has been archived by the owner on Apr 5, 2024. It is now read-only.

Fix usage of pack_fseek in dumb_packfile_skip #94

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

connorjclark
Copy link

According to the Allegro 4 docs, the second argument to pack_fseek denotes a relative offset, but dumb assumes it is an absolute offset. This resulted in a bug where music files would not load: for example, in it_xm_load_sigdata the call to dumbfile_skip would mess up the file handle and result in the subsequent version check seeing garbage data.

There is another faulty usage of pack_fseek in this file from dumb_packfile_seek, however I don't see a way to resolve it as there is no way to jump to an arbitrary position in an allegro packfile. Besides, it seems unused, so probably best to delete it.

@connorjclark
Copy link
Author

Besides, it (seek) seems unused, so probably best to delete it.

Oh, it's actually used for mod.

I found the dumb_register_stdfiles feature and am using that instead with no problems.

@kode54
Copy link
Owner

kode54 commented Jun 4, 2022

This does nothing for dumb_packfile_seek, which expects an absolute offset.

@connorjclark
Copy link
Author

Yes, as I said I didn't fix that one because I don't think there is a way to use packfile api to do it

@kode54
Copy link
Owner

kode54 commented Jun 4, 2022

It didn't occur to me that this library even had a point in being deeply integrated with Allegro. And in fact, I already recommend replacing it with libOpenMPT, as it's both faster and more accurate, and handles quirks with different versions of trackers/files better.

@connorjclark
Copy link
Author

The packfile usage certainly doesn't make any sense to me, that entire "filesystem interface" should probably just be deleted. Besides, it seems it's lacking some important functional (seeking). The stdlib fs should be good for any allegro user.

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

Successfully merging this pull request may close these issues.

2 participants