search

mmitch / gbsplay

gameboy sound player
https://mmitch.github.io/gbsplay/
Other
98 stars 20 forks source link

fix length of copyright string #54

Closed mmitch closed 3 years ago

mmitch commented 3 years ago

I think I stumbled about a typo from 2003. https://ocremix.org/info/GBS_Format_Specification lists all three text fields having a length of 32.

codecov[bot] commented 3 years ago

Codecov Report

Merging #54 (e4b2aaa) into master (18f2201) will not change coverage. The diff coverage is 50.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #54   +/-   ##
=======================================
  Coverage   45.31%   45.31%           
=======================================
  Files          19       19           
  Lines        2827     2827           
=======================================
  Hits         1281     1281           
  Misses       1546     1546
Impacted Files Coverage Δ
gbs.c 30.76% <50.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 18f2201...e4b2aaa. Read the comment docs.

ranma commented 3 years ago

Originally yes, but then it was truncated to 30 in a revision to make space for the length field at offset 0x6e...

ranma commented 3 years ago

Hmm, though on a quick check of a few files I don't see a length filled in at 0x6e, I wonder where I got that from...

ranma commented 3 years ago

Ok, I can only find a copy of the same "GBS FILE SPECIFICATION 1.02" locally, so AFAICS I made that change for the extended header support.

ranma commented 3 years ago

See gbsformat.txt, which would also need to be updated.

mmitch commented 3 years ago

Is the extended header something "official" or is it a gbsplay-only extension?

If we repurpose two existing fields in the GBS specification, perhaps the GBS Version should be upgraded from 1 to 2, to make it clear that the field content has changed. But if we do that, we could just add 2 extra bytes at the end of the header instead of re-using the copyright field.

ranma commented 3 years ago

It's gbsplay-only. I intentionally didn't change the GBS Version since it is optional and a player not knowing about it could still play the file just fine. And of course I don't think anyone is using the extension so maybe we should just remove it altogether. :)

([edit]In particular the edit functionality you need for creating a file with this extension is in the old xmms plugin...[/edit])

mmitch commented 3 years ago

I will have a look at removing the extended header stuff then.

mmitch commented 3 years ago

The XMMS plugin currently only handles real GBS files, so I can completely remove all code regarding the table for subsongs, titles and lengths, right?

We still have subsong titles in VGM files, but XMMS won't read them. Or should I keep the subsong handling in case we need it again? (eg. upgrade to XMMS2)

ranma commented 3 years ago

Or should I keep the subsong handling in case we need it again? (eg. upgrade to XMMS2)

For now maybe just remove it completely? The VGM format support is limited to a single track at the moment. Though it would certainly be nice to have something like the HVSC song database for setting subsong lengths and titles where known (https://hvsc.c64.org/download/C64Music/DOCUMENTS/Songlengths.faq).

mmitch commented 3 years ago

With Pull Request #55 already merged, this PR here is not needed any more. Closing.