lmms icon indicating copy to clipboard operation
lmms copied to clipboard

Add Beat Preview to PatternClipView

Open regulus79 opened this issue 1 year ago • 7 comments

This PR changes the way PatternClips are drawn in include a simple "beat preivew," where each note in any non-empty instrument tracks within the pattern are drawn as short little rectangles on the ClipView. SampleClips and AutomationClips within patterns are not currently supported.

The height of the note boxes changes depending on how many tracks there are in the pattern, and how many of them actually have notes (empty tracks are only drawn half as tall).

There is also some padding at the top and bottom along with a little bit of spacing between each note, both vertically and horizontally. This can be edited in the css, along with the note color.

I am not currently satisfied with the state of the visualization; perhaps some of you may have ideas on how to make it better.

Screenshot From 2025-05-23 20-59-29

Screenshot From 2025-05-23 20-57-51

Todo

  • [x] Fix clip view not updating when note velocity changes ~~- [ ] Add SampleClip support (likely going to be adding a single note rectangle at the sample startoffset)~~ ~~- [ ] Add AutomationClip support (or not)~~
  • [x] Fix graphical bug of horizontal line of empty pixels
  • [x] Fix the drawing of notes to be between the clip highlight, otherwise the notes near the top and bottom botder will be slightly hidden.

regulus79 avatar Oct 23 '24 01:10 regulus79

I'd reopen this. Also, looking at the demo, I'd introduce the spacing between the elements in terms of empty tracks, since it might help with easier identification of what the clip is (ie. if there's some free space at the top, this is the clip that doesn't have drums).

bratpeki avatar May 02 '25 19:05 bratpeki

some of you may have ideas on how to make it better.

I am afraid i need to say that i think this is confusing for the eyes. There are two things that i feel is important here:

  1. We only see one repetitive pattern throughout the whole PatternClipView
  2. There are no inherent info about 'hit' variations (velocity or whilt)
    What is the information that the user need, in that style of PatternClipView , that cant be told in a label?

What we all know is that current patternEditor is inferior. Placing percussion-events directly only on beats, is simply not good enough. The absurdly important swing/ grooving is no possible with that design. A visualisation of that inferiority wont really help.

musikBear avatar May 27 '25 16:05 musikBear

Thanks for your feedback!

There are no inherent info about 'hit' variations (velocity or whilt)

There is actually! I didn't show it in the screenshots up there, but if the note/box has partial velocity, it will show up as partially transparent on the clip view.

image

What is the information that the user need, in that style of PatternClipView , that cant be told in a label?

I was assuming that the user may like to see the shape of the beat from the Song Editor instead of having to check the pattern editor. What kind of label were you thinking of?

Placing percussion-events directly only on beats, is simply not good enough. The absurdly important swing/ grooving is no possible with that design.

The current state of the PR does kind of support off-beat notes. Here I'm doing some triplet stuff and it appears to work okay:

image

regulus79 avatar May 28 '25 01:05 regulus79

There is actually! I didn't show it in the screenshots up there, but if the note/box has partial velocity, it will show up as partially transparent on the clip view.

Yes that is valid info. I am impressed of your work, this is clever, but that info is is only one keypress away already. Toggling the pattern-editor up and down will show the same thing

What is the information that the user need, in that style of PatternClipView , that cant be told in a label?

What kind of label were you thinking of?

Just the name on the Pattern-track like: image

The current state of the PR does kind of support off-beat notes. Here I'm doing some triplet stuff and it appears to work okay:

Again clever indeed. Does i understand this correctly if i think i an looking at a Beat that has been opened in piano-roll? How does the view in Pattern-editor look in that situation? I expect it is something like: image And there i have to say that your representation is definitively superior! Your actually show the spacing.

With these in depth explanations of the functionality i can see that your PR has functional meaning and add better information about the structure in the beat. 👍 I just hope we can preserve the space for labels on the PatternClipView, because text on top of those symbols will be impossible to read. Thanks for clarifying and good job!

musikBear avatar May 28 '25 16:05 musikBear

some of you may have ideas on how to make it better.

I am afraid i need to say that i think this is confusing for the eyes. There are two things that i feel is important here:

1. We only see one repetitive pattern throughout the whole PatternClipView

