New Spectrum Analyzer#4950
Conversation
|
What's the status here? Work in progress? Review in progress? Review required? |
|
"Review required", probably. Or at least that's what I intended when I posted this PR. I've been using the analyzer as a user for the last two weeks and I found some more features and refinements that I would like to implement in the future, so in that respect it is also work in progress. It may take some time before I come back to it, though, since I meanwhile moved back to other stuff that has higher priority for me. The extra features I have in mind would be:
|
The mentioned additional features sound like you could do them in a further, independent PR, so I'd say we request a review for this? |
I agree. That way, if some people start using the plugin and request additional features, I could implement them all at once. |
|
OK, your changes are all good. I'll go on with the other classes now. |
JohannesLorenz
left a comment
There was a problem hiding this comment.
Review Part 2: SaProcessor class.
Sorry for the high amount of comments, and as always, feel free to ignore irrelevant comments.
Co-Authored-By: Johannes Lorenz <1042576+JohannesLorenz@users.noreply.github.com>
Co-Authored-By: Johannes Lorenz <1042576+JohannesLorenz@users.noreply.github.com>
Co-Authored-By: Johannes Lorenz <1042576+JohannesLorenz@users.noreply.github.com>
Co-Authored-By: Johannes Lorenz <1042576+JohannesLorenz@users.noreply.github.com>
…nsigned int conversion)
Note, this is not true. They take mostly equally long. I'll |
JohannesLorenz
left a comment
There was a problem hiding this comment.
This was the last functional review which includes SaSpectrumView, SaWaterfallView and all changes in the "core" (the only thing following is a simple style review). Your changes of recent comments were all OK, until the still open realtime comment.
Additional issue I need to check (unless you have a good idea):
- When merging this with branch
variable-tab-widget(remote git@github.com:JohannesLorenz/lmms.git, it is an Lv2 sidebranch), 3osc looks wrong. Without that merge, it looks OK. It works if I comment-out your changes insrc/gui/MainWindow.cpp. This is likely a bug invariable-tab-widget.
|
One more issue: Loading an old project with an old spectrum analyzer will fail. You'll need to add an upgrade routine to |
|
Oops, I did not think of that at all. Thanks for catching that. |
|
It seems that manually renaming the new plugin from |
I did not think of that simple solution. Good idea 😄 |
|
Btw, I'm still busy fixing the 3osc-bug that arised when merging |
There was a problem hiding this comment.
Style check is OK.
There are only two things to be done:
- Making this realtime safe. We agreed on doing this in another PR.
- When this branch will be merged into master, 3osc will be displayed too large. This is because of bugs in/revealed by branch
variable-tag-widget. I'll have to fix them there before we can merge this branch.
Replace old spectrum analyzer by new one with higher resolution and many new features. Resolves LMMS#2847.
Fix the scaling of `PixmapButton` when used in layouts. In this case `PixmapButton::sizeHint` is queried which used `devicePixelRatio` to divide the size of the active pixmap. As a result the size is always the same in pixels regardless of the scaling factor of the application. This is fixed by removing the calls. Example: If the scaling factor is 2 then the pixmap will also report twice the size in pixels and hence request more space in layouts, etc. However, if we divide by the device/pixel ratio then the original size of the image/pixmap will be reported and therefore it will be too small in layouts. The calls to `devicePixelRatio` have been introduced with pull request LMMS#4950 (via commit c3b4d51) which replaced the old Spectrum Analyzer. The pixmaps of the Spectrum Analyzer still look good with this change. Fixes LMMS#7052.
This PR replaces the old Spectrum Analyzer plugin with a new, more full-featured, resizable and DPI-aware spectrum analyzer. It resolves issue #2847 and provides a good starting point for #4395 and #1904, since most of the requested Equalizer changes are already implemented in the new analyzer.

Outside of the SpectrumAnalyzer folder, the changes are limited to: