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

feat: Implement graph widget in OC tab #283

Closed
wants to merge 13 commits into from

Conversation

In-line
Copy link
Contributor

@In-line In-line commented Mar 5, 2024

graph

@In-line In-line force-pushed the graph-widget branch 2 times, most recently from 631170c to c5690ba Compare March 5, 2024 00:31
@In-line In-line marked this pull request as draft March 5, 2024 16:54
@In-line
Copy link
Contributor Author

In-line commented Mar 5, 2024

@ilya-zlobintsev I'm in the middle of refactoring and adding throttle visualization, but feel free to review WIP impl too

@In-line In-line marked this pull request as ready for review March 5, 2024 20:56
@In-line
Copy link
Contributor Author

In-line commented Mar 5, 2024

New look & feel
plot

@In-line
Copy link
Contributor Author

In-line commented Mar 5, 2024

@ilya-zlobintsev feel free to review

@ilya-zlobintsev
Copy link
Owner

This is a very cool addition, but there are some considerations:

  • The graph currently shows GPU usage and temperature on a single chart. This works fine since the values have a similar range, but in the future it could be useful to have graphs for clockspeed, voltage or power usage - those would have to be separate. Not yet sure how that should look
  • IMO the OC page is getting kind of crowded, I think it would be best to hide the graph behind something like an expander by default, like it's done for power states. Alternatively, and considering point 1, there could be a dedicated popout window specifically for real-time graphs that the user can open on the side as needed. It could also contain more graphs in the future.
  • The graph is currently drawn on a white background - ideally it would look like something that tries to integrate with the rest of the widgets by following the GTK theme, but I am not sure if that's feasible to implement. Though if the popup window is implemented this isn't as much of an issue, as there would be a separate window that's mostly white graphs, and the main window without them. Another solution could also be having dark/light modes for the graph, and selecting one based on system settings.
  • Passing data to the plot via a json isn't the best. It should be possible to set it directly, but probably needs to be connected to the widget manually instead of relying on derived properties + bindings in the template
  • I see that you had to downgrade gtk-rs for this. Probably can't do much about it until plotters-cairo gets updated

I'm also gonna need to read up some theory to understand how the cubic spline works 🙂

But overall this would be a very useful thing to have. Typically I use amdgpu_top for real-time monitoring, but it would be nice to have some historical data in LACT too, especially when you're over/underclocking and want to see the impact.

@ilya-zlobintsev
Copy link
Owner

@In-line I've made another PR based on this one, implementing most of the things mentioned in the previous comment. Please take a look at it when you can: #301

@ilya-zlobintsev
Copy link
Owner

I'm going to close this PR as it has been superseded by #301 (feel free to leave your thoughts about further improvements)

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