2. There are no inherent info about 'hit' variations (velocity or whilt)
   What is the information that the user need, in that style of PatternClipView  , that cant be told in a _label_?

What we all know is that current patternEditor is inferior. Placing percussion-events directly only on beats, is simply not good enough. The absurdly important swing/ grooving is no possible with that design. A visualisation of that inferiority wont really help.

Let me disagree tho; this would be automatic, unlike clip labels or track names, and would allow me to more easily recognize a specific drum pattern at a glance.

I personally find it a useful change. FL Studio also shows this kind of preview and it's even less informative there (but it's also pretty different cause everything is a pattern on their end)

immagine

RoxasKH avatar May 28 '25 18:05 RoxasKH

I just hope we can preserve the space for labels on the PatternClipView, because text on top of those symbols will be impossible to read.

That's a very good point. Currently it looks a little like this when you have a label:

image

It does seem a bit harder to read. Do you want me to change it so that it doesn't draw below the label? Or maybe darken the note/beat rectangles to make them contrast better? Or maybe darken the transparent shadow behind the label?

regulus79 avatar Jun 04 '25 22:06 regulus79

It does seem a bit harder to read.

Yes that is an issue.

Do you want me to change it so that it doesn't draw below the label? Or maybe darken the note/beat rectangles to make them contrast better? Or maybe darken the transparent shadow behind the label?

Its difficult. Its not only the text-label that is difficult to read, also the pattern is difficult to see. Nomatter what is done one or the other will 'suffer' I think the best situation will be to at least avoid that both suffer.. The problem could be that both the pattern and the label are white, they 'melt' into each other, and both is then difficult to see. How does a black pattern look, or maybe a dark-blue shade would not break the theme as much as black would.. idk...

musikBear avatar Jun 05 '25 20:06 musikBear

How does a black pattern look, or maybe a dark-blue shade would not break the theme as much as black would.. idk...

Maybe it would make sense to change the pattern color sometime. But if we also change the note/beat rectangle things to be, say, black instead of white to not interfere with the text, we would probably also want to change midi clip notes to be the same color for consistency(?)

I don't know how major of a ui change we want to do

If I make the notes black, it looks like this: image

If I make the label background darker, it looks like this: image

regulus79 avatar Jun 22 '25 15:06 regulus79

If I make the label background darker, it looks like this:

It is the same conundrum again. When the text is made readable the patterns are obscured, and vis-a-vi. Doubt any combo, that will make both pattern and label stand out will be possible. One thing that always has bothered me, is that the label-background covers the complete length of the track. That problem is more annoying with this new pattern-preview, where the shading-bar only is an annoyance that obscures the pattern, without having any function. Its a shame that our UI is hampered with that kind of necessary compromises, but we all know that, and that qT is to blame.

musikBear avatar Jun 23 '25 19:06 musikBear

One thing that always has bothered me, is that the label-background covers the complete length of the track. That problem is more annoying with this new pattern-preview, where the shading-bar only is an annoyance that obscures the pattern, without having any function. Its a shame that our UI is hampered with that kind of necessary compromises, but we all know that, and that qT is to blame.

Actually it turns out that's super easy to fix!

image

Does that look a bit better?

Or maybe a little less dark:

image

regulus79 avatar Jun 23 '25 20:06 regulus79

Looks great imo

bratpeki avatar Jun 24 '25 08:06 bratpeki

Actually it turns out that's super easy to fix!

😱 WHHAUT!! But..... That is That is a MUST add/ improvement nomatter what!! That stupid label-bar has been such a pia and boom -Ýou make a fix! That fix alone is pure gold! And Yes it is easier to see the pattern, not where the bar is, but now it can be seen in the rest of the track! -Thanks to that label-bar fix! Imo, we must add that to release-candidate also if it conflicts with feature-lock! Great stuff @regulus79

musikBear avatar Jun 24 '25 20:06 musikBear

Why make the dark background more dark?

bratpeki avatar Jun 24 '25 21:06 bratpeki

Why make the dark background more dark?

musikBear expressed that the white color of the notes in the pattern clip made it harder to read the white text on the label. https://github.com/LMMS/lmms/pull/7559#issuecomment-2945978461

regulus79 avatar Jun 24 '25 21:06 regulus79

