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

Fix rendering of the Plotter widget on initial states #1709

Merged

Conversation

rafaellehmkuhl
Copy link
Member

Fix #1708

Copy link
Contributor

@ArturoManzoli ArturoManzoli left a comment

Choose a reason for hiding this comment

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

The plotter widget now shows up on Cockpit start, but it has no values. It says: Max: Infinity; Min: Infinity; Current: NaN.

It's already better than it was, but I think would be even better if the initial value on the plot widget was the input-Widget last value, when available. I think all input widget has that last value saved on boot

image

@rafaellehmkuhl rafaellehmkuhl force-pushed the do-initial-plotter-rendering branch from ca81b25 to 9d14e43 Compare February 21, 2025 20:09
@rafaellehmkuhl
Copy link
Member Author

The plotter widget now shows up on Cockpit start, but it has no values. It says: Max: Infinity; Min: Infinity; Current: NaN.

It's already better than it was, but I think would be even better if the initial value on the plot widget was the input-Widget last value, when available. I think all input widget has that last value saved on boot

Done!

@rafaellehmkuhl
Copy link
Member Author

@ArturoManzoli I've added an extra thing: if there's only one value (which is the regular case for input widgets on boot), draw an horizontal line. It seems more intuitive.

image

@rafaellehmkuhl rafaellehmkuhl changed the title Do initial plotter rendering Fix rendering of the Plotter widget on initial states Feb 21, 2025
@ES-Alexander
Copy link
Contributor

if there's only one value (which is the regular case for input widgets on boot), draw an horizontal line. It seems more intuitive.

This could be confusing, by making it seem like there are values available (e.g. for a sensor that is actually broken / not communicating) - it would be preferable to show something like an un-filled circle at the starting position (on the left), and ideally show the current value text as NaN1.

We could maybe generalise this by making it so any valid value followed by nothing is a filled circle, and any valid value directly followed by NaN is an empty circle, which would then help to display things like a rangefinder losing its target lock or something. That way we could just start plotters with NaN, and if they have a last recorded value then they can start with that followed by NaN (which then also makes it clear where the real values start once they begin).

Footnotes

  1. I think it's fine for min/max to be set, since they don't actually require valid data points, especially given they'll likely be optionally manually controllable in future.

Copy link
Contributor

@ArturoManzoli ArturoManzoli left a comment

Choose a reason for hiding this comment

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

On the first cockpit boot after initializing the server, the plot widget really shows the last value, but on a F5 refresh the plot state gets back to the NaN.

Screenshare.-.2025-02-24.7_44_38.AM.mp4

@ArturoManzoli
Copy link
Contributor

ArturoManzoli commented Feb 24, 2025

if there's only one value (which is the regular case for input widgets on boot), draw an horizontal line. It seems more intuitive.

This could be confusing, by making it seem like there are values available (e.g. for a sensor that is actually broken / not communicating) - it would be preferable to show something like an un-filled circle at the starting position (on the left), and ideally show the current value text as NaN1.

We could maybe generalise this by making it so any valid value followed by nothing is a filled circle, and any valid value directly followed by NaN is an empty circle, which would then help to display things like a rangefinder losing its target lock or something. That way we could just start plotters with NaN, and if they have a last recorded value then they can start with that followed by NaN (which then also makes it clear where the real values start once they begin).

Footnotes

  1. I think it's fine for min/max to be set, since they don't actually require valid data points, especially given they'll likely be optionally manually controllable in future.

I agree with you, and this might get a little confusing on some cases. But on general use I think it will be fine.
One quick solution I think would be to use a dashed line on the boot to show the last value as a horizontal line. Once a new value is provided to the data-lake variable, the line become solid again.

The empty circle would be mathematically more accurate, but I suppose, will require some extra hours of work

@rafaellehmkuhl
Copy link
Member Author

rafaellehmkuhl commented Feb 24, 2025

