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.

Sign up for GitHub

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

Jump to bottom

dockfft: zoom slider uses log scale with max 100000x #1248

Merged
merged 2 commits into from
Aug 2, 2023
Merged

dockfft: zoom slider uses log scale with max 100000x #1248

merged 2 commits into from
Aug 2, 2023

Conversation

willcode
Copy link
Contributor

@willcode willcode commented May 20, 2023 •
edited
Loading

A log scale is more sensible for the zoom slider, and it was difficult to make it work at all with a linear scale.

Fixes #1235

Also:

  • The slider is now log scaled so zoom per tick is consistent over the entire range
  • Center frequency stays the same when zooming using the slider - it previously was centered on the demod freq which led to strange behavior when both mouse-based and slider-based zoom were used

@argilo
Copy link
Member

argilo commented Jun 17, 2023

After this change, it's no longer possible to zoom all the way out using the slider, because its minimum value is 1, which corresponds to a zoom level of 1.12x:

gqrx/src/qtgui/dockfft.ui

Lines 300 to 302 in 996737c

<property name="minimum">
<number>1</number>
</property>

Changing that to 0 seems to solve the problem.

@willcode willcode force-pushed the feature/zoom-slider-log-scale branch from 124b033 to 015cc10 Compare June 17, 2023 21:09
@argilo
Copy link
Member

argilo commented Jun 17, 2023

Fixes #1235

This PR implements the suggestion I made in #1235 (comment), but it doesn't solve the bugs described in #1235; I verified that they're still present.

@willcode
Copy link
Contributor Author

I'll take a look.

@willcode willcode force-pushed the feature/zoom-slider-log-scale branch 6 times, most recently from 1eb2691 to 0597b35 Compare June 19, 2023 18:34
@argilo argilo added the feature label Jun 21, 2023
@argilo
Copy link
Member

argilo commented Jun 21, 2023

This is mostly looking good, but there are still a couple problems that I think should be addressed:

  1. When zooming out using the mouse wheel while hovering over the frequency axis, it's now possible to zoom out beyond 1x, leaving black on both sides of the plot, and the Freq Zoom indicator showing 0x:
    Screenshot from 2023-06-21 16-54-50
  2. When zooming out using the mouse wheel or the Freq Zoom slider, it's now much easier to end up with empty black space on the left or right of the plot & waterfall. Perhaps we should disallow this completely.

argilo
argilo reviewed Jun 21, 2023
View reviewed changes
src/qtgui/plotter.cpp Outdated
@@ -1525,6 +1502,7 @@ void CPlotter::draw(bool newData)
{
m_2DPixmap.fill(PLOTTER_BGD_COLOR);
QPainter painter2(&m_2DPixmap);
painter2.translate(QPointF(0.5, 0.5));
Copy link
Member

@argilo argilo Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change (and the corresponding change in drawOverlay) deserves to be in its own PR, since it's unrelated to zooming.

@willcode willcode force-pushed the feature/zoom-slider-log-scale branch from 0597b35 to 7b19b67 Compare June 21, 2023 23:39
@willcode
Copy link
Contributor Author

PRs split. I'll take a look at zoom/pan limits.

@willcode willcode force-pushed the feature/zoom-slider-log-scale branch from 7b19b67 to db873d4 Compare June 26, 2023 13:21
@willcode
Copy link
Contributor Author

willcode commented Jun 26, 2023 •
edited
Loading

Zoom and pan are now limited to avoid blank space.

This does make it a bit difficult to zoom in on something at the very low/high end of the scale, especially the low end. Given that there's generally no useful information in the margins, this works for me. There's no good way to allow pan but not zoom outside the valid area.

@willcode willcode force-pushed the feature/zoom-slider-log-scale branch from db873d4 to 95ecd36 Compare June 26, 2023 13:29
@willcode
Copy link
Contributor Author

There are still some problems with this ... still working ...

@willcode willcode force-pushed the feature/zoom-slider-log-scale branch from 95ecd36 to 95c92e4 Compare June 27, 2023 01:24
@willcode
Copy link
Contributor Author

OK, ready for you to try out.

@willcode willcode force-pushed the feature/zoom-slider-log-scale branch 4 times, most recently from 36eb586 to 5f91743 Compare July 2, 2023 16:16
@willcode willcode force-pushed the feature/zoom-slider-log-scale branch from 5f91743 to 41404fe Compare July 25, 2023 12:06
@argilo
Copy link
Member

argilo commented Jul 27, 2023

Sorry for the long delay in getting to this.

I noticed another bug: if you're fully zoomed out, then try to zoom in one step using the mouse wheel, the zoom is not centred around the mouse position. I think that's a side effect of the change to setFftCenterFreq; the zoomStepX method tries to update the FFT centre frequency but can't because the span hasn't been updated yet.

@argilo
Copy link
Member

argilo commented Jul 27, 2023

if you're fully zoomed out

The same behaviour also happens if you're zoomed in and are close to the upper or lower frequency limit.

@willcode
Copy link
Contributor Author

Isn't that just a side effect of not wanting any blank space?

@argilo
Copy link
Member

argilo commented Jul 27, 2023 •
edited
Loading

No, it's not. It should always be possible to zoom in around the mouse cursor, regardless of its position, because the frequency limits are moving towards the cursor frequency.

The problem here is that setFftCenterFreq doesn't know that there's going to be a new, decreased span. So it clamps the center frequency based on the old span, not the new.

@willcode
Copy link
Contributor Author

OK, I was looking at a different effect ... will fix.

@willcode
dockfft: zoom slider uses log scale with max 100000x
3ebb051 
Also, zoom and pan are constrained so no "empty space" is shown
at the sides of the plot.

Signed-off-by: Jeff Long <[email protected]>
@willcode willcode force-pushed the feature/zoom-slider-log-scale branch from 41404fe to 3ebb051 Compare July 27, 2023 19:23
@argilo
Copy link
Member

argilo commented Jul 27, 2023

Your update looks like it should fix it. Taking it for a spin now.

@argilo argilo merged commit d7219a6 into gqrx-sdr:master Aug 2, 2023
10 checks passed
@willcode willcode deleted the feature/zoom-slider-log-scale branch August 2, 2023 12:00
@argilo argilo mentioned this pull request Sep 26, 2023
Merged
argilo added a commit that referenced this pull request Sep 29, 2023
@argilo
Add back clamping in band plan coordinate calculation
9c52c58 
It was inadvertently removed in #1248.
This was referenced Sep 29, 2023
Merged
Merged
jj1bdx pushed a commit to jj1bdx/gqrx that referenced this pull request Feb 25, 2024
@argilo @jj1bdx
Add back clamping in band plan coordinate calculation
601bc67 
It was inadvertently removed in gqrx-sdr#1248.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@argilo argilo argilo left review comments

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Freq zoom slider behaves erratically
2 participants
@willcode @argilo