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

Channel stripping for YM2612 (and SN76496) #20

Merged
merged 4 commits into from
Apr 4, 2022

Conversation

meyertime
Copy link
Contributor

See #19.

I followed @ValleyBell's instructions to fix the channel stripping for SN76496 and implement it for YM2612. I tested it with some Genesis .vgm files (from Sonic 1), and it is working.

There's only one problem, and I'm hoping to get some guidance. The command line help text indicates that specifying something like -Strip:YM2612:DAC should work for stripping the DAC, but I could not find where that information gets put in ChnMask. Instead, at this point, -Strip:YM2612:6 strips the DAC. Any insight?

@ValleyBell
Copy link
Contributor

DAC as "channel 6" is fine. It looks like the "DAC -> channel 6" assignment is something I never did.

It is supposed to go here:

else if (CurChip == 0x05 && ! stricmp(ChipPos, "Mem"))

else if (CurChip == 0x02 && ! stricmp(ChipPos, "DAC"))
{
	TempChip->ChnMask |= (1 << 6);
}

@meyertime
Copy link
Contributor Author

Ok, I made the change, and the DAC parameter works now. I also made a couple refinements to the filtering.

I'm pretty sure it filters things accurately, based on my reading the manual and the .vgm files that I tested with. A couple footnotes:

  • If timers are not needed in .vgm files, we could also strip 24H-26H.
  • Depending on how fancy we want to get, we could also strip part of certain registers, but we would have to change the design to allow modifying the data in some cases instead of just stripping out the entire command. However, I don't think it would affect playback at all. For example:
    • 27H contains both timer stuff and channel 3/6 mode. We need to preserve it for channels 3 and 6, but we could remove the timer bits from the data.
    • Part II B6H contains both stereo output control and some LFO information for channel 6. The stereo output control also affects the DAC. According to the manual, this is the only information related to channel 6 that affects the DAC. If stripping channel 6 but not stripping the DAC, we could remove the other bits besides the stereo output control from this command.

@ValleyBell ValleyBell merged commit 0384d94 into vgmrips:master Apr 4, 2022
@ValleyBell
Copy link
Contributor

looks good!
I may remove the "strip register 2B for channel FM6" though. If you strip "FM6" it may strip the DAC enable write as well, disabling the DAC entirely.

Re timers: Those are removed by vgm_cmp already. I should fix that though, because if CSM mode gets enabled, Timer A does have an effect.
If you would want to go advanced and do register modifications, I'd split the stripping code into a separate tool.

YM2612 FM6/DAC handling in general can get difficult, because you can switch between FM6 and DAC whenever you want. So removing data from register B6h can only be done when you can guarantee that no DAC -> FM6 switch occours.

@meyertime
Copy link
Contributor Author

Regarding register 2B, it should only strip it if both FM6 and DAC are being stripped. My thinking here is that if neither FM6 or DAC is used, there's no reason to care about DAC enable. However, I would of course defer to you, as I'm sure you have much more knowledge of the YM2612 than I do.

It also just occurred to me that we could potentially strip register 22 if all FM channels are stripped (leaving only the DAC), because the LFO is not used by the DAC.

But I'm not worried about it, or doing the more advanced stripping. It works for my purposes, as I'm just trying to isolate each individual channel in its own .vgm file so I can render them separately. The resulting file may still have a little unnecessary information, but it functions as I need it. I put the comments the PR in case anyone else wanted to pick it up in the future for any reason.

Thanks again for your help on this!

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