Add an auto reload feature

[kostoskistefan]

Hello!

My previous pull request added a reload button to the VST plugin which is quite useful I think. Even after having that button, I felt like I wanted this to be more optimized for certain scenarios.

One example scenario:
I'm using my left hand to tap notes on a guitar that is connected to a DAW (Digital Audio Workstation). I use my right hand to tweak some parameter of the circuit. After saving the circuit, I have to switch back to the DAW and click the Reload button. You can imagine how many times this has to be done if someone is constantly tweaking the circuit and testing different values of some components and trying to hear the difference.

Therefore, this pull request implements an auto-reload functionality.

Here's an overview of how it works:

A checkbox is added to the VST plugin that enables the Auto Reload functionality (disabled by default). Checking it instantiates a FileSystemWatcher. When a circuit is loaded, it keeps listening for a write access on that file. Then when you update the circuit in the LiveSPICE application and save it, the VST is notified and automatically reloads the design.

Something to note is that this will automatically reset the potentiometers and other parameters to their default values. I did not do anything regarding this problem, because I saw that there is an existing pull request that will eventually enable dynamic parameters.

Unchecking the checkbox destroys the FileSystemWatcher to free up resources, however small they are.

Another thing to note is when the schematic file is saved from the LiveSPICE application, the OS keeps the file locked for a short period of time after saving it. This caused an issue with the VST, because it is unable to read the file immediatelly. This is remedied by waiting for either the file to get unlocked, or until a timeout has occured (currently set to 1 second). If the file cannot be read, it won't reload the circuit on the VST. So far, I did not encounter any problems with this approach and never got even close to the timeout period, but still, it is something that I wanted to be transparent about. I did not find any alternative implementations of this problem on the internet, so I went with this one.

I tried to keep everything commented so you can check it out and decide if you want to remove the comments.

I made sure to test all possible scenarios that I could think of and everything seems to work as expected regarding the Auto-Reload feature.

However, if you decide to accept this pull request, I have a suggestion. I feel like the UI design of the VST is quite cluttered now, especially after adding the Reload button and Auto Reload checkbox. My proposal is to redesign the UI in a way that will be both convenient to use and less cluttered. I am willing to take this task, but I wanted to firstly consult you, because I don't want to give the wrong impression and make it feel like I am undermining everyone involved in the UI design, because it is good and functional and that is what's important.

Here is how it currently looks after adding the checkbox:

Here's a simple mockup design of how it might look, but any feedback is greatly appreciated. Additionally, I quite like that the schematic is loaded as a background of the VST, so I'd absolutely like to keep that feature, but it's ommited from the mockup just to save some time.

Default view:

Hamburger menu opened:

Settings opened:

These are just quick mockups I created and it doesn't mean that everything is set in stone. Additionally, I cannot make promises about how much time this will take or even if I'm able to accomplish everything, but I will try my best if you approve. Please let me know what you think and what my next steps should be.

[Federerer]

Hi, I remember that I had this implemented locally at some point, because I also hated the workflow 😉 Currently I mostly use ReaRoute Asio driver from reaper with ReaInsert plugin, so I can use full LiveSPICE UI and also use other vst plugins at the same time 😁
Regarding the UI - there is a discussion here: #137, I even started some implementation, but never had time to finish this properly.

[kostoskistefan]

To be honest, I a bit disappointed in myself for not thinking of ReaRoute at all. I totally forgot about it, and this is a perfect use case for it, so thanks for sharing your workflow.

About the UI, I absolutely love your design. I forgot to check the discussions, so I missed this thread, sorry. My main issue with the UI were the Oversample and Iterations settings, which remain mostly constant throughout the workflow and do not need to be constantly visible and take up valuable screen space. But, I can see that you had that taken care of in your design, which is awesome. I'd love to see your updated UI version in some future release of LiveSPICE!

However, I still think that the auto-reload feature might be useful for some people that do not use Reaper and do not want to tinker with "more complicated" audio routing compared to just adding one VST plugin to a track and that being it.

In any case, thanks for enlightening me with the info you provided!

[dsharlet]

Thanks for the PR, and the very thorough description of the change. It makes sense to me and the implementation looks good given the issues you described.

Do you think there is something we could change in the LiveSPICE application that would avoid the need for the timeout/retry logic? We're currently using this API: https://learn.microsoft.com/en-us/dotnet/api/system.xml.linq.xdocument.save?view=net-8.0#system-xml-linq-xdocument-save(system-string). We could use the Stream version of that, open our own FileStream, with some different flags/settings. We could use FileShare.Read to allow the VST plugin to read it. Of course, this is only useful if the file is reliably fully written before the VST plugin attempts to read it, which I'm not sure we could guarantee.

re: Oversample and Iterations: I think we should just get rid of these options everywhere. I think we already decided to do this once already, it's just that testing this well is tricky. I'll try to do another round of testing of this now.

[dsharlet]

Of course, I basically went and re-did #195 just now forgetting that existed. That has more discussion on the topic

[kostoskistefan]

