-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
New Spectrum Analyzer #4950
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. |
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.
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 |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: