-
-
Notifications
You must be signed in to change notification settings - Fork 994
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
SlicerT UI update #7453
base: master
Are you sure you want to change the base?
SlicerT UI update #7453
Conversation
4cce529
to
917436e
Compare
917436e
to
0c356d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style review.
plugins/SlicerT/toolbox.png
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this toolbar png, would it make sense to make the background colored via the stylesheet and cover the entire lower band?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure exactly what you mean. This png only covers the lower left, and does not increase in size. The right box resizes and has the colour drawn through code.
If you mean drawing the entire background through code, I don't think that is really possible, since it has some complex shapes, gradients and a blur effect.
And if you mean only having the background be one colour, that would probably not look very good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah drawing the entire band with one colour is what i meant. It looks a bit discontinuous in the current look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how it looks currently, I don't think a flat colour would work all that great. Anyway, you should probably talk with @RoxasKH, since he designed it. There are also a few conversations on the dev-only channel on discord, where we talk about why we chose this design. The gist of it is that a flat colour looks empty, scaling the background looks bad and making it tileable is too much effort.
Co-authored-by: Rossmaxx <74815851+Rossmaxx@users.noreply.github.com>
QString sampleName = m_slicerTParent->getSampleName(); | ||
if (sampleName == "") { sampleName = "No sample loaded"; } | ||
|
||
brush.drawText(5, boxTopY - s_sampleBoxHeight, width(), s_sampleBoxHeight, Qt::AlignLeft, sampleName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like lots of manual layout computations. Ideally the different components would be implemented as widgets of their own which would then be put into Qt's layouts so that they can resize dynamically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure that using Qt's layouts would be more work and more complex. I use that many offsets to get everything to align pixel-perfectly, so even if I used layouts, I would have to add the offset in some way or another. And the move parts are honestly the simplest, the complex part is drawing the text, dividers, gradients, knobs and backgrounds. Don't really see how that could be implemented any differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would definitively be possible with the right resize policies and margins. Layouts are used exactly for situations like these so that complex layout calculations do not have to be reimplemented over and over again. Using widgets in layouts might also lead to more reusable components. However, I guess it's nothing for now.
@DanielKauss, I only did a very quick review but overall I like that this PR lays the base to free the instruments from their confined and tiny windows. 😃 Another good candidate for a resizable and large instrument window would be AudioFileProcessor. Did you also test the rendering performance of the wave view with very large samples? It looks like you are caching the waveform in a |
Did this already on the original PR. The waveform only gets drawn if the seeker changes, and a while ago I also optimized the waveform drawing code, although I don't know if that is still in the codebase. Also tested it again just now and seems fine. |
I have just tested the PR locally and noticed that resizing is very slow and sluggish after having loaded a sample that's 15 seconds long. I guess it will be even worse with longer samples. Based on the performance I get the impression that the code simply renders every sample into the caching pixmap. This will then lead to problems as soon as the pixmap needs to be updated. A 15 seconds long sample has around 720000 samples at a standard sample rate of 48 kHz. If the view is around 1000 pixels wide then this means that 720 samples are rendered on one pixel column. @DanielKauss, if I remember correctly, the view for the samples in the sample track has some optimizations. Can you please check? |
This is somewhat strange. I just tested with a 2 minute sample, and everything worked smoothly. It didn't update at the full 60 fps when resizing, only something like 25, which I think is acceptable for something the user won't do much. And this was with a 2 minute sample, which is exaggerated anyway. And yes, while resizing, the sample gets redrawn on every
No, the waveform drawing function is optimized so that it only draws a maximum of 512 samples per pixel, and I think that is the worst case scenario.
Doesn't look like it, seems like we both just use the Is the lag on resizing really that noticeable? I didn't change any of the drawing code from the original SlicerT, and there everything ran smoothly. If the lag is only minimal, and only present while resizing, I don't think it's worth optimizing for. |
Unfortunately for me it can go well into "several seconds" territory. I have recorded a resizing action here: Bildschirmaufnahme_20240818_103949.webmWhat's interesting is that at the beginning it's ok but that it gets slower and slower as the window gets larger. This might indicate that it is not so much the sample processing but rather the rendering that creates the problems. |
Pretty strange. I have created a small patch to measure the timings. Can you please apply it to this branch and send me the resulting console output when you resize the window? If you don't have a build system set up or something I can send you an executable. Patch is here: timing_patch_slicerT.patch.txt For me, I get the following output, using a 2 minute sample and after resizing for around 5 minutes to check for memory leaks:
Time is in microseconds, and as you can see drawing the big waveform is indeed what takes up the most time. However, for me it still only takes 26 milliseconds, so no idea how you would get a multiple second long delay
This makes sense, since we have to process ~512 samples per pixel-column, and with a wider resolution this just increases. So both the processing time and rendering have to increase. I would be grateful if you could test this out for me, and maybe also provide me with some of your system details. Sorry if this is asking for too much, but thank you anyway. |
@DanielKauss, I have applied the patch and have also added the current time at the start of the resize event to the output. It surprised me that the output indicated an update rate of around 20 Hz/frames per second, i.e. 0.05 seconds per resize event. The visual appearance was laggy like in the video above though. Then it dawned on me that the differences we both observe in performance might be a "Wayland vs. X11" effect as I am running under Wayland. Testing everything under X11 gave a rather smooth output. Attached you can find the output of a resizing action under Wayland and under X11: So the problem seems to be that the Wayland compositor runs at a certain frame rate, in my case 60 Hz, and that if an application is not able to render at that rate then some of its output is dropped. This is a consequence of Waylands "every frame is perfect" approach. This is likely also the reason that I see some updates at the beginning of the resize action but then only very few intermediate ones in between. All other frames are simply dropped and I see the previous rendering of the application. So the problem statement must be corrected to: Resizing the SlicerT window with large samples is laggy under Wayland. Or put differently: LMMS must ensure that it can render at 60 Hz. Otherwise these effects might also show up in other places. |
Wayland could definitely be the problem here. I tried switching to Wayland, but opening LMMS immediately crashes the whole WM (cinnamon). Wayland in general barely works on my system at all, and I have no experience with it either, so I can't really work on this. Optimizing the UI even further to always render more than 60 fps seems unfeasible, especially when for the resize we have to redraw everything. The only band aid fix I can think of is to only redraw the window once the user has stopped resizing, but this would involve some kind of timer, similar to this SO question, and would look worse on all the other systems. Anyway, I can't really work on this, so you should decide what we should do with this. However, this seems like an underlying issue in either Qt, Wayland or the compositor, so this might be difficult to work out. |
@DanielKauss, I have created #7462 to deal with the inefficient rendering. If Wayland becomes more and more commonplace it will make LMMS look bad if it drops so many frames in certain situations. Other applications also manage to render large waveform views so it should be doable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like noted, I only did a quick review and tested the running application. I leave the more detailed code style review to @Rossmaxx. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style and comment review.
|
||
m_wf = new SlicerTWaveform(248, 128, instrument, this); | ||
m_wf->move(2, 6); | ||
m_wf->move(0, s_topBarHeight); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Michael did point this out, basically, you can use a QVBoxLayout to get the elements listed vertically, and the bottom toolbox can be wrapped in a QHBoxLayout. That way, you'll have to do little calculation.
connect(m_resetButton, &PixmapButton::clicked, m_slicerTParent, &SlicerT::updateSlices); | ||
|
||
update(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you switch to layouts, you might not even need this update call
connect(m_resetButton, &PixmapButton::clicked, m_slicerTParent, &SlicerT::updateSlices); | ||
|
||
update(); | ||
} | ||
|
||
Knob* SlicerTView::createStyledKnob() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be easier to add another knob style in Knob.cpp and use that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would that be any easier? Even if it were, the knob styles are supposed to be for general use, not something specific like knobs used by only one plugin.
I'll leave this here in case someone else does a review. Using Qt layouts for the resizing is a waste of time, and would only complicate the code even further. With the current method, I can place each object on the exact pixel that I want, where as layouts do not have that level of precision. Even if I did use layouts, all the hard coded numbers would have to be implemented as well, just in the form of offsets. And it would only really affect around 12 lines: the knob and button move functions in the resize event, and the text drawing in the paint event. As for performance, I would hope that doing a few additions is insubstantial in comparison to all the drawing stuff, which would not be affected at all if I switched to layouts. And before this PR, SlicerT also had these values hard coded, just without taking into account the window size. The same is true for all the other plugins, they all move the knobs to hard coded positions without layouts (even something like Monstro) because it is way simpler to get perfect positions that way. |
The other plugins all have a set size and cannot be resized though. Layouts usually make life easier when working with resizable widgets. However, I agree that adding layouts is not part of this PR because it already worked with hard-coded layouts before. |
Most of the credit goes to @RoxasKH who designed the new UI.
Fixes #7060. This PR updates the existing SlicerT UI to make it bigger and resizable. Also has some small bug fixes and changes to the plugin window related files to allow them to be resizable. Here is the new UI:
The new minimum size is 516x400, and it can be resized without limits.
Changes outside SlicerT
I started this PR a few months ago, so these changes may have been implemented already somewhere else.
The
pluginView
class now has the functionssetResizable
andisResizable
to allow plugins to make their window resizable. This gets checked in theInstrumentTrackWindow
to set a few window flags.Also changed a few things about how effects, LFO and arpeggio act when resized. Again, this might not be needed anymore.
SlicerT bugfixes
Sorry for the huge amount of random numbers in the code, but it's mostly just correcting the UI elements offsets. Anyway, if there is anything I should fix/change please let me know, thank you.