Add ladder effect - Githubissues

jotego / jt12

FM sound source written in Verilog, fully compatible with YM2612, YM3438 (JT12), YM2203 (JT03) and YM2610 (JT10)

GNU General Public License v3.0

120 stars 22 forks source link

Add ladder effect #65

Closed birdybro closed 2 years ago

birdybro commented 2 years ago

This adds the ladder effect the same way @Kitrinx did in the Genesis MiSTer core. Hopefully this addresses this issue --> https://github.com/jotego/jt12/issues/63

In the case of the Genesis MiSTer core, the correct way to hook up the sound to toggle this is like so:

A toggle in the configuration string:
Instantiating the ports into the core's top-level module and (in this case) adjusting the audio balance to match MDFourier:

Submitting PR in hopes of resolving the issues cited here --> https://github.com/MiSTer-devel/Genesis_MiSTer/issues/209 as my PR (https://github.com/MiSTer-devel/Genesis_MiSTer/pull/210) was rejected because the upstream should be updated instead.

jotego commented 2 years ago

I have to look into this commit more carefully. It seems a bit radical. Why eight instances of the accumulator module? The whole JT12 design is pipelined and now there are eight parallel instances?

birdybro commented 2 years ago

@Kitrinx might be able to explain in more detail. As long as ladder is not used the audio output sounds identical to me. I was unable to use your verilator vgm examples since they appear to be outdated.

I should have opened this as a draft btw, I forgot to do it that way :p

Kitrinx commented 2 years ago

I can't really remember this PR as I did it nearly 3 years ago. I will have to look it over, but at the time I think it was idiomatic for the design.

birdybro commented 2 years ago

https://github.com/MiSTer-devel/Genesis_MiSTer/commit/195d16cad5e1a708a27fc0323531b2e4c7482f0d - Here's the original commit 3 years ago for reference, but some code on the Genesis side related to that has changed since then.

birdybro commented 2 years ago

Regarding the 8 instances, just checked with Kitrinx and I guess you mean this:

This looks like 6 instances instead of 8, which coincides with the left and right FM ladder channels being [4:0].

The changes simulate a cross channel interference that happens on real hardware. If there is a better way to implement the ladder effect, then I won't have any hard feelings if you choose to do it another way. Just trying to help bring it back to the MiSTer Genesis core since it's a critical part of the sound of the system to us audiophiles.

jotego commented 2 years ago

Thank you. I have to spend some time reviewing it.

birdybro commented 2 years ago

Any help I can provide, any additional testing you need done? :)

birdybro commented 2 years ago

Updated PR to match downstream changes proposed here --> https://github.com/MiSTer-devel/Genesis_MiSTer/pull/216. Conflict is only a result of this PR for syntax changed not being merged --> https://github.com/jotego/jt12/pull/66