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

Remove clamping from Y-coordinate calculations #1296

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

argilo
Copy link
Member

@argilo argilo commented Oct 2, 2023

The plotter clamps the Y coordinate of many things:

  • max line
  • average line
  • max hold line
  • min hold line
  • peaks

This is not visible at the bottom of the plot, but at the top of the plot, lines and peaks are drawn across the top of the window:

Screenshot from 2023-10-02 16-26-18

This seems undesirable and wastes CPU time. I think the clamping can be safely removed. After:

Screenshot from 2023-10-02 16-28-09

The top-bin highlighting in the histogram plot suffers from a similar problem, but it's not so easy to fix so I'll leave that for later.

@argilo
Copy link
Member Author

argilo commented Oct 2, 2023

@willcode Since you've done a lot of work on the plotter, I expect you might know if there is anything that depends on this clamping. So far I haven't noticed any negative effects.

@willcode
Copy link
Contributor

willcode commented Oct 2, 2023

I don't think it will affect anything else. It was mainly to show that the signal was above scale. But then it should probably do the same thing at the bottom.

@willcode
Copy link
Contributor

willcode commented Oct 2, 2023

The bottom clipping probably has a one-off somewhere.

@argilo
Copy link
Member Author

argilo commented Oct 2, 2023

Yeah, I expect it would need to be plotHeight - 1 to be visible.

Without clamping, it's still possible to tell that the lines have gone out of scale (since they disappear). I suppose with this change you can't tell there's an off-screen peak circle, but I don't think that's a big deal.

I'll let this sit for a bit to see if anyone else has an opinion on clamping vs not.

@willcode
Copy link
Contributor

willcode commented Oct 2, 2023

Some indication of off-scale is nice. We could do it a different way. Nothing great comes to mind at the moment.

@argilo
Copy link
Member Author

argilo commented Oct 2, 2023

I suppose an off-scale indication does become useful when the entire signal is off-screen.

@argilo
Copy link
Member Author

argilo commented Oct 3, 2023

The bottom clipping probably has a one-off somewhere.

I opened #1299 as an alternative, which fixes that bug.

@argilo argilo marked this pull request as draft October 31, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants