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

New .nbs format is missing data, which prevents external programs from reading the file properly #67

Closed
Bentroen opened this issue Aug 6, 2019 · 3 comments
Assignees
Labels
C: Bug Something isn't working S: Resolved in next release This bugfix or suggestion has been applied to the code

Comments

@Bentroen
Copy link
Member

Bentroen commented Aug 6, 2019

While updating a Python library that deals with NBS files, I found a few issues with the current format, which goes from removal of vital data from the file, to a few mistakes in the current documentation.

The screenshot below is an NBS file opened in a hex editor, where I highlighted some pertinent info:
HEX 1

  • Removal of layer count breaks external programs

The song length and layer count are no longer stored because, according to this, they are not used anywhere in the program. Though, the reason why it used to save that info was so that programs reading the NBS file can know where to stop reading the layer data, and start reading the custom instrument data (if present). This is shown in the original NBS format specification, as well as the new one, despite the format itself not showing any signs of it:

Part 3: Layers
Here the information about the layers are stored, which in this case are the layer names, their volumes and their stereo. These values are repeated the same number of layers there are (the song height, specified at the beginning of the file) [...].

The removal of this data prevents external programs from reading the file properly. For example, in pynbs, a Python module I used to work with a lot, the number of layers, which is obtained from the header prior to reading the layer data, is passed as a parameter to the function that reads the actual layers, so the program knows how many layers it should look for. Without that info, the program isn't able to tell where the layer data stops and the instrument data starts, which prevents it from reading the file properly.

So, even though the layer count may not actually be used in the program itself, it's vital for external programs reading the files.

  • Unnecessary removal of song length in the file format

In a similar fashion, the song length, which was a very handy value to have, is no longer stored in the file. Basically everything you could ever want to do with a NBS file involves the song length in some way, and although it's still possible to calculate it while iterating over the notes, it has become unnecessarily complicated. Since the program already keeps track of that value, it's just a matter of writing it to the file.
I'm aware that, as of issue #18, the bytes which used to store the song length are now 0 as a way to determine if the file was saved in the new format. Though, since there's already a way to detect it, I see no problem in adding it back somewhere further down the header.
(It's a bit incoherent for the file to store cosmetic stuff such as the number of right-clicks, but NOT the song length, don't ya think? :P)

  • Missing info on documentation:

    • endnb2 short in the header
      After the vanilla instrument count, and before the integer that stores the length of the title string, there are two bytes being written which are not shown in the documentation. These come from a short that's referred to as endnb2 in the code (from scripts/save_song/save_song.gml). What is that doing?

    • 'Note blocks removed'
      Looking at the code, 'Note blocks removed' still seems to be saved, though it's absent from the documentation.

    • How the actual note blocks are stored
      From what I've seen there were no changes in the way note blocks are stored; though, it's not shown on the new documentation.

@Bentroen Bentroen added the C: Bug Something isn't working label Aug 6, 2019
@HielkeMinecraft
Copy link
Collaborator

I'm sorry about the documentation not being 100% correct. It's still quite new.

The endnb2 that is being stored, but not documented, is actually the layer count.
The same with 'note blocks removed', It is still saved, the documentation is just wrong about it.

I don't really understand what you mean by "How the actual note blocks are stored", they are stored the same as they've always been, where does it tell you that is isn't?

I agree that we should put this data back into the nbs format. I want to get it right this time, so nbt version 3 will (hopefully) be the final one ;)

@HielkeMinecraft HielkeMinecraft self-assigned this Aug 6, 2019
@Bentroen
Copy link
Member Author

Bentroen commented Aug 6, 2019

No problem! I understand that the new documentation is very recent, but I'm sure with a bit of time and thought we can get it perfect :)

Ooohh, it all makes sense now! If endnb2 is the layer count, at least there's a way to update the script for now, even if it's not the final version of the format.

What I was talking about is the properties of the note blocks themselves (i.e. pitch and instrument). The "Note blocks" section mentions the number of skips until the next note/tick, but not what happens when it does find the note block:

Step 3: Byte: Note block instrument
The instrument of the note block. This is 0-9, or higher if the song uses custom instruments.
0=Piano (air), 1=Double Bass (wood), 2=Bass Drum (stone), 3=Snare Drum (sand), 4=Click (glass), 5=Guitar (wool), 6=Flute (Clay), 7=Bell (Block of Gold), 8=Chime (Packed Ice), 9=Xylophone (Bone Block)
Step 4: Byte: Note block key
The key of the note block, from 0-87, where 0 is A0 and 87 is C8. 33-57 is within the 2 octave limit. After reading this, we go back to Step 2.

Since there were no changes to that, it's just a matter of copying it to the new page.

@HielkeMinecraft
Copy link
Collaborator

Oh whoops thanks for telling me, I'll update the docs.

If there's anything else you wish the .nbs format has, now is the time to suggest it. (I sent you a message on Twitter because imo it's better to chat, but you can send it here too)

@Bentroen Bentroen added the S: Resolved in next release This bugfix or suggestion has been applied to the code label Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Bug Something isn't working S: Resolved in next release This bugfix or suggestion has been applied to the code
Projects
None yet
Development

No branches or pull requests

2 participants