ssj71 / rkrlv2

Rakarrack Effects Ported to LV2 Plugins
GNU General Public License v2.0
53 stars 10 forks source link

Fixes to convertbank.py and rkrremap.py #30

Open Stazed opened 7 years ago

Stazed commented 7 years ago

The main issues are...

convertbank.py typo for otrem "44" vs "45" added the gate

rkrremap.py EQ and Parametric EQ -have that confusing indexing, but Gain & Q (EQ) and Gain (Parametric) are last in the bank file.
Compressor, Expander, NoiseGate (off by 1 - parameter index starts at 1, but saved in bank start at 0)

Echo - url typo - "echo" vs "eco"
Valve - url typo - "valve" vs "Valve"

Added the regular Phaser (replaced Analog)
Added NoiseGate

Cabinet - parameters reversed.

Coil Crafter - skip origin and destinations in the rkrlv2.C

All the remaining issues were plus(+) or minus(-) issues, I think...

In case you run the converter and try loading the .carxp files in carla, there will still be some parameter errors in some cases. This is because I found some mis-alignments in the rkrlv2.C file that I have not sent you yet...

Thanks for doing this conversion, it was a tremendous time saver!!!

ssj71 commented 7 years ago

The #eco typo is deliberate because that URL actually resolves to the documentation for the echo effect. The user never really sees these so I just went ahead and kept the error. Changing URI is a big no-no, anybody using these in a project will update and find all their old projects broken.

Since I haven't ever done a full release we can negotiate about it, but really I don't see a strong argument for changing. They are mostly behind the scenes stuff that users don't see. I was planning for those sort of changes to happen in https://github.com/rkrv2/rkrv2.

Regarding the EQs parameters, is the problem that in the bank file the parameter order is reversed? If so we should read it and just remap it. Unless I'm misunderstanding we shouldn't be changing the port order from what shows in the TTL. I know they're confusing, but it was set that way so that it could be merged upstream into rakarrack without having to change their UI to the new mapping. And now that that is set it shouldn't change. Thats another thing that could break peoples projects using these. So I think for example eql should change like:

- (0, 0) : (1, 'Gain', 'GAIN', 64) ,
-         (0, 1) : (2, 'Q', 'Q', 64) ,
-         (0, 2) : (3, '31 Hz', 'HZ31', 64) ,
-         (0, 3) : (4, '63 Hz', 'HZ63', 64) ,
-         (0, 4) : (5, '125 Hz', 'HZ125', 64) ,
-         (0, 5) : (6, '250 Hz', 'HZ250', 64) ,
-         (0, 6) : (7, '500 Hz', 'HZ500', 64) ,
-         (0, 7) : (8, '1 kHz', 'KHZ1', 64) ,
-         (0, 8) : (9, '2 kHz', 'KHZ2', 64) ,
-         (0, 9) : (10, '4 kHz', 'KHZ4', 64) ,
-         (0, 10) : (11, '8 kHz', 'KHZ8', 64) ,
-         (0, 11) : (12, '16 kHz', 'KHZ16', 64) ,
+         (0, 0) : (3, '31 Hz', 'HZ31', -64) ,
+         (0, 1) : (4, '63 Hz', 'HZ63', -64) ,
+         (0, 2) : (5, '125 Hz', 'HZ125', -64) ,
+         (0, 3) : (6, '250 Hz', 'HZ250', -64) ,
+         (0, 4) : (7, '500 Hz', 'HZ500', -64) ,
+         (0, 5) : (8, '1 kHz', 'KHZ1', -64) ,
+         (0, 6) : (9, '2 kHz', 'KHZ2', -64) ,
+         (0, 7) : (10, '4 kHz', 'KHZ4', -64) ,
+         (0, 8) : (11, '8 kHz', 'KHZ8', -64) ,
+         (0, 9) : (12, '16 kHz', 'KHZ16', -64) ,
+         (0, 10) : (1, 'Gain', 'GAIN', -64) ,
+         (0, 11) : (2, 'Q', 'Q', -64) ,

Without any changes to rkrlv2.C. Will that work?

Stazed commented 7 years ago

The url for echo in the mainfest and ttl are both:

http://rakarrack.sourceforge.net/effects.html#eco

I just changed to match the actual (typo) url, not the other way around :).

Regarding the eql and eqp you are correct. I was not trying to change the ttl, the bank file is reversed. The reason I did not catch this error, was that my way actually works to correctly load in carla!!! I tested your correct way and it works as well! Very confusing!. Am I correct in guessing the host loads by symbol match, not index? Would make sense since the ttl index value is different from the rklv2.C index value.

eqp should be adjusted as well.

ssj71 commented 7 years ago

Doh! Sorry about misreading the diff on the URI. It seems carla disregards the index anyway, but yeah, lets make it right. Thanks for continuing work, sorry I'm a little picky, but even though I don't have time to do this myself right now I do just want to make sure we aren't messing up stuff for the users. :)

we should probably add a comment in the remap script that the dictionary format is (effect#,bank param index):(carla param index (lv2 order), label, symbol, offset) I had to dig quite a bit to remember what those were. :)

Just to verify, so in the bank the first number in the array for a compressor preset is indeed the threshold, and it doesn't have a meaningless number first? When I get to rkrv2 (the next gen) I really want to get rid of all these weird special cases!

Thanks again!

Stazed commented 7 years ago

Just checked the compressor again and the threshold is correct as well as the other parameters. Don't worry about being picky, I am still leaning, I try to test everything, but that does not mean it is correct :)

On another note, you mentioned merging into rakarrack: https://github.com/Stazed/rakarrack.git

Over the weekend I gave it a try to complete your merge. Got it to build and run rakarrack and also pulled in the rkrlv2(from my experimental fork) remaining stuff and got that to build. See the top of the README for the uh.. merged build instructions. I don't have the dsp knowledge to maintain an official version but if it could be of use to you or anyone else feel free to use. I am going to tinker with it for a while and try to clean things up.

ssj71 commented 7 years ago

Hey, thats really great! Since you've got that going, contact the rakarrack team and see if they are interested in merging these changes. When I contacted them they mostly said they were too busy to do the merge themselves but they might be able to review it and do a merge. TBH a lot of what needs to be done has nothing to do with DSP so there's probably a lot more you could do than you might think.

ssj71 commented 6 years ago

Hey Stazed, sorry I haven't been around much. Is this pull request just waiting for me to merge it? I still haven't tested any of it unfortunately. :(

Stazed commented 6 years ago

Well, hmm...

I'm not sure if it makes sense to pull that in or not at this point. Some of the changes were to be dependent on other planned plug parameter alignment fixes. So, it might be closer to correct, but not 100%.

As for the parameter fixes... probably not gonna get to it. The reason is, been working on the merge and found some things to be fixed. The fixes have diverged from the original code for both the lv2 and rakarrack. Enough so, that it does not make sense to revert things back to your base code. Also added the convolotron, and some changes to rakarrack bank files (backward compatible) that required further changes to the converbank.py... etc.

Not sure where that leaves you. If you want to see what has changed, the wip branch is the latest and ongoing...