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

Change GraphWidget to ChartFX #657

Merged
merged 11 commits into from
Jul 4, 2020
Merged

Change GraphWidget to ChartFX #657

merged 11 commits into from
Jul 4, 2020

Conversation

carbotaniuman
Copy link
Contributor

Overview

This widget changes the underlying graphing engine to chart-fx. This alleviates the memory cost of storing a large amount of doubles (as they are stored in unboxed form), as well as improving the refresh rate slightly.

We've also added an autoscrolling toggle to allow manual scrolling. Zooming and panning is allowed thanks to chart-fx.

Video

https://youtu.be/8uW_hd7A7zI

Copy link
Member

@SamCarlberg SamCarlberg left a comment

Choose a reason for hiding this comment

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

Good first pass. I still need to review performance

@SamCarlberg
Copy link
Member

UI issues

Why do the chart labels have [a.u.] at the end? It's probably the default unit used by ChartFX, but it should be specified. X axis should always be s or seconds; Y axis might need to be user-configurable

The chart and scroll toggle button overflow the widget bounds at the default minimum size. Making the autoscroll option a context menu thing or a widget setting (or both) would be helpful in reducing clutter. Making it a setting would also mean it could be specified from the WPILib API.

image

The chart label text is always black, making it hard or impossible to read on dark themes
image

The chart stops growing vertically after a certain point, leaving useless whitespace at the bottom of the widget
image

@carbotaniuman
Copy link
Contributor Author

Is there any documentation on what specifically dark theme changes/where the code is located?

@AustinShalit
Copy link
Member

Nice getting dark mode working. We need to use stable versions of the software (no snapshots). Do you think you can convince that developer to push a release?

@carbotaniuman
Copy link
Contributor Author

@AustinShalit It looks like they're already moving to the next major version so I don't think that's going to happen, but I still have the rest of @SamCarlberg 's future review to address.

@SamCarlberg
Copy link
Member

The Good

Performance seems much better now. I can run at 60fps fairly smoothly, but memory usage goes up really fast as a result. The GC seems to be able to handle it (goes from ~4GB heap to ~700MB, so there is a lot of garbage). Updates are much slower at larger heap sizes. The 10Hz configuration you've set should be OK, but I would like to see a global setting in the base plugin preferences to allow users to increase the update rate if they want.

The Bad

  • Axis labels are black on the midnight theme and aren't readable

  • Unselecting Y-axis autoranging raises an error because you're binding the tick unit property, and the axis tries to set its own value to that bound property. Not sure if this is a bug in your code or in chartfx.

  • Toggling visible datasets in the graph widget preferences has no effect

  • Changing a small existing number widget (for example, a number field) to a graph makes the graph area overflow the bounds of the widget tile. This is probably more of an issue with how shuffleboard handles tiles and isn't something that needs to be addressed in this PR. (Though the simplest solution would probably be to check that the minimum size of the target type is smaller than the tile, and disable the option if the tile is too small)

Overall, I'm very happy with the changes you've made. Fix the small things and we can merge it

Copy link
Member

@SamCarlberg SamCarlberg left a comment

Choose a reason for hiding this comment

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

Some minor things. You'll also need to refactor or reformat to pass checkstyle and PMD.

@SamCarlberg SamCarlberg merged commit 824258e into wpilibsuite:master Jul 4, 2020
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.

4 participants