On the first cockpit boot after initializing the server, the plot widget really shows the last value, but on a F5 refresh the plot state gets back to the NaN.

I'm trying to reproduce this problem here, but without success. Maybe it's a problem on the dial widget side?

Just tested here. It's a bug with all input widgets that are inside collapsible containers. Nothing to do with this PR, so opened #1714 to track that.

Kapture.2025-02-24.at.11.13.39.mp4

@rafaellehmkuhl
Copy link
Member Author

rafaellehmkuhl commented Feb 24, 2025

if there's only one value (which is the regular case for input widgets on boot), draw an horizontal line. It seems more intuitive.

This could be confusing, by making it seem like there are values available (e.g. for a sensor that is actually broken / not communicating) - it would be preferable to show something like an un-filled circle at the starting position (on the left), and ideally show the current value text as NaN1.

We could maybe generalise this by making it so any valid value followed by nothing is a filled circle, and any valid value directly followed by NaN is an empty circle, which would then help to display things like a rangefinder losing its target lock or something. That way we could just start plotters with NaN, and if they have a last recorded value then they can start with that followed by NaN (which then also makes it clear where the real values start once they begin).

Footnotes

  1. I think it's fine for min/max to be set, since they don't actually require valid data points, especially given they'll likely be optionally manually controllable in future.

Agree with the circle solution for single and NaN values. Will implement that.

About the other ideas, regarding generalization, I think they are valuable but out of the scope of this PR, since its aimed for fixing the initial rendering, which is broken. Do you think its ok if we left those for a future PR?

@rafaellehmkuhl
Copy link
Member Author

rafaellehmkuhl commented Feb 24, 2025

@ArturoManzoli @ES-Alexander let me know what you think would be the ideal solution. We can also mix the options, regarding circle size, dashed lines, circle position, etc. I've pushed the branch with those cases handled so you can also try changing the rendering code and see what could be the preferable solution.

Options 1 and 2 (circles on the left of the canvas, big and small):

image image

.

Option 3 and 4 (circles in the middle of the canvas, big or small):
image image

@rafaellehmkuhl rafaellehmkuhl force-pushed the do-initial-plotter-rendering branch from 12ad475 to 4322243 Compare February 24, 2025 15:03
@ArturoManzoli
Copy link
Contributor

@ArturoManzoli @ES-Alexander let me know what you think would be the ideal solution. We can also mix the options, regarding circle size, dashed lines, circle position, etc. I've pushed the branch with those cases handled so you can also try changing the rendering code and see what could be the preferable solution.

Option 1 best represents the value as a stationary number for me!

@ES-Alexander
Copy link
Contributor

About the other ideas, regarding generalization, I think they are valuable but out of the scope of this PR, since its aimed for fixing the initial rendering, which is broken. Do you think its ok if we left those for a future PR?

Yeah, that's quality of life, so no worries - I've opened it as #1716 to track.

let me know what you think would be the ideal solution ... regarding circle size, dashed lines, circle position, etc.

Circle on the left definitely seems most intuitive to me. Circle size maybe best in between the current big and small options?

@rafaellehmkuhl rafaellehmkuhl force-pushed the do-initial-plotter-rendering branch from 4322243 to 0aa7aa4 Compare February 25, 2025 11:24
@rafaellehmkuhl
Copy link
Member Author

image

@ES-Alexander @ArturoManzoli done.

@rafaellehmkuhl rafaellehmkuhl requested review from ArturoManzoli and ES-Alexander and removed request for ArturoManzoli and ES-Alexander February 25, 2025 11:24
@rafaellehmkuhl rafaellehmkuhl merged commit 6c09fa1 into bluerobotics:master Feb 25, 2025
11 checks passed
@rafaellehmkuhl rafaellehmkuhl deleted the do-initial-plotter-rendering branch February 25, 2025 11:42
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.

Canvas of the Plotter widgets do not render until the variable changes
3 participants