Skip to content

Remove clamping from Y-coordinate calculations#1296

Draft
argilo wants to merge 1 commit into
masterfrom
remove-vertical-clamping
Draft

Remove clamping from Y-coordinate calculations#1296
argilo wants to merge 1 commit into
masterfrom
remove-vertical-clamping

Conversation

@argilo

@argilo argilo commented Oct 2, 2023

Copy link
Copy Markdown
Member

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

argilo commented Oct 2, 2023

Copy link
Copy Markdown
Member Author

@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

willcode commented Oct 2, 2023

Copy link
Copy Markdown
Contributor

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

willcode commented Oct 2, 2023

Copy link
Copy Markdown
Contributor

The bottom clipping probably has a one-off somewhere.

@argilo

argilo commented Oct 2, 2023

Copy link
Copy Markdown
Member Author

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

willcode commented Oct 2, 2023

Copy link
Copy Markdown
Contributor

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

@argilo

argilo commented Oct 2, 2023

Copy link
Copy Markdown
Member Author

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

@argilo

argilo commented Oct 3, 2023

Copy link
Copy Markdown
Member Author

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