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

Signal handles and multi-axis support #9

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

TimonTerBraak
Copy link
Contributor

Maybe you can have a look at this? Not necessarily merge it right away, it probably needs some work / thinking, even if you would like this feature.
This branch prototyped multi-axis support, such that signals can individually scaled and panned, as it is quite common that correlated signals have a vast different domain. It also adds 'signal handles' to the left of the plot, similar to most digital oscilloscopes. This handle can be used for both panning and selecting that signal as being 'active'. Note that the vertical axis on the right is then changed to the active signal. While correct, this part needs some visual changes to ensure that it is clear to which signal the vertical axis belongs to.
I think something similar to this feature is useful to add the plotting of digital signals. It then allows you to organise the plot area, again similar to digital oscilloscopes.
Let me know what you think.

@windelbouwman
Copy link
Owner

Glad to see this change was possible at all, I think the codebase can be modified fairly easy.

I'm not too sure about this change, one thought I had is how far into the direction of logic analyzer / scope I would like to go. There are good tools such as sigrok which are logic analyzers which usually have this kind of view.

Another idea about the multi axis would be to have an option for the plot to show a seperate axis for each signal in the plot, and allow to select it. Then the vertical zooming/panning will be done for the selected signal. I think at least all axis should be visible, preferably with the color of the signal. Maybe we also require the grid to be present multiple times?

Anyway, good proposal! I'm not sure where this whole thing is going, so I would like to keep this idea around for some time.


self._draw_curves()
self._draw_legend()

if self.options.show_legend:
Copy link
Owner

Choose a reason for hiding this comment

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

This option certainly makes sense.

@@ -77,17 +82,17 @@ def _draw_curve(self, curve):

if data:
if isinstance(data[0], Aggregation):
self._draw_aggregations_as_shape(data, curve_color)
curve.average = self._draw_aggregations_as_shape(curve.axis, data, curve_color)
Copy link
Owner

Choose a reason for hiding this comment

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

You could query the average from by querying the summary of the data in view to the time series database (tsdb). This will give you a quicker result than re-calculating the average in the draw function.

Also: having the draw function doing two things (draw the plot and calculate the average) is probably not so clean, it would be better to seperate the two functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, still have to get familiar with the codebase.

@@ -109,16 +132,19 @@ def horizontal_zoom(self, amount, around):
self.chart.horizontal_zoom(amount, around)
# Autoscale Y for a nice effect?
self.chart.autoscale_y()
self.repaint()
Copy link
Owner

Choose a reason for hiding this comment

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

What does this function do? update also triggers a paint action eventually. Does this result in snappier look / feel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The handles on the left hand side otherwise lagged behind in the repainting. Not sure about the reason though. Maybe that 'update' only invalidated the chart area itself?

@TimonTerBraak
Copy link
Contributor Author

Glad to see this change was possible at all, I think the codebase can be modified fairly easy.

Yes, it is quite accessible.

I'm not too sure about this change, one thought I had is how far into the direction of logic analyzer / scope I would like to go. There are good tools such as sigrok which are logic analyzers which usually have this kind of view.

The problem of plotting multiple signals with a vast different range remains. If one signal is of orders of magnitude larger than a second one, plotting them in the same area makes little sense for the secondary signal. This is typically resolved by scaling the range of either of the signals (using a dedicated axis).

Another idea about the multi axis would be to have an option for the plot to show a seperate axis for each signal in the plot, and allow to select it. Then the vertical zooming/panning will be done for the selected signal. I think at least all axis should be visible, preferably with the color of the signal. Maybe we also require the grid to be present multiple times?

Note that in the 'legend bar' branch, the selectability of signals has improved and it is always clear what signal is selected. Just to let you know...

The challenge here is to keep the plot area as clean as possible. Not even all decent scopes are good at that. The visuals of the data aggregations are nice, but are at the same time already much more that a single line. Adding multiple grids and axis gives too much clutter, perhaps.
The black drawing color is clean. Coloring the axis is not that nice, because you have to be careful on the color usage. When doing so, it works best on a black background; then it requires a switch to a dark-themed tool.
The question that remains is how valuable the y-axis actually is. If we have plots with multiple signals, is it not likely that we want to relate some secondary signals to a dominant one (the one with the y-axis)? And we now have the cursor to quickly have the actual value of all signals.
If multiple visible axis are important, would it be sufficient to create multiple plots, each having a different y-axis?

@windelbouwman windelbouwman force-pushed the master branch 5 times, most recently from 9d0c231 to 63233b2 Compare July 1, 2020 18:22
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.

2 participants