Unfortunatelly, I don't think anything can be done from the applications point of view to fix this issue. It is the OS that does the file locking and no matter which approach you use to write the file, it will still be locked by the OS.

The FileSystemWatcher only listens for a change in the Last Write flag of the file, and can't (as of now) listen to an event that indicates that the file is unlocked.

Here is what the .NET devs are saying regarding this problem:

Source: microsoft/dotnet#437

The problem is that there is an inherent race condition. Even if you were notified as soon as there were no locks, by the time your process tries to open the file, another process may have locked it.

The only safe (atomic) solution is to repeatedly try to open it and catch the IOException that occurs. That way, when the IO exception does not occur, you're already holding onto an open file and no race condition with another process is possible.

And you can't close the file and reopen it in subsequent code, or you're back to race condition land. In that case you need to pass the FileStream around your code, not the path.

There still is an open Issue about this problem, which is marked to get fixed in the future, but for now, we are stuck with manually checking whether the file is locked or unlocked.

That being said, after I read the quoted text above, I realized that my implementation doesn't follow this rule:

And you can't close the file and reopen it in subsequent code, or you're back to race condition land. In that case you need to pass the FileStream around your code, not the path.

This is probably okay for LiveSPICE, because no other application will try to write to the schematic files, but in any case, just to be on the safe side, I will try to rewrite the code and update this PR.

Edit:
I also forgot to give my opinion about the Oversample and Iterations parameters. Personally, I find them useful. I also use LiveSPICE on an older system where those two parameters are incredibly useful for achieving real time audio processign with a larger circuit design loaded. Yes, the quality is not as good with lower values, but it is definitelly usable. Therefore, if you've found a way to automate those parameters optimally, I think that that should be absolutely done. Or maybe leave the oversampling parameter and remove the iterations as mentioned in your PR.

Edit 2:
I just saw your newly opened PR #218, and I might've spoken too soon in my previous edit. When that PR gets merged, I'll test the VST and share my experience with it. Thanks!

[dsharlet]

This claims that File.Replace will use ReplaceFile, which is atomic. So, LiveSPICE could write its file to some temporary file, then use File.Replace to atomically update the actual file being observed by the VST plugin. Maybe that would avoid any failed reads? I'll send a PR to use that approach in case you want to experiment.

re: Oversample and Iterations: I think the question is more do you actually observe a better quality result from high oversampling factors? I can't hear anything... but my audio equipment in general is not very high end, so maybe I can't really test this properly. I also can't see anything visually in signal visualizations, but that's probably not a reliable test. #218 is basically assuming that Oversample = 1 and Iterations = huge is always the best thing to use, if the quality impact is negligible and the performance is going to be best.

[dsharlet]

That was a bit more annoying than I expected, the APIs for doing this in .NET are surprisingly new given the age of the functions they wrap. But if you're up for it, maybe give #219 a try?

[kostoskistefan]

I will perform some tests with the #219 PR and report the results.

About the Oversampling and Iterations, I will reply on the other PR, because it has practically nothing to do with the Auto-Reload functionality discussed in this PR.

[kostoskistefan]

I did some testing with PR #219 and it seems to be working better now. However, I can see that your PR provides a fallback approach where the file is saved in a non-atomical fashion. This can still cause an issue with the file being locked by the OS. So here is what I did:

• Instead of only listening to the LastWrite event, now the FileSystemWatcher listens to the Rename event as well. If the atomic ReplaceFile succeeds, then the circuit is simply reloaded without the need for any other additional checks. If it fails, then the file would've been saved using a non-atomic approach and it will trigger a LastWrite event. In that case, we still need to check for the file lock.
• The previous implementation of the Auto-Reload functionality had a bug I mentioned a few comments before, so now, if the LastWrite event is triggered, the file needs to be reloaded from an already opened FileStream, because closing the FileStream and then reading the schematic from a given path may introduce a race condition. For this, I had to add a Load function that takes a file stream instead of a file path. I basically copied the existing load functions and changed the parameter.
• Now that the Rename event is watched, I've added a minor detail to the code, where if someone goes and manually changes the name of the loaded schematic (from file explorer), it will be updated in the VST.

I tested everything I could think of and I couldn't manage to get an error, which is great. Thank you for providing the atomic save code!

[dsharlet]

I added the fallback out of an abundance of caution. I basically only did it to avoid throwing my own error, i.e. I expect the fallback to fail if the MoveFile call fails. I updated the comment accordingly. I think the only reason to do this is if it means we can simplify the VST code. If the VST code still needs the old codepath anyways (especially since it will almost never run, and is at risk of being broken without anyone noticing), then it isn't worth doing this.

[kostoskistefan]

Okay, now this clears some things up. If the fallback is never actually going to be called, then that means that a significant chunk of code can be removed from the VST plugin and there won't be any workarounds used at all. Basically, the FileSystemWatcher will only listen to Rename events (which occur when the MoveFileEx function is called) and it will simply reload the schematic without any additional checks. I will update the code and push it to GitHub.

[dsharlet]

This looks good to me, thanks again for the PR. I'm going to merge this now.