Skip to content

Unlimited Drum Randomizer#955

Merged
sapphire-arches merged 3 commits into
SynthstromAudible:communityfrom
mweigt:community
Jan 14, 2024
Merged

Unlimited Drum Randomizer#955
sapphire-arches merged 3 commits into
SynthstromAudible:communityfrom
mweigt:community

Conversation

@mweigt

@mweigt mweigt commented Jan 12, 2024

Copy link
Copy Markdown
Contributor

Please test!

Drum randomizer now does not have a limit on number of files to randomize, and is more efficient by pulling code outside of loops.

Also, SHIFT+(random pad) now randomizes ALL non-muted kit rows.

It uses "Reservoir Sampling with k=1" for randomly choosing from a previously unknown number of files in the directory without a second pass. This gets rid of the need to allocate a list of N filenames for choosing from N files.

@m-m-adams

Copy link
Copy Markdown
Collaborator

Hey thanks for the PR! I'm slightly concerned about performance impacts if someone tries to use this while a song is playing, since the 25 item limit exists to prevent hiccups in audio rendering.

While this PR is better designed and removes the need for the file name array, I think the worst case execution could slow down audio rendering. Some of our users seem to keep multiple hundreds of samples in the same folder so that case needs to be handled

Before merging I'd like sanity checks on the max number of files to read. One approach would be a max of say 50 files, and then cheating a bit by starting the next randomization reads from the current sample instead of the first file in the dir

alternatively you might be able to call the audio routine from here in the case of a long loop, but it's better to avoid that if possible since it makes audio execution more difficult to trace

Thanks!

@sapphire-arches sapphire-arches left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! A few style nits, but this looks like a nice improvement overall.

Comment thread src/deluge/gui/views/instrument_clip_view.cpp Outdated
Comment thread src/deluge/gui/views/instrument_clip_view.cpp Outdated
Comment thread src/deluge/gui/views/instrument_clip_view.cpp Outdated
Comment thread src/deluge/gui/views/instrument_clip_view.cpp Outdated
@mweigt

mweigt commented Jan 12, 2024

Copy link
Copy Markdown
Contributor Author

Hey thanks for the PR! I'm slightly concerned about performance impacts if someone tries to use this while a song is playing, since the 25 item limit exists to prevent hiccups in audio rendering.
While this PR is better designed and removes the need for the file name array, I think the worst case execution could slow down audio rendering. Some of our users seem to keep multiple hundreds of samples in the same folder so that case needs to be handled

I tried it with a few hundred sample files, and a number of tracks playing. I turned the volume of the drumset down to only hear the other tracks, and randomized the complete kit. Although it took about a second to randomize, I could not hear any dropouts of the other tracks in this time (TBH, I have no Idea how this can work, but it does). What happens if you have a few thousand files in each samples directory? Good question!

Before merging I'd like sanity checks on the max number of files to read. One approach would be a max of say 50 files, and then cheating a bit by starting the next randomization reads from the current sample instead of the first file in the dir

This is not trivial... because if one were to use only the filename (that is currently stored with the sample), one would have to search the directory for that file entry next time round, which is exactly the operation making the whole thing slow. So one would have to remember the directory offset somewhere (where?) and then go to that offset directly using the low-level fs functions... I will look into it.

alternatively you might be able to call the audio routine from here in the case of a long loop, but it's better to avoid that if possible since it makes audio execution more difficult to trace

I agree that the code is confusing enough as it is...

@m-m-adams

Copy link
Copy Markdown
Collaborator

Ya store the pointer to the SD location, not finding the name again

@m-m-adams

Copy link
Copy Markdown
Collaborator

Disregard my performance comments - the sd dma complete interrupt calls the audio rendering so this shouldn't interfere

Sample streaming should be fine too with the calls to load enqueued clusters

@mweigt

mweigt commented Jan 12, 2024

Copy link
Copy Markdown
Contributor Author

Ok, that explains some things (including why the randomizing gets slower when tracks are playing).

I will look into your and boib's suggestions during the weekend, and mark the PR as draft until then.

Cheers,
M

@mweigt
mweigt marked this pull request as draft January 12, 2024 13:06
@sapphire-arches sapphire-arches added this to the Release 1.1 milestone Jan 14, 2024
@mweigt mweigt closed this Jan 14, 2024
@mweigt mweigt reopened this Jan 14, 2024
@mweigt
mweigt marked this pull request as ready for review January 14, 2024 17:00
@mweigt

mweigt commented Jan 14, 2024

Copy link
Copy Markdown
Contributor Author

OK:

  • I changed the variable names to camelCase
  • Changed the popups to discern between multiple pressed, all active, and single row randomized (in which case the new filename is displayed)
  • Added a null check (which I am not sure is strictly necessary, but better safe than sorry, I guess)
  • Replaced two reference chains by direct get methods (getCurrentOutputType and getCurrentInstrument)
  • Improved some path string manipulations
  • Made chosenFilename a statically allocated char array instead of the dynmic String class, to avoid unnecessary function calls and mallocs in the hot loop (this works b/c FAT32 limits filenames to 255 chars, and yes, I made the array 256 long to have space for the null terminator)
  • Fixed a logic bug: Now, when neither shift is pressed, nor an audition pad is held, we correctly fall through to the normal edit action so the note can be edited
  • I tested it with large folders: randomizing from 250 sample is basically instant, 7200(!) samples can take a second, but still feels usable to me and no crashes or dropouts result (the ui seems to be blocked in this time, though)

From my standpoint this seems suitable for release.

I would be willing to revisit this at a later stage if people discover bugs or lots of people think the performance is too bad
(I do have an idea how to make it faster, but this would be low-level fat hacking that would require some time to write and test - it also would make the randomness a bit less random, which I would like to avoid)

@sapphire-arches sapphire-arches left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of other small tweaks and this is probably good to merge. Thanks again!

Comment thread src/deluge/gui/views/instrument_clip_view.cpp Outdated
Comment thread src/deluge/gui/views/instrument_clip_view.cpp
Comment thread src/deluge/gui/views/instrument_clip_view.cpp Outdated
Comment thread src/deluge/gui/views/instrument_clip_view.cpp Outdated
@sapphire-arches

Copy link
Copy Markdown
Collaborator

LGTM now, nice work!

@sapphire-arches
sapphire-arches added this pull request to the merge queue Jan 14, 2024
Merged via the queue into SynthstromAudible:community with commit d90c0ee Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants