Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Preset browser #1383

Merged
merged 23 commits into from
Jul 26, 2022
Merged

Preset browser #1383

merged 23 commits into from
Jul 26, 2022

Conversation

akimaze
Copy link
Collaborator

@akimaze akimaze commented May 22, 2022

Hi this is the most important feature that I missed the most in Radium, so I tried to implement it.

Now it look s like this:

image

How it works

Searching

In order not to create some tag system etc. I adopted a solution similar to the one in LMMS. Presets can be in any number of subfolders. The search searches through all subfolders and returns results that match not only the file names but the folders as well. For example, if we are looking for bass, all files containing the word bass will be displayed, but also all other files that are in a subfolder with the name containing the word bass:

image

In my opinion subfolders are better than the tag system - creating presets is much faster when you only have to enter the filename.

Playing

When you press the left mouse button on the preset a temporary instrument is created and it plays the note selected with the currently checked note button at the bottom of the preset panel. You can change octave by F1, F2 keys. We can also add checkbox to enable/disable this feature. But this feature is essential for me.

Currently only one note is playing but i the feture we can add mode to play chords.

Creating and adding instrument to current track

Simply double click on preset in the tree to add instrument and assign it to current track. Previous instrument is not deleted.
Added instrument name is changed to preset file name.

Showing/hidding preset panel

I added option in menu, by default its visible.

Opportunities for the future

We can add tabs on the top to add samples folders and saved blocks to import to project.

Configuration

Currently Preset Browser searches ~/Radium Presets folder. I tried to add configuration in preferences but I don't know how implement saving this path from QLineEdit. I added Preset browser" tab, but in the future (when there will be samples/blocks tabs) its name should change to "Browser" and there will be more content.

Other bugs

I found a bug that makes a deadlock in instrument deleting, see 5cf3bbb. I don't know this is right solution but after that change preset browser preview preset works always.

Optimization

To enable preset playback I create an instrument and when the mouse button is up I remove the instrument. It is quite slow, but I think you probably know a better solution. Maybe hidden instrument for preset preview. That's too hard to me to make (I don't know Radium sources so well currently).

Performance

Currently, I checked the search performance on a small number of presets, so I don't know how it will work on a large number of presets.

Code

I don't know Radium sources so well currently, so some things may done wrong or not optimal. I tried to write code in your style.

I hope you enjoy this feature. If you want to include it in Radium. Feel free change anything you want.

@kmatheussen
Copy link
Owner

kmatheussen commented May 23, 2022 via email

@akimaze
Copy link
Collaborator Author

akimaze commented May 23, 2022

You just need to click the various presets quickly (I need about 10-20 clicks), and at some point the Radium window will freeze. With this fix (5cf3bbb) it never happened.

@kmatheussen
Copy link
Owner

Everything seems to work fine. It's very good that the browser automatically updates when files are created.

I was a little bit confused in the beginning though since it seemed like the instrument was deleted immediately after it was created when you clicked on them. Of course, it's actually supposed to do that, and you must double-click to create an instrument, not single-click as I did. But it would probably be good if there was an option to create an instrument without it showing up briefly in the mixer. I'll see if I can fix that.

I can also reproduce the deadlock. It seems to be something deeply wrong. Your fix is probably fine though, but I would like to understand what happens.

@kmatheussen
Copy link
Owner

Okay, I understand what happens. I don't know if you studied it, but in case not, it's not so deep problem, it's caused by calling juce::MessageManager::getInstance()->callFunctionOnMessageThread while holding the JUCE_show_hide_gui_lock lock. This causes a deadlock because 'callFunctionOnMessageThread' is not async and the message thread itself can't run anything because it's trying to obtain the JUCE_show_hide_gui_lock lock

The JUCE api for 'callFunctionOnMessageThread' even warns about this deadlock! :-) https://docs.juce.com/master/classMessageManager.html#a554f81587d57127c9cb5be72aced11c1

@akimaze
Copy link
Collaborator Author

akimaze commented May 24, 2022

I was a little bit confused in the beginning though since it seemed like the instrument was deleted immediately after it was created when you clicked on them. Of course, it's actually supposed to do that, and you must double-click to create an instrument, not single-click as I did. But it would probably be good if there was an option to create an instrument without it showing up briefly in the mixer. I'll see if I can fix that.

That would be great, and if it could be possible to add a flag that the instrument should not be saved in the project file. Then we could not delete it on mouse up event and if someone presses the same preset a second time, we could use it again (simply play note).

I was also wondering if it is possible to determine which plugin the preset is using without loading it. Then, if you select another preset that uses the same plug-in, you could only load its state not recreating the whole instrument. But I don't know if it is possible.

@kmatheussen
Copy link
Owner

kmatheussen commented May 24, 2022 via email

@kmatheussen
Copy link
Owner

kmatheussen commented May 24, 2022 via email

@kmatheussen
Copy link
Owner

Here's a more-than-half-finished patch to add option to create invisible audio-instruments. It's not hiding mixer strips and it's not hiding mixer connections, but I'm on my way out and don't have time to finish it, but you can assume it's going to be finished when I get back. :-)

invisibleinstruments.diff.txt

@akimaze
Copy link
Collaborator Author

akimaze commented May 25, 2022

I was trying to speed up preset loading by code like this (only reload plugin state):

    if (isLegalInstrument(presetDemoInstrument)) {
      // try modify instrument
      disk_t *file = DISK_open_for_reading(filePath);
      if (file) {
        hash_t * preset = HASH_load(file);
        DISK_close_and_delete(file);
         
        if (preset && HASH_has_key(preset, "audio")) {
          hash_t* audio = HASH_get_hash(preset, "audio");
          
          if (audio) {
               struct Patch *patch = getPatchFromNum(presetDemoInstrument);
            if (patch) {
                 hash_t* pluginst = HASH_get_hash(audio, "plugin_state");
              if (pluginst) {
                PLUGIN_recreate_from_state((SoundPlugin *)patch->patchdata, pluginst, false);

                int notenum = root->keyoct + 24 + selectedNote(); // default is 36 C2 
                if(notenum>=0 && notenum<127)
                {
                  playnote_id = playNote(notenum, 120, 0, 0, presetDemoInstrument); // startuje granie nuty
                 }
                stopIgnoringUndo();  
                return;
              }
    ...

But with VST3 plugins (Surge XT), the note is not played. But when I open plugin GUI and click in piano key they works. Is something else needed here? VST2 works OK.

@akimaze
Copy link
Collaborator Author

akimaze commented May 26, 2022

OK I found that i need add:

std::this_thread::sleep_for(std::chrono::milliseconds(50));

after

PLUGIN_recreate_from_state((SoundPlugin *)patch->patchdata, pluginst, false);

to hear note, so is there any way to check instrument is ready?

@akimaze
Copy link
Collaborator Author

akimaze commented May 26, 2022

OK now presets loading works a lot faster but there are some todos :

  1. Should I delete hash_t* ?
  2. How to check plugin is ready (currently I added std::this_thread::sleep_for(std::chrono::milliseconds(50));)?
  3. Make 2000 presets and check performance ;)

And make preset preview instrument invisible. I didn't apply your patch because I had modified code. But you can safely commit to this branch or merge :-)

@kmatheussen
Copy link
Owner

Great work! Sorry for late reply.

Should I delete hash_t* ?

No need, it's garbage-collected using BDW-GC.

How to check plugin is ready (currently I added std::this_thread::sleep_for(std::chrono::milliseconds(50));)?

Plugin should actually be ready. Maybe it's a JUCE bug, or I haven't used the JUCE api correctly. I'll check.

Small note to your code: It's extremely important that startIgnoringUndo() is followed by stopIgnoringUndo(), so just to be 100% sure that's done correctly, it's probably better to use the radium::ScopedIgnoreUndo class instead.

@kmatheussen
Copy link
Owner

I can't see anything about this in the JUCE API. I guess it's another VST3 quirk in juce. Do you only have to call 'std::this_thread::sleep_for' after calling 'PLUGIN_recreate_from_state', or do you also have to call it after 'createAudioInstrumentFromPreset'?

@akimaze
Copy link
Collaborator Author

akimaze commented May 31, 2022

I can't see anything about this in the JUCE API. I guess it's another VST3 quirk in juce. Do you only have to call 'std::this_thread::sleep_for' after calling 'PLUGIN_recreate_from_state', or do you also have to call it after 'createAudioInstrumentFromPreset'?

Only in this one place (PLUGIN_recreate_from_state). I suspected that maybe reading state of plugin is made in another not synced thread and I should use a callback or something else to start play note. I think createAudioInstrumentFromPreset do other things after reading plugin state so maybe plugin has enough time to load its state.

However, loading only the plugin state is much faster than creating the entire instrument and plug-in. This makes searching, or rather auditioning, multiple presets (of the same plugin) much more enjoyable. I'm just not sure if other things should be loaded here, such as the state of the radium compressor, filter. Are they also saved in .rec preset?

@akimaze
Copy link
Collaborator Author

akimaze commented May 31, 2022

Small note to your code: It's extremely important that startIgnoringUndo() is followed by stopIgnoringUndo(), so just to be 100% sure that's done correctly, it's probably better to use the radium::ScopedIgnoreUndo class instead.

That should be more safety (I didn't know about that solution before). For the next week, I don't have time to change that. So you can change that if you don't want to wait for me.

@kmatheussen
Copy link
Owner

kmatheussen commented Jun 5, 2022 via email

@kmatheussen
Copy link
Owner

I've pushed the complete "optional invisible instrument" patch to master: dd413aa

@kmatheussen
Copy link
Owner

And here is a better fix for the deadlock: 888cdf7

The reason for the delay is that on macOS only the GUI need some time to be deleted in the background before the plugin can be deleted. If not you get a crash at program shutdown. This is documented in the JUCE api, and I have also experienced it myself. If I remember correctly it's caused by a badly designed API from Apple. So on Linux and Windows your fix is fine, but I want to run the same code as much as possible on all three platforms.

@kmatheussen
Copy link
Owner

(and this: 0c3315c)

@akimaze
Copy link
Collaborator Author

akimaze commented Jun 12, 2022

Hi, I made git rebase, added radium::ScopedIgnoreUndo, reverted my deadlock fix. But I noticed few bugs:

  1. When I click to hear preset undo sometimes is registered (on radium::ScopedIgnoreUndo but also with previous start/stopIgnoringUndo()). It only happens sometimes.
  2. I added deletePresetDemoInstrument() to delete instrument when preset browser is destroyed, it should be also called when someone load or create new song. I didn't found the right place in source code.

Is the hidden instrument saved in project? I think it should not be saved.

@akimaze
Copy link
Collaborator Author

akimaze commented Jun 13, 2022

I found that the unneeded undo is made by PLUGIN_call_me_very_often_from_main_thread() by this two lines:

  if (is_old && s_euds.size() > 0) // || Undo_num_undos() != s_last_undo_num)
    add_eud_undo(s_euds, s_stored_effect_nums, curr_time, s_last_time, s_last_undo_num);

Its made sometimes when preset browser use PLUGIN_recreate_from_state(plugin, pluginState, false);

@kmatheussen
Copy link
Owner

I guess this should fix the undo:

kjetil@tthp:~/radium_presetbrowser$ git diff audio/SoundPlugin.cpp
diff --git a/audio/SoundPlugin.cpp b/audio/SoundPlugin.cpp
index 67d9ac742..dc85902e4 100644
--- a/audio/SoundPlugin.cpp
+++ b/audio/SoundPlugin.cpp
@@ -1493,7 +1493,8 @@ static void add_eud_undo(QVector<EffectUndoData> &s_euds, QHash<instrument_t, QS
       
       if (patch != NULL) {
         //printf("    EUD undo %d: %f\n", eud.effect_num, eud.effect_value);
-        ADD_UNDO(AudioEffect_CurrPos2(patch, eud.effect_num, eud.effect_value, AE_ALWAYS_CREATE_SOLO_AND_BYPASS_UNDO));
+        if (patch->is_visible)
+          ADD_UNDO(AudioEffect_CurrPos2(patch, eud.effect_num, eud.effect_value, AE_ALWAYS_CREATE_SOLO_AND_BYPASS_UNDO));
       }
     }
   }

And temporary instruments are saved to disk, so that needs to be fixed as well.

@akimaze
Copy link
Collaborator Author

akimaze commented Jun 13, 2022

Thanks fixed :)

@kmatheussen
Copy link
Owner

Here's patches to fix saving only visible instruments to disk:

e5dc108
1bce676

Note that I haven't tested when there are invisible instruments in the mixer though.

@akimaze
Copy link
Collaborator Author

akimaze commented Jun 16, 2022

I'm on a short vacation and I'll check/test when I get home.

@akimaze
Play preset only on left mouse button click
8d244a8
@akimaze
Ability to show/hide preset browser.
922bfeb
@akimaze
Ability to set preset root folder, preset browser tab in preferences.
0687e7e
@akimaze
Clean up includes.
5f49ce9
@akimaze
Remove uneeded log.
1dddb45
@akimaze
Faster loading of presets when the previous preset was the same or us…
37bd848 
…ed the same plugin.
@akimaze
Fix typos and comments.
10579e5
@akimaze
Set preset player intrument name to "Preset Preview"
b0a1639
@akimaze
Comment debug.
0b039f7
@akimaze
Use radium::ScopedIgnoreUndo, some comments updates.
0e30958
@akimaze
Fix compilation after rebase.
05265fd
@akimaze
Revert "Fix deadlock when we create instrument during remove another …
2b01f0f 
…one."

This reverts commit 96f3059.
@akimaze
Add deletePresetDemoInstrument() and remove preset instrument on pres…
63a4273 
…et browser destroy.
@akimaze
Quick fix for undo in preset demo instrument.
324ae28
@akimaze
Copy link
Collaborator Author

akimaze commented Jun 22, 2022

Sorry for delay. In the meantime, I installed pkgconf, and it turns out that compilation only works with pkg-config. With pkgconf I get a lot of undefined reference errors from QT. It took me a while to figure out what has changed in my environment.

I rebased the pull request. It seems that the hidden instrument is not being saved. But sometimes the modular mixer is empty.

Steps to reproduce:

  1. Open radium
  2. Click on preset in browser
  3. Open mixer by F7 -> mixer is sometimes empty

But when you change view to "normal" mixer by clicking "M" button you can see all instruments.

@kmatheussen
Copy link
Owner

What happens, at least to me, is that the scrollbars have been moved so that the modules are not visible. So I can just scroll back. I guess this happens because invisible instruments are positioned very far away from the other modules.

@akimaze
Copy link
Collaborator Author

akimaze commented Jul 2, 2022

Yes you are right, the scrollbar range is very large. I didn't notice that the first time.

@akimaze
Copy link
Collaborator Author

akimaze commented Jul 10, 2022

I added removing preset browser instrument, so loading another song or create new song works now :)

@akimaze
Don't use FocusSnifferQTreeView for preset browser tree - let editor …
abf3513 
…keys work, fixes a bug F1, F2 works only when editor is selected.
@kmatheussen
Copy link
Owner

The extremely large mixer scene rect seems to be caused by a quirk/bug in Qt. The scene rect is automatically calculated from the visible items in it, but if the mixer hasn't been shown yet, it's also calculated from the invisible items. This workaround seems to work. I'm not sure if it'll always work though:

kjetil@tthp:~/radium_presetbrowser$ git diff Qt/
diff --git a/Qt/Qt_Main.cpp b/Qt/Qt_Main.cpp
index 04d7618b7..4e2395d70 100755
--- a/Qt/Qt_Main.cpp
+++ b/Qt/Qt_Main.cpp
@@ -2570,11 +2570,21 @@ protected:
         // No, we still need to do this. At least in qt 5.5.1. Seems like it's not necessary in 5.7 or 5.8 though, but that could be coincidental.
         if(num_calls_at_this_point<150/_interval)
           updateWidgetRecursively(g_main_window);
-        
+
+        // Show mixer briefly to workaround a Qt quirk/bug causing SceneRect size to be calculated from invisible items when the scene hasn't been shown yet.
+        // (Fixes extremely large Mixer scene rect if previewing preset before opening the mixer for the first time)
+        {
+          if(num_calls_at_this_point==50/_interval)
+            GFX_ShowMixer();
+
+          if(num_calls_at_this_point==70/_interval)
+            GFX_HideMixer();
+        }
+

@kmatheussen kmatheussen merged commit 1dff78a into kmatheussen:master Jul 26, 2022
@kmatheussen
Copy link
Owner

Thanks! Great work! I'm going to hide the preset browser during startup though, and I'm probably going to move the preferences line edit somewhere else.

@kmatheussen
Copy link
Owner

Added couple of commits:

0df6a0f
1521ad6

@kmatheussen
Copy link
Owner

Fix for preferences:
0b4c48c

@akimaze
Copy link
Collaborator Author

akimaze commented Jul 27, 2022

Great news, thanks for merging :)

I'm going to hide the preset browser during startup though

It would be great if displaying/hiding state of the preset browser would be saved in the configuration file to not to have to turn it on/off every time you launch Radium (depending on the user's preferences).

@kmatheussen
Copy link
Owner

I'm going to hide the preset browser during startup though

It would be great if displaying/hiding state of the preset browser would be saved in the configuration file to not to have to turn it on/off every time you launch Radium (depending on the user's preferences).

Yes, good idea. That scheme would probably fix #1378 as well if done for all windows...

@kmatheussen
Copy link
Owner

Done: 4f52c7b

@akimaze akimaze mentioned this pull request Aug 27, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@akimaze @kmatheussen