I agree this is a nice addition, but I think it'd be great if this was a separate PR, to allow clear distinction and reverting if it is ever needed. Tagging @messmerd for his opinion. If he's okay with it, all good.

bratpeki avatar Jun 24 '25 21:06 bratpeki

https://github.com/LMMS/lmms/pull/7559#issuecomment-2997860048; you chose the "little less dark" one?

bratpeki avatar Jun 24 '25 21:06 bratpeki

https://github.com/LMMS/lmms/pull/7559#issuecomment-2997860048; you chose the "little less dark" one?

Yes, and I copied that screenshot to the main PR description

Perhaps there is a better way to improve contrast without changing the color so much. I could look into increasing the text shadow, but I'm not sure.

regulus79 avatar Jun 24 '25 21:06 regulus79

Okay, cool, thanks for the clarif! Testing soon!

bratpeki avatar Jun 24 '25 22:06 bratpeki

Maybe they'd look better if they were just a little bit lighter, though I'm not sure.

I can easily make it lighter if you want.

Here's an opacity of 75, which is what we used previously: image

Here's an opacity of 100: image

Here is 120, which is what is currently in the PR: image

regulus79 avatar Jun 25 '25 12:06 regulus79

I can easily make it lighter if you want.

Its surprising how much it matters. Pattern-clip label with 75 opacity is almost not readable The difference between 100 and 120 is marginal, so we could prolly go with 100 as the forementioned 'compromise'

musikBear avatar Jun 25 '25 20:06 musikBear

Found one bug on Discord, it was patched with https://github.com/LMMS/lmms/pull/7559/commits/20ec5d8506d0588c56d3d2a5f1416f4c42bb685a!

Since @messmerd gave the code a thumbs up and now @rubiefawn also did some cleanup and no doubt checked that the code is convention-friendly, I say we get this is in soon.

bratpeki avatar Dec 01 '25 23:12 bratpeki

@bratpeki found the following bug on 2025-06-30 and notified the Discord testing channel. I haven't attempted to reproduce this bug with the latest build, so it may or may not be an issue. Including it in the PR for completeness:

insane find @regulus79, somehow Carla Rack and Patchbay bug out The terminal says: [carla] Carla assertion failure: "numScopedInitInstances == 0" in file carla_juce.cpp, line 85 Maybe because I had no plugins in them?

https://github.com/user-attachments/assets/843b27f0-4244-48a0-bd26-fbd32be4d8c5

rubiefawn avatar Dec 02 '25 00:12 rubiefawn

I was able to replicate the issue with Carla. Turns out this issue is present on the master branch as well. Something about Carla breaks the pattern editor, resulting in several empty MIDI clips being inserted into the PatternStore, which causes affected instrument tracks to be silent, and in this PR causes their contents to not be rendered in the preview. This is visible in an affected save file; example attached (instances of Carla are replaced with OpulenZ):

7559-test.mmp.xml (GitHub has a file type allowlist, just remove the ".xml" from the filename)

<!-- These first two midiclips shouldn't exist! There's only one pattern. How did this happen? -->
<midiclip autoresize="1" off="0" name="" len="192" type="0" pos="0" steps="16" muted="0"/>
<midiclip autoresize="1" off="0" name="" len="192" type="0" pos="0" steps="16" muted="0"/>
<midiclip autoresize="1" off="0" name="" len="192" type="0" pos="0" steps="16" muted="0">
  <note key="69" vol="100" pan="0" len="12" type="1" pos="24"/>
  <note key="69" vol="100" pan="0" len="12" type="1" pos="72"/>
  <note key="69" vol="100" pan="0" len="12" type="1" pos="120"/>
  <note key="69" vol="100" pan="0" len="12" type="1" pos="168"/>
</midiclip>

rubiefawn avatar Dec 02 '25 01:12 rubiefawn

Thanks for investigating the Carla issue! Given that it also exists on master as well and is probably specific to something Carla is doing, I would suppose it's probably fine to merge this PR as is, and fix the Carla issue in a separate PR?

This PR has two approvals so far, has been tested by bratpeki, and has been open for over a year now, so I think I may go ahead and merge it now.

regulus79 avatar Dec 02 '25 22:12 regulus79