Skip to content
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

Merged
merged 55 commits into from
Jul 17, 2019
Merged

New Spectrum Analyzer #4950

merged 55 commits into from
Jul 17, 2019

Conversation

he29-net
Copy link
Contributor

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.
sp_snapshot3

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

  • fft_helpers: Added / modified / replaced some functions; most of it was used only by the old analyzer and the Equalizer, so it should not affect anything else. All changes should be compatible with the Equalizer.
  • PixmapButton.cpp: Make the reported sizeHint respect device pixel ratio set for the pixmap, otherwise the button size for SVG rendered in native screen resolution would be too large. It should not affect any other buttons, since they do not explicitly set custom device pixel ratio for their pixmap (i.e. size will be divided by 1).
  • EffectView.cpp and EffectControlDialog.cpp: Make it possible to have resizable effect control windows; implemented in the same way as in PR First version of a new dynamic LADSPA control dialog #2068 as suggested by @michaelgregorius.
  • MainWindow.cpp: Resize a new windowed widget (sub-window) to its sizeHint(). Otherwise it always starts at its smallest possible size.

@he29-net he29-net closed this Apr 17, 2019
@he29-net he29-net reopened this Apr 17, 2019
@JohannesLorenz
Copy link
Contributor

What's the status here? Work in progress? Review in progress? Review required?

@he29-net
Copy link
Contributor Author

he29-net commented May 5, 2019

"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:

  • hi-res / transient mode -- don't shift the window by n samples, but by n/2 (processing each sample twice). This prevents situations where a short signal peak (e.g. most percussion sounds) randomly falls right next to the edge of the FFT window, the window squishes signal nearly to zero and there is therefore almost nothing to display. By overlapping the windows, short impulses would be captured much more consistently, but naturally, there is some performance hit. Which leads me to:
  • advanced config popup: zero padding amount, waterfall gamma curve, peak falloff speed, curve drawing resolution -- all of these values are currently fixed, but I can imagine someone may have a reason to change them. An on/off switch for the transient mode could be included here, along with warnings that some of the advanced settings may negatively impact performance.
  • waterfall cursor (at least for frequency, maybe time as well) -- I originally did not want to implement this, since the value readout would have to occlude some part of the waterfall, but I ended up wishing it was there a few times while using the analyzer, so I reconsidered..

@JohannesLorenz
Copy link
Contributor

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.

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?

@he29-net
Copy link
Contributor Author

he29-net commented May 6, 2019

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.

@JohannesLorenz
Copy link
Contributor

OK, your changes are all good. I'll go on with the other classes now.

Copy link
Contributor

@JohannesLorenz JohannesLorenz left a 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.

plugins/SpectrumAnalyzer/SaProcessor.h Outdated Show resolved Hide resolved
plugins/SpectrumAnalyzer/SaProcessor.h Show resolved Hide resolved
plugins/SpectrumAnalyzer/SaProcessor.h Show resolved Hide resolved
plugins/SpectrumAnalyzer/SaProcessor.h Outdated Show resolved Hide resolved
plugins/SpectrumAnalyzer/SaProcessor.h Outdated Show resolved Hide resolved
plugins/SpectrumAnalyzer/SaSpectrumView.h Outdated Show resolved Hide resolved
plugins/SpectrumAnalyzer/SaSpectrumView.h Outdated Show resolved Hide resolved
plugins/SpectrumAnalyzer/SaProcessor.h Outdated Show resolved Hide resolved
plugins/SpectrumAnalyzer/SaSpectrumView.h Outdated Show resolved Hide resolved
plugins/SpectrumAnalyzer/SaProcessor.cpp Outdated Show resolved Hide resolved
he29-net and others added 6 commits June 23, 2019 11:47
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>
@JohannesLorenz
Copy link
Contributor

JohannesLorenz commented Jun 26, 2019

Considering the FFT, I confused this with IFFT. IFFT can take very long (especially for generating longer waves), but FFT is usually very fast.

Note, this is not true. They take mostly equally long. I'll correct remove that part from my comment.

Copy link
Contributor

@JohannesLorenz JohannesLorenz left a 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 in src/gui/MainWindow.cpp. This is likely a bug in variable-tab-widget.

plugins/SpectrumAnalyzer/SaWaterfallView.cpp Outdated Show resolved Hide resolved
plugins/SpectrumAnalyzer/SaWaterfallView.cpp Show resolved Hide resolved
plugins/SpectrumAnalyzer/SaSpectrumView.h Outdated Show resolved Hide resolved
plugins/SpectrumAnalyzer/SaWaterfallView.h Outdated Show resolved Hide resolved
plugins/SpectrumAnalyzer/SaWaterfallView.cpp Show resolved Hide resolved
src/gui/MainWindow.cpp Outdated Show resolved Hide resolved
@JohannesLorenz
Copy link
Contributor

One more issue: Loading an old project with an old spectrum analyzer will fail. You'll need to add an upgrade routine to src/core/DataFile.cpp

@he29-net
Copy link
Contributor Author

he29-net commented Jul 8, 2019

Oops, I did not think of that at all. Thanks for catching that.

@he29-net
Copy link
Contributor Author

It seems that manually renaming the new plugin from Analyzer to spectrumanalyzer to match the old name does the trick; the new analyzer loads with default settings. The DataFile.cpp is pretty scary so I would prefer this simple fix rather than touching that file if it's OK.. :D

@JohannesLorenz
Copy link
Contributor

It seems that manually renaming the new plugin from Analyzer to spectrumanalyzer to match the old name does the trick

I did not think of that simple solution. Good idea 😄

@JohannesLorenz
Copy link
Contributor

Btw, I'm still busy fixing the 3osc-bug that arised when merging variable-tag-widget into this branch. Looks like I have it 40% fixed. Should be done by this weekend.

@JohannesLorenz JohannesLorenz self-requested a review July 11, 2019 18:44
Copy link
Contributor

@JohannesLorenz JohannesLorenz left a 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.

@JohannesLorenz JohannesLorenz merged commit c3b4d51 into LMMS:master Jul 17, 2019
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Replace old spectrum analyzer by new one with higher resolution and
many new features.

Resolves LMMS#2847.
michaelgregorius added a commit to michaelgregorius/lmms that referenced this pull request Jan 7, 2024
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.
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.

5 participants