Expander deletion crashes

[imDanSable]

Hey Dave,

Some time ago me and gpt 3.5 wrote a framework to deal with expanders with a very similar functionality to your implementation of you mix/offset/send/mute/fade/etc. modules. My implementation is still buggy, lengthy complex and possibly beyond repair, so I figured I'd check out your implementation. It put me to shame. So simple and effective.

When I did some further testing I found that when I remove a module in a chain of expanders, the other modules remain connected. I think you might want to add onRemove to your MixBaseModule and/or MixExpanderModule .

As was discussed on the forum, onExpanderChange is erroneously not called when a neighboring module is deleted, which might be the cause.

When I delete a module in a chain, it either crashes right away on my linux system or a few moments later or not at all on in windows:

It happens at: https://github.com/DaveBenham/VenomModules/blob/282f75361b4f08307750e04bb0299e2244045271/src/MixModule.hpp#L370

Probably related is the issue an issue when I have say the sequence: mix 4, offset, send, pan and I delete pan, a crash in the audio thread occurs at:

https://github.com/VCVRack/Rack/blob/8c6f41b778b4bf8860b89b36d5503fd37924077f/src/engine/Module.cpp#L287

My Expanders keep a signal slot back to the Expandable and send a non blocking thread safe message the Expandable to refresh its list of connected expanders on that particular side.

I'm curious what elegant solution you might come up which I can learn from.

[DaveBenham]

Before I get into any particulars, I have two basic questions. Are you running a stock VCV Rack binary supplied by VCV, or are you compiling your own? Likewise, are you running stock Venom binaries from the library (or one of my releases from github), or are you compiling your own?

[imDanSable]

I experienced it on self compiled vcv rack binaries and plugins and not in a clean patch. Changing to stock on windows and only venom modules I can reproduce a vcv crash when I delete the expander connected to mix4, which leaves the ones to the right of it still connected, and then go into the module browser and start scrolling or typing.

Here I deleted offset.

[DaveBenham]

Bingo! I can now reproduce the problem on both Windows and Mac. Thanks!

I never discovered the issue because I always have "smart rearrangement" enabled, so when I delete an inner expander, the right expanders collapse to the left, and onExpanderChange is triggered which updates my expander pointers, expanders are no longer orphaned, and all is well.

But if "smart rearrangement" is disabled, then a gap is introduced, but VCV is not triggering onExpanderChange so my expander pointers are now bogus, leading to all manner of nasty results - Ouch!

I am pretty sure I can eliminate the problem by eliminating reliance on onExpanderChange, and instead call get[left/right]expander() as needed in base module process() and expander widget step() to populate my pointers. I just worry about what that will do to performance.

[DaveBenham]

Ugh - I made the change and it works! But performance is horrible for the base module as you add expanders :(

A simple chain of Mix4Stereo, Offset, Mute, Pan originally had Mix4Stereo CPU at 0.7% on my machine. After the fix it more than doubles to 1.8%. The expander CPU is virtually unchanged, as it only needs to check during the step() routine.

I want to make the fix conditional - only firing when "smart rearrangement" is disabled. But I can't rely on that because I can't guarantee there will be a module to the right of the deleted module.

But maybe I can add onRemove to the expander module that sets a global flag to enable the fix temporarily (maybe two step() cycles), and then automatically clears the flag again. That should be a negligible performance hit.

[imDanSable]

Not sure if it helps, but if I extend my module with 3 expanders it runs at 0.1, where yours runs at 0.9 on my machine (v2.6.1) I don't use step() (I don't know what it does :) )

I maintain a list of pointers to my expanders in my Expandable, and in Expander::onRemove I update the list of these pointers in Expandable in the Expander::onRemove thread. I first used an atomic and a thread safe signal/slot, but it seems to work without.

Let me know if you want the code. It's not in my repository yet.

[DaveBenham]

My base module CPU usage is expected to jump as I add expanders because the base module does all the work - the expanders do virtually nothing except provide additional parameter values and inputs/outputs to the base module. My base modules directly access the expander(s) - I do not use messaging.

But if I am forced to call get[Left/Right]Expander() within my process(), then performance tanks. Those routines are relatively slow.

For each Venom module that involves expanders (either the base or the expander), I maintain the current left and right expander (NULL if not valid). But that is dependent on onExpanderChange() being reliable.

I now have a solution that does not kill performance. I added a plugin global variable venomDelCnt and for every Venom module an onRemove() that increments the venomDelCnt.

Each module involved with expanders maintains its own venomDelCnt, and if it does not match the global value, then I use get[Left/Right]Expander() and then set the local venomDelCnt to the current value. Otherwise I simply rely on my cached left and right expander pointers, which is much faster.

[DaveBenham]

@imDanSable - Please test out my latest code in dev - v2.7.dev. You can get the binaries at https://github.com/DaveBenham/VenomModules/actions/runs/8166282901. It should solve all my expander problems. I tested on Mac where I was getting crashes like you on linux, and all is good.

The dev version includes 3 new modules, only one of which is documented in dev. The Multi Merge and Multi Split are not yet documented.

[imDanSable]

Yes. It's all good on windows and linux. And with the same performance as before.

Happy to see the multisplit and multmerge. I got these too :)

[DaveBenham]

Great - thanks! You made an important contribution with your bug report and testing!

The Multi Split/Merge are pretty cool - the Swiss army knives of merge/split.

Merge can have any combination of monophonic and polyphonic inputs, all of which get merged to the first occurring patched output. And of course with multiple patched outputs you can have multiple merges.

Likewise, Split can split multiple poly inputs, by default automatically determining how many channels go to each output. But each output port has a context menu to assign a specific channel count, so you can take complete control. If you assign a value to a few outputs, then the remaining input channels are automatically assigned to the other output channels.

[DaveBenham]

The workaround fix will appear "soon" in the library, either as a 2.6.2 bugfix update, or else a 2.7.0 major release with new modules. I haven't decided yet.