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

request: VGM pattern #180

Open
ghost opened this issue Oct 24, 2023 · 9 comments · May be fixed by #294
Open

request: VGM pattern #180

ghost opened this issue Oct 24, 2023 · 9 comments · May be fixed by #294

Comments

@ghost
Copy link

ghost commented Oct 24, 2023

Hi

I would like to request a VGM pattern (video game music)
You can see the header here

https://vgmrips.net/wiki/VGM_Specification

I'm a total beginner and can't make this myself :(
I hope somebody can create this pattern (for latest VGM version)

@itsmeow
Copy link
Contributor

itsmeow commented Oct 26, 2023

I think a lot of pattern-writers are/were beginners once! I'd suggest taking a look at some other simple patterns to learn how it works and modify it as you go. I was able to make a basic one with no prior experience other than the documentation and the existing pattern files.

@paxcut
Copy link
Contributor

paxcut commented Oct 26, 2023

I looked at the specification for VGM and that's a fairly complex format. For what purpose do you need the ImHex pattern?

@ghost
Copy link
Author

ghost commented Oct 26, 2023

I looked at the specification for VGM and that's a fairly complex format. For what purpose do you need the ImHex pattern?

Exactly what you say: it's complex and i want to learn it using a pattern for imhex.
So that's why i'm interested in a pattern.

@WerWolv
Copy link
Owner

WerWolv commented Oct 26, 2023

To me the format looks fairly straight forward. It's just large.
My recommendation is to just start slow, maybe write a pattern that just parses the header and some of the data after it. Then once you got more comfortable, implement all the structs whose offsets are stored in the header.

As an example, I'd start like this:

#include <type/magic.pat>

struct Header {
    type::Magic<"Vgm "> ident;
    u32 eofOffset;
    u32 version;
    // ...
};

struct VGM {
    Header header;
}:

VGM vgm @ 0x00;

@paxcut
Copy link
Contributor

paxcut commented Oct 26, 2023

The complexity of the format far surpasses the complexity of the concepts needed to write a pattern in general. The absolute best way to learn a complex format is to write the pattern for it so that you learn all the intricacies of how the data is stored.

To me the format looks fairly straight forward.

Emphasis should be placed in the 'To me' part. To me a simple format is one I can read once and have a mental image of it. I couldn't do that with this one. But I second the recommendation. Learning the basics of pattern language is not difficult at all but rather easy instead.

@applecuckoo
Copy link
Contributor

Hey! I've been working on the general layout of this pattern for a few days and I've posted the result here as a GitHub Gist. It's got quite a few problems, so I'm not going to PR it in. Here are a few problems:

  • First of all, it can't read the version number properly - it's made up of 8 4-bit BCD digits, which doesn't work with type.bcd. This is quite a problem since for most VGM files, the actual data is being misread as the headers from the newer version.
  • For some chips, bit 31 is a 'chip identification bit', while for all chips(?) bit 30 is a dual-chip bit. This isn't being read at the moment.
  • The Gd3 (metadata) tag at the end of the file doesn't seem to work - all the values are 'runtime error: No variable named 'data' found'.
  • Also, naming convention is probably quite bad - I don't know how to make all those variables pretty, lol.

@paxcut
Copy link
Contributor

paxcut commented Jul 26, 2024

it shouldn't be too hard to implement your own decoding based on what you said. the one the imhex defines works on 8 bits per char and you want to use 4 bits instead. this is the code it uses. can it be modified to handle the particular packed version you want to use?

   struct BCD<auto Digits> {
        u8 bytes[Digits];
    } [[sealed, format_read("type::impl::format_bcd")]];

  fn format_bcd(ref auto bcd) {
            str result;

            for (u32 i = 0, i < sizeof(bcd.bytes), i += 1) {
                u8 byte = bcd.bytes[i];
                if (byte >= 10)
                    return "Invalid";

                result += std::format("{}", byte);
            }

            return result;
        };

I don't understand what the problem is with bits 30 and 31. All you mention is that it is not being read. To read any bit usually one creates a mask to and (&) the value as a way to check, but it also possible to use bitfields in a more direct approach.

As for the last problem I'm not sure what the error message means, there is no variable named data and without a test input file to try to run the pattern it is hard to track down where the error may originate. Can you supply one or suggest a source for it?

@applecuckoo
Copy link
Contributor

applecuckoo commented Jul 27, 2024

@paxcut I've been using this file for testing - you should just be able to get it by clicking the 'Donload' button (no, that's not a typo).

I might just leave the dual-chip and chip selection bits, since they aren't essential to decoding the file.

As for the packed BCD situation, it would probably require a bit of a different approach, since I'd have to use bitfields instead of u8 values. I might come back to this in the future once I'm more comfortable with pattern writing.

@applecuckoo applecuckoo linked a pull request Aug 28, 2024 that will close this issue
@applecuckoo
Copy link
Contributor

Finally managed to blast through all of this - the final result is in PR #294. The version detection works now, and that issue with the null strings in the metadata has since been fixed in 8f1f491.

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 a pull request may close this issue.

4 participants