Skip to content

Feature: NOT FILL probability iteration for notes - #994

Merged
m-m-adams merged 9 commits into
SynthstromAudible:communityfrom
soymonitus:monitus/not_fill_probability
Jan 19, 2024
Merged

Feature: NOT FILL probability iteration for notes#994
m-m-adams merged 9 commits into
SynthstromAudible:communityfrom
soymonitus:monitus/not_fill_probability

Conversation

@soymonitus

@soymonitus soymonitus commented Jan 16, 2024

Copy link
Copy Markdown
Collaborator

Going left from Probability: 5%, you go to "NOT FILL", and after that , you go to "FILL" as the last prob iteration.
I solved it by making use of the "prevBase" flag, which makes the real probability stored in the uint8_t to be 128 (like zero + last bit set to 1)
When SyncScaling button is held, the NOT FILL notes are highlighted in red color

Tasks:

  • Setting it for individual notes
  • Setting it for whole rows
  • Make it work also for when you hold several notes

@soymonitus

Copy link
Copy Markdown
Collaborator Author

Ready for review!

@soymonitus soymonitus changed the title Feature: NO FILL probability iteration for notes Feature: NOT FILL probability iteration for notes Jan 17, 2024
@sichtbeton

sichtbeton commented Jan 17, 2024

Copy link
Copy Markdown
Contributor

This is such a great addition.

@m-m-adams m-m-adams 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.

This can be simplified a bit, but generally works and is useful!

&& getCurrentInstrumentClip()->doesProbabilityExist(
editPadPresses[i].intendedPos, probabilityValue,
kNumProbabilityValues - probabilityValue)) {
if (probabilityValue == 0) {

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.

this isn't necessary, it can be handled by the prev base logic. Instead remove the check that probability must be greater than 0 on setting prevBase to true. That simplifies the code so it follows the same logic as prevBase, including hiding NOT FILL when there's no fill notes. I think that makes sense since not fill isn't helpful if there's no fill notes

If I've missed something and that's not desirable, then modify the if condition slightly below from if (probability > 0 && ...) to be if (probability == 0 || ...)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It can't follow the same logic as prevBase because the prevBase logic searches the row for any previous notes with the same probability. The NOT FILL condition doesn't work this way, it must be independent from other FILL notes. You could have just NOT FILL notes in a clip if you wanted, that's why the probability 0 is handled differently. The logic here is to allow to scroll in this order (without checking other notes): 0 ==> 0+prevBase ==> 1.

I will do the change to if (probability == 0 || ...) that you suggested!

editPadPresses[i].intendedPos, probabilityValue,
kNumProbabilityValues - probabilityValue));
if (probabilityValue == 1) {
// From 5% (value: 1) we go down to NOT FILL (value: 0 | 128)

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.

same here

@soymonitus

Copy link
Copy Markdown
Collaborator Author

@m-m-adams I have simplified the logic, you were right. I have compiled and tested again just in case. Everything working as expected for the three cases: holding single notes, whole rows, and holding several notes. Also tested in 7SEG thanks to the amazing on-device emulator! There it shows FILL. (with a dot) for not fill notes

@m-m-adams
m-m-adams added this pull request to the merge queue Jan 19, 2024
Merged via the queue into SynthstromAudible:community with commit 6ecec2b Jan 19, 2024
@soymonitus
soymonitus deleted the monitus/not_fill_probability branch January 20, 2024 15:26
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