MarcBoule / ImpromptuModular

Virtual Eurorack Modules for VCV Rack
Other
93 stars 10 forks source link

Part module ignoring non-C based split notes. #45

Closed pgatt closed 2 years ago

pgatt commented 2 years ago

Hi Marc,

I've tried to use Part to get a Left Hand Right Hand thing going on in VCV2. It's fine when set to C4 as the split note but when you change the split note to something other than C4, C4 remains the split note. Weirdly I have found the same bug in LeftHandRightHand by RJModules and KeySplit by JLMod which do the same thing.

On Mac running the most recent beta.

MarcBoule commented 2 years ago

Hi Paul, It's hard to see what's going on in your use case, but I've just tried it now and it seems to be splitting properly, from what I can tell. Here's the small test I did splitting on G5#:

image

If you can't get this example to work, can you send me your patch? Cheers, Marc

pgatt commented 2 years ago

Yeah your patch works sorry, I hadn't worked out what was breaking but why can't I plug a v/oct into the split input? The module takes the voltage input and displays it on the dial, but then the split only happens on C notes.

https://www.dropbox.com/s/kung44n19oem4gq/Part.vcv?dl=0

https://www.dropbox.com/s/9zp6x79crg2uafg/Part%20-%20Screenshot.png?dl=0

MarcBoule commented 2 years ago

I think I see what you mean, the Split input is a CV input for the split point, and although the CV adjusted split point is reflected on the display, the actual split point used internally does not seem to take it into account. I'll look into this, thanks!

pgatt commented 2 years ago

Awesome, I'm not going mad. However still weird that the same bug is effectively across three modules from three developers if I'm right.

MarcBoule commented 2 years ago

This is now fixed. If you build plugins, please feel free to pull and test. If not and you're on Windows, I can drop a new build in the relases, if you're on other platforms, I can do this also, but it will take longer. Cheers! P.S. If the same bug is in the other devs' modules, than either they had the same brain cramp as I did, or they copied my code, haha!

pgatt commented 2 years ago

I have built and it "works", but there seems to be an issue with accuracy now. Might be a rounding error or similar. I'm using Reftone to set the split point - but sometimes the note I've set is the first note in the high part, other it's the highest note in the low part. Is the behaviour not meant to be that the selected note is always one or the other to be the most useful?

MarcBoule commented 2 years ago

I believe it's indeed related to rounding, since the comparison is mathematically strict in the code, so any inconsistency is likely not caused by the code but by the exact signal value the module is getting.

https://github.com/MarcBoule/ImpromptuModular/blob/master/src/Part.cpp#L149

The easy cases are notes like C4, C5 which are interger voltages, but depending on how other modules consider the other notes, float-wise, it may affect how the comparisons happens.

image

For example, if one module considers C4# to be 0.0833 volts exactly, and another one considers 0.083333, then things could appear different for both.

pgatt commented 2 years ago

Couldn't you round both the same way before you calculate?

MarcBoule commented 2 years ago

There is no actual roundig in my code, I meant how other modules treat note voltages. My code is a simple "greater or equal" on voltages. One voltage is the CV input, and the other voltage is the Split Knob + Split input.

Here are the two lines of code that do this:

float getSplitValue() {return clamp(params[SPLIT_PARAM].getValue() + inputs[SPLIT_INPUT].getVoltage(), -10.0f, 10.0f);}

bool isHigh = inputs[CV_INPUT].getVoltage(c) >= getSplitValue();

There's literally nothing I can change to my code, it's just a ">=" comparison, that's it. Or do you mean I should actually do some rounding?

pgatt commented 2 years ago

Ah sorry I didn't know it wasn't that easy. Maybe, you should actually do some rounding, I'm not sure, but I mean it would be nice to be able to predict that the note you select as the split point is always the highest note of the low part or always the lowest note of the high part but not randomly one or the other.

MarcBoule commented 2 years ago

Agreed, I'll have to think about this more, since it's not clear that the problem is with my module.

I added a debug print of the exact voltage on the Split CV input, just to test how Reftone is behaving, and something does not look right with it. Here are two examples:

image

image

In the first example, the voltage should be exactly -2.000000 volts, and in the second example, it should be exactly +3.000000 volts, so perhaps the problem you are experiencing comes from that module? But I get what you are saying though, if I were to add some code that disregards these small differences in voltage and make it locks on the "right" note regardless of these imprecisions, then you would likely get the consistent behavior you seek. I'll have to evaluate this to see if it's a good idea and if it has any unforseen side-effects, and also to see if it would instead be better left for Matt to tweak his module.

MarcBoule commented 2 years ago

In the mean time, one trick you can do to get consistent behavior is set the Split knob's value to 1mV greater or lower than your intended setting, So if the split knob is at 0 in your patch, then set it to 0.001 like this, and I'm going to bet it will fix the issue:

image

This would be a neat way to keep my pure comparison exact (as it is now in the code), while also circumventing any issues arising from imprecise modules from other devs.

pgatt commented 2 years ago

Yep, that all sounds good to me. I have sometimes ran the voltage through a quantizer for stuff like this too - just force the voltage to play nice, but it would be nice if I didn't have to.

MarcBoule commented 2 years ago

Ok, after more thinking, I think the best solution is to have that 0.001V (1mV) delta applied automatically in the PART module, such that decisions it makes are not influenced by miniscule differences in voltages caused by other modules.

image

By default the option is on, but it will be kept off for any loaded patches just to not affect existing patches.

If you can give it a spin, please let me know if ever you see anything misbehaving with it. With that option activatged, you should now get consistent splits.

So I think I'll close the issue, but feel free to reopen if you see any problems. Thanks for the bug report, I think we improved the module :-)