Implementing small feature - best practices, git guidelines...?

[BURN-MICROSUCK]

I'm in the process of adding a small but powerful feature to the 'analog phaser' effect. It's essentially a tick box called "static LFO" which when enabled, makes the 'frequency' slider directly control the LFO position instead of the frequency (which stays at zero). I've been using this trick for decades on a Behringer unit (only phaser i've used which has this feature) to make killer tone filters that really beef up (or can even sometimes replace) a speaker cab sound.

It could also be used to create some kinds of wah-like effects by mapping a midi control to the LFO position parameter. The Boss GT-PRO has such wah models that are phaser/flanger based, and they sound pretty similar to what could be achieved with this.

I'd like to ask if there are any 'code of conduct' guidelines/rules for writing the code, i can see the code in aphaser.{h,C,c*} is pretty hectic in its indentation and style, so i'm not expecting something too strict there. How about submitting patches / pull requests, what would be the best way to do this when this feature is functional?\

EDIT:

Looking at the LFO code (src/EFX_common/EffectLFO.{h,C} it looks like it would make more sense to implement this feature there (ie. add a LFO type "static") than directly in the phaser FX as i'm currently doing. Also the added benefit would be that it can easily be applied to other effects in the same way. What do you guys think?

[Stazed]

I'd like to ask if there are any 'code of conduct' guidelines/rules for writing the code...

There is no formal code of conduct/guidelines.

For FX and other non-UI files the '{}' should be on the same indentation for start and end, and indentation should be 4 spaces (no tabs). For UI files, the changes should be made in fluid and the .cxx and .h files should be manually generated using File/Write Code.

Also, it is very helpful to include code documentation comments to make review easier.

PRs should be made and apply to the wip(work-in-process) branch.

The concern I have is about adding a control parameter. This means changes to the bank save/restore, export/import, presets, MIDI control, UI and LV2 .ttl files. It has been a while since I seriously dug through the code here, but I think adding a control parameter would (sadly) not be backwards compatible.

So, adding to the LFO code certainly looks to be the way to go if it is possible!!!

Regardless, you can always send a PR and if you can get it to work without breaking changes, then it may be accepted (no guarantees).

[BURN-MICROSUCK]

All right, i think i got something working! I took the second approach of adding a new LFO type "static" where the frequency/tempo slider directly determines the (non-oscillating) value returned by the LFO.

This does however require a tiny bit of tweaking for each effect to use this feature, since otherwise the EffectLFO class has no way of knowing what min/max values the UI allows. The effect just needs to set the LFO'S ui_freq_min and ui_freq_max public member vars, and update two LV2 parameters in the "ttl" file - extra "Static" entry and max value for "LFO type" currently from 11 to 12.

This (afaik) doesn't break anything for backward compatibility with bank/preset files, midi etc. I'll do some more testing before cleaning up my code and doing a PR.

[Stazed]

@BURN-MICROSUCK , if/when you submit the PR, please use another github username.

I do not want this project or myself to be perceived as approving messaging of this type. IMO, this is not the place for it...

Thanks

[BURN-MICROSUCK]

I made this account when they bought out github in 2018. Let's just say i'm far from being the only one who wasn't very happy.....

For the LFO code, it's pretty much done, including getting all the effects that use LFO's to enable it. I'm tweaking presets i added to the analog phaser, then a bit more testing and i'll submit!

From my notes:

Changes made for each effect to use the static LFO -- 

- increment TYPE's "lv2:maximum" (currently from 11 to 12) in the .ttl file
for LV2 plugin, and add the "Static" entry with value 12:

                  lv2:symbol "TYPE" ;
                  lv2:name "LFO Type" ;
                  [...]
11 to 12  ->      lv2:maximum 12 ;
                  [...]
                  lv2:scalePoint [ rdfs:label "Triangle Bottomed Sine"; rdf:value 11 ] ;
add line  ->      lv2:scalePoint [ rdfs:label "Static"; rdf:value 12 ];

- if the plugin's code checks for LFO type, increase the max value there too. (currently none)

- if the LFO's slider allows min-max range other than 1-600, it must let the LFO code know
these by setting lfo->ui_freq_min and lfo->ui_freq_max so that the slider spans one full LFO cycle.
(All plugins currently use this range, so again none)

DONE:

- implemented static LFO feature in src/EFX_common/EffectLFO.{h,C} where a "Static" entry is added to
the list of LFO types, through which the LFO's frequency/rate/tempo setting controls the LFO position instead.

- applied to the effects that use LFO:
    APhaser
    Alienwah
    Chorus
    Dual_Flange
    Echotron (2 lfo's)
    MuTroMojo
    Opticaltrem
    Pan
    Phaser
    Synthfilter
    VaryBand (2 lfo's)
    Vibe
    WahWah

- Stereo LFO positions calculated in a "wrap-around" fashion, ie. not dependent on time or
oscillation pattern. For example, if the position that trails behind the other (R if stereo
is positive, L if negative) is within 'stereo width' of max limit, the other one will have
wrapped around to the left instead of changing its direction, whereas normally (at least with
a symmetrical oscillation pattern) when it's at half of 'stereo width' from lower/upper limit,
both values are equal or pretty close. The opposite is also true with leading
position -> lower limit.

- Added example presets (LV2 and internal) to APhaser effect for 4-phase and 8-phase static phaser.

- apply to remaining effects (see "missing" above)

TODO:

- adjust my 2 'aphaser' presets for stereo/corrected code
- document code properly, indent & style as requested by project author/maintainer
- test more thoroughly on all effects

[BURN-MICROSUCK]

So after testing this for a few hours....

Stereo LFO positions calculated in a "wrap-around" fashion, ie. not dependent on time or
oscillation pattern. For example, if the position that trails behind the other (R if stereo
is positive, L if negative) is within 'stereo width' of max limit, the other one will have
wrapped around to the left instead of changing its direction, whereas normally (at least with
a symmetrical oscillation pattern) when it's at half of 'stereo width' from lower/upper limit,
both values are equal or pretty close. The opposite is also true with leading
position -> lower limit.

...this is just annoying. When usign an external source to control the LFO (midi controller, pedal, some external LFO or whatever) it causes an abrupt transition of the R channel from min to max or vice-versa, which is always at some point along the range that depends on cycle and L/R separation. I'm changing it back to the normal way.

[DeimosLabs]

Created this account, now i just need to remember how to create a pull request.....