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.

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

Feature / New "Reverse" probability (for sequenced and arpeggiated notes) #3314

Merged

Conversation

soymonitus
Copy link
Contributor

@soymonitus soymonitus commented Jan 25, 2025

Reverse Probability (RVRS): It applies a chance of inverting the Reverse sample setting just for the current note to be played. This probability only affects the oscillators whose type is set to Sample. Note: you can find this parameter also at the root level of the sound menu, under Randomizer, because this parameter affects both sequenced notes and arpeggiated notes.

This PR also contains this:

  • Fixed midi follow XML generation, where Sidechain Level parameter was not correctly mapped
  • Even more cleanup of arpeggiator code
  • Expose these params for non-arp notes: Velocity Spread, Note Probability, Reverse Probability, Lock randomizer
  • Now, when Lock Randomizer is ON, the sequence is reseted when clip starts from the beginning (or when it loops), rather than restarting on every chord change (which was very limiting)
  • Removed outdated strings
  • Reordered arpeggiator params in automation view menu so they have the same order as in the sound menu
  • Make the Global Randomizer menu be an "horizontal" menu too, accessible by doing shift+Velocity shortcut
list

Copy link
Contributor

github-actions bot commented Jan 25, 2025

Test Results

108 tests  ±0   108 ✅ ±0   0s ⏱️ ±0s
 16 suites ±0     0 💤 ±0 
 16 files   ±0     0 ❌ ±0 

Results for commit ad78107. ± Comparison against base commit 43da502.

♻️ This comment has been updated with latest results.

…synth and kit affect-entire
# Conflicts:
#	src/deluge/gui/views/audio_clip_view.cpp
@soymonitus
Copy link
Contributor Author

Found the crash and fixed it. This is ready for review!

@soymonitus
Copy link
Contributor Author

The person that reviews this: please take special attention to the places where i have replaced "reversed" with isReversed() AND the places where i have not replaced it. I think i made the right choices but maybe some more experimented dev can find something else

# Conflicts:
#	src/deluge/processing/sound/sound.cpp
# Conflicts:
#	src/deluge/gui/views/automation_view.cpp
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy has made some suggestions. Please note that they are machine-generated, and you should review them carefully before applying to avoid introducing bugs.

There were too many comments to post at once. Showing the first 25 out of 73. Check the log or trigger a new build to see more.

src/deluge/gui/menu_item/arpeggiator/note_mode.h Outdated Show resolved Hide resolved
src/deluge/gui/menu_item/arpeggiator/octave_mode.h Outdated Show resolved Hide resolved
src/deluge/gui/menu_item/arpeggiator/octaves.h Outdated Show resolved Hide resolved
src/deluge/gui/menu_item/arpeggiator/randomizer_lock.h Outdated Show resolved Hide resolved
src/deluge/gui/ui/menus.cpp Show resolved Hide resolved
src/deluge/model/sample/sample_controls.cpp Show resolved Hide resolved
src/deluge/model/sample/sample_controls.h Outdated Show resolved Hide resolved
src/deluge/model/voice/voice.cpp Show resolved Hide resolved
src/deluge/model/voice/voice.cpp Show resolved Hide resolved
src/deluge/model/voice/voice.cpp Show resolved Hide resolved
@soymonitus
Copy link
Contributor Author

Updated loopy pro template and docs. Ready for review

@@ -29,5 +29,8 @@ class Octaves final : public Integer {
bool isRelevant(ModControllableAudio* modControllable, int32_t whichThing) override {
return !soundEditor.editingGateDrumRow();
}
void getColumnLabel(StringBuf& label) override {
label.append(deluge::l10n::getView(deluge::l10n::built_in::seven_segment, this->name).data());
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can't do this, string view data is an array of characters and not a null terminated string

More realistically you could refactor string buf append to take a string view pretty easily - it just needs the signature changed and then an update to use strncat with the view length instead of strcat

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try to implement that suggestion

Copy link
Collaborator

Choose a reason for hiding this comment

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

See review below, tldr just use l10n::get

Copy link
Collaborator

@stellar-aria stellar-aria left a comment

Choose a reason for hiding this comment

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

Minor nits, overall logic seems sound, although I didn't do a deep-dive

src/deluge/gui/menu_item/arpeggiator/octaves.h Outdated Show resolved Hide resolved
src/deluge/gui/menu_item/arpeggiator/randomizer_lock.h Outdated Show resolved Hide resolved
tests/spec/string_spec.cpp Outdated Show resolved Hide resolved
src/deluge/modulation/arpeggiator.cpp Outdated Show resolved Hide resolved
@m-m-adams m-m-adams added this pull request to the merge queue Feb 15, 2025
Merged via the queue into SynthstromAudible:community with commit a495e10 Feb 15, 2025
6 of 7 checks passed
@soymonitus soymonitus deleted the monitus/arp_reverse_prob branch February 16, 2025 10:29
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