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

GaugePanel: Setting the neutral-point of a gauge #53989

Merged
merged 33 commits into from Nov 18, 2022
Merged

GaugePanel: Setting the neutral-point of a gauge #53989

merged 33 commits into from Nov 18, 2022

Conversation

sfranzis
Copy link
Contributor

@sfranzis sfranzis commented Aug 20, 2022

What this PR does / why we need it:
The neutral or zero point of a gauge is at the left of the panel. When I set the range from e.g. Min=-5 to Max = 5 then the display of negative values looks not natural.
I would like the neutral-point to be configurable in the standard field options (where we have Min, Max, ...).

Example use case: a solar battery inverter, which shows positive values when charging and negative values when discharging.

Fixes #38273

Usage: If min and max of a gauge panel are additive inverse numbers, e.g. min=-500, max=500 then the neutral point of the gauge is put at 12 o'clock. Negative values are plotted to the left side, positive values to the right side.

If min and max of a gauge panel are opposite numbers, e.g. min=-500, max=500 then the neutral point of the gauge is put at 12 o'clock.
Negative values are plotted to the left side, positive values to the right side.
See Gitlab discussions  #38273
remove unneded const angularModules
@CLAassistant
Copy link

CLAassistant commented Aug 20, 2022

CLA assistant check
All committers have signed the CLA.

@grafanabot grafanabot added pr/external This PR is from external contributor area/frontend labels Aug 20, 2022
@sfranzis sfranzis changed the title Setting the neutral-point of a gauge #38273 Setting the neutral-point of a gauge Aug 20, 2022
@sfranzis sfranzis requested a review from a team as a code owner August 20, 2022 09:19
@sfranzis sfranzis requested review from gguillotte-grafana and knylander-grafana and removed request for a team August 20, 2022 09:19
@tclausing
Copy link

tclausing commented Sep 8, 2022

I'm happy to see this feature. Though, limiting to inverse min/max scenario seems arbitrary. Why not expose a setting for "fill from zero" regardless of the angle of the zero point?

Perhaps this should even be default behavior of a gauge. I can't think of an example where painting from a negative min to a positive value would be desired since a gauge is meant to show relative magnitudes, which implies painting from zero to represent magnitude correctly.

@mellieA
Copy link
Contributor

mellieA commented Sep 9, 2022

@sfranzis thank you for this contribution! I've moved this to the to the User Essentials dev project for review and comments

@mellieA
Copy link
Contributor

mellieA commented Sep 9, 2022

Approvers, here are some images copied over from the feature discussion that this PR resolves:

image

@sfranzis
Copy link
Contributor Author

I'm happy to see this feature. Though, limiting to inverse min/max scenario seems arbitrary. Why not expose a setting for "fill from zero" regardless of the angle of the zero point?

Perhaps this should even be default behavior of a gauge. I can't think of an example where painting from a negative min to a positive value would be desired since a gauge is meant to show relative magnitudes, which implies painting from zero to represent magnitude correctly.

An additional setting would require changes on the data model and more. For this I'm not yet deep enough in the code. This is my first change.
I agree with the idea making this to the default behavior. But also in this case you need to set min and max values to have a fixed scale.

Copy link
Contributor

@gguillotte-grafana gguillotte-grafana left a comment

Choose a reason for hiding this comment

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

There are some phrasing changes that could make the docs addition clearer and require less domain knowledge to apply.


Adjust various options.

- **Min/Max -** If the values of min and max are additive inverse numbers (e.g. min=-500, max=500), then the neutral point of the gauge is put at 12 o'clock. Negative values are plotted to the left side, positive values to the right side.
Copy link
Contributor

Choose a reason for hiding this comment

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

Aiming for clarity with plainer language here, while understanding the limitations in the comments:

Suggested change
- **Min/Max -** If the values of min and max are additive inverse numbers (e.g. min=-500, max=500), then the neutral point of the gauge is put at 12 o'clock. Negative values are plotted to the left side, positive values to the right side.
- **Min/Max -** Define a range to gauge, with the neutral point plotted at the center of the gauge. Values higher or lower than the neutral point plotted to the left or right of center, respectfully. The values of min and max must be opposite numbers; for instance, if the min is -500, the max must be 500.

c1, // line color
1, // line width
c1, // fill color
blur);
}


/**
* if gauge min is negative and has the absolute value of min and max are the same
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* if gauge min is negative and has the absolute value of min and max are the same
* if gauge min is negative and the absolute value of min and max are the same

Comment on lines 57 to 59
## Standard options

Adjust various options.
Copy link
Contributor

Choose a reason for hiding this comment

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

With only one setting listed, "various options" doesn't work well here. If I understand the comments, this is changing a default behavior of gauges, so would "Adjust default gauge behaviors" work instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @gguillotte-grafana thanks for your review.

@ashharrison90 ashharrison90 requested review from a team, joshhunt, ashharrison90 and JoaoSilvaGrafana and removed request for a team September 16, 2022 08:40
Comment on lines 362 to 386
/**
* if gauge min is negative and has the absolute value of min and max are the same
* then calculate a1 and a2 in a way, that zero is at 12 o'clock
*
* @method calculateAnglesForGauge
* @param {Object} gaugeOptionsi the options of the gauge
* @returns {Object}
*/
function calculateAnglesForGauge(gaugeOptionsi, layout, data) {
let angles = {};
var setNorth = (gaugeOptionsi.gauge.min + gaugeOptionsi.gauge.max) == 0.0

angles.a1 = gaugeOptionsi.gauge.startAngle;
angles.a2 = calculateAngle(gaugeOptionsi, layout, data);

if(setNorth && angles.a2<=1.5) {
angles.a1 = angles.a2;
angles.a2 = 1.5;
}
if(setNorth && angles.a2>1.5) {
angles.a1 = 1.5;
}

return angles;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @sfranzis thank you for contributing to Grafana! 🎉

I have to agree with the sentence "limiting to inverse min/max scenario seems arbitrary", I think it would end up being almost a "hidden feature" that if you set min and max to the same absolute value you get this extra feature on the Gauges that have negative numbers.

I think ideally this should always happen if the minimum of the gauge is < 0, and then we should be able to see some kind of indication of where 0 is as well, maybe with a dotted line or with the actual number 0 next to that point.
So I feel like in order for us to merge this it would need a little bit more work. This is my suggestion for what this function should look like:

        /**
         * Calcualte the angles for the gauge, depending on if there are
         * negative numbers or not.
         * 
         * @method calculateAnglesForGauge
         * @param {Object} gaugeOptionsi the options of the gauge
         * @param  {Number} data the value of the gauge
         * @returns {Object}
         */
        function calculateAnglesForGauge(gaugeOptionsi, layout, data) {
            let angles = {};
            var hasNegative = gaugeOptionsi.gauge.min < 0.0
            var isNegative = data < 0.0
            
            if (hasNegative) {
                if (isNegative) {
                    angles.a1 = calculateAngle(gaugeOptionsi, layout, data);
                    angles.a2 = calculateAngle(gaugeOptionsi, layout, 0.0);
                } else {
                    angles.a1 = calculateAngle(gaugeOptionsi, layout, 0.0);
                    angles.a2 = calculateAngle(gaugeOptionsi, layout, data);
                }
            } else {
                angles.a1 = gaugeOptionsi.gauge.startAngle;
                angles.a2 = calculateAngle(gaugeOptionsi, layout, data);
            }
            
            return angles;
        }

What do you think? Then the only thing left would be to find a way to represent where the 0 is when the min is negative. Would you like to give that a go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @JoaoSilvaGrafana thanks for your suggestion. I'm fine with it. 👍

@sfranzis
Copy link
Contributor Author

Working on it.

  1. Regarding the modified logic, (when minimum < 0) it works is general, but is a bit tricky when no values for min/max are set. In this case the values vor min/max are calculated. Any idea how to check if no value is set in the GUI?
  2. Zero marker - I implemented this:

image

@LarsStegman
Copy link
Contributor

LarsStegman commented Sep 24, 2022

Thanks for the pull request. Just my 2 cents: I would prefer to be able to override the neutral point to set it to any arbitrary value. The current implementation would be fine for the default, i.e. the neutral point is always at min, unless the user overrides it.

@JoaoSilvaGrafana
Copy link
Contributor

Working on it.

  1. Regarding the modified logic, (when minimum < 0) it works is general, but is a bit tricky when no values for min/max are set. In this case the values vor min/max are calculated. Any idea how to check if no value is set in the GUI?
  2. Zero marker - I implemented this:
image

Hey @sfranzis sorry for the delay, been on holiday 😄

  1. I'm not sure I understand your point, could you give me an example of where it doesn't work? I tried it with various values and no min/max and it behaves correctly:

Screenshot 2022-09-30 at 16 20 10

2. I think we might need to put this through a UX check, could you push your changes and I can ask some designers here and see what they think? 🙂

Thanks for working on this!

@JoaoSilvaGrafana
Copy link
Contributor

Thanks for the pull request. Just my 2 cents: I would prefer to be able to override the neutral point to set it to any arbitrary value. The current implementation would be fine for the default, i.e. the neutral point is always at min, unless the user overrides it.

Hey @LarsStegman , just trying to understand your point. Could you give an example of what you expected to happen with some values? By neutral point do you mean the point in the middle of the Gauge?

@LarsStegman
Copy link
Contributor

LarsStegman commented Sep 30, 2022

Thanks for the pull request. Just my 2 cents: I would prefer to be able to override the neutral point to set it to any arbitrary value. The current implementation would be fine for the default, i.e. the neutral point is always at min, unless the user overrides it.

Hey @LarsStegman , just trying to understand your point. Could you give an example of what you expected to happen with some values? By neutral point do you mean the point in the middle of the Gauge?

Hi, I would like the "zero point" settable. For example:

  • the current value is 3
  • The minimum value is -5
  • The maximum value is 5
  • the neutral point is manually set to -1

The gauge would then be drawn from -1 to 3

This could be useful to show how far away from a setpoint the current value is, e.g. a speed setting and the actual speed.

@JoaoSilvaGrafana
Copy link
Contributor

Oh fantastic!
Could you merge that with this branch then? We are having a quick call with UX tomorrow about this and I can demo it to them 😄
Thanks and great job!

@sfranzis
Copy link
Contributor Author

sfranzis commented Nov 15, 2022

Example use case for neutral value different from 0:
Monitoring the divergence of line frequency (50.0 Hz in Europe 60.0 Hz in the US and other countries)
image

@sfranzis
Copy link
Contributor Author

Oh fantastic!
Could you merge that with this branch then? We are having a quick call with UX tomorrow about this and I can demo it to them 😄
Thanks and great job!

Ok, I will do it later today when I have no disturbance. :-)

@sfranzis
Copy link
Contributor Author

Hey @JoaoSilvaGrafana,
merged & synced with main. Build passed. If you accept my changes I will also add a few lines to documentation.
Regards :-)

Copy link
Contributor

@JoaoSilvaGrafana JoaoSilvaGrafana left a comment

Choose a reason for hiding this comment

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

Great job on this! I showed your work to some colleagues and they really like the NumberInput option you created! 🎉 Just left some comments on it

packages/grafana-ui/src/components/Gauge/Gauge.tsx Outdated Show resolved Hide resolved
packages/grafana-ui/src/components/Gauge/Gauge.tsx Outdated Show resolved Hide resolved
public/app/plugins/panel/gauge/module.tsx Outdated Show resolved Hide resolved
public/app/plugins/panel/gauge/models.cue Outdated Show resolved Hide resolved
public/vendor/flot/jquery.flot.gauge.js Outdated Show resolved Hide resolved
@JoaoSilvaGrafana JoaoSilvaGrafana removed the migrate/status-icebox Temporary label that can be deleted after 2022 Nov label Nov 16, 2022
@sfranzis
Copy link
Contributor Author

sfranzis commented Nov 16, 2022

Thanks for your review and comments. Implemented it and pushed.
Just one idea left. I would like to have the Neutral option also in the Overrides section. In some panels the Overrides offer only Standard options, Data Links and Thresholds. I've already searched hours, but still no idea how to get Gauge > Neutral into the overrides. 🙈
With this feature you could build Gauge panels with different data (e.g. Voltage neutral at 230, Frequency at 50):
image

Do you have an idea or hint how to implement this?

@JoaoSilvaGrafana
Copy link
Contributor

Do you have an idea or hint how to implement this?

I'm not really sure how to implement overrides no... I had a quick look around and couldn't get it to work properly. I think what we have right now in this PR is already really valuable for us to merge it, and then if you figure out the overrides you can do it as a separate PR what do you think? 😄

@JoaoSilvaGrafana JoaoSilvaGrafana added this to the 9.2.6 milestone Nov 17, 2022
@JoaoSilvaGrafana JoaoSilvaGrafana changed the title Setting the neutral-point of a gauge Gauge: Setting the neutral-point of a gauge Nov 17, 2022
@JoaoSilvaGrafana JoaoSilvaGrafana changed the title Gauge: Setting the neutral-point of a gauge GaugePanel: Setting the neutral-point of a gauge Nov 17, 2022
@JoaoSilvaGrafana JoaoSilvaGrafana modified the milestones: 9.2.6, 9.3.0 Nov 17, 2022
@sfranzis
Copy link
Contributor Author

sfranzis commented Nov 17, 2022

I think what we have right now in this PR is already really valuable for us to merge it, and then if you figure out the overrides you can do it as a separate PR what do you think? 😄

Go for it 👍 I'm looking forward to seeing the current change in an official release. 😊

@sfranzis
Copy link
Contributor Author

@JoaoSilvaGrafana found the override thing.
You have to implement the property as a custom field config, not as an option.
Changed it and pushed. If I implemented in a later request, it would get a breaking change.
Please see again and tell me what you think. 🙂

image

Copy link
Contributor

@JoaoSilvaGrafana JoaoSilvaGrafana left a comment

Choose a reason for hiding this comment

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

Great job on this, we really appreciate all the work you've put into it 👏

@JoaoSilvaGrafana JoaoSilvaGrafana merged commit e823a90 into grafana:main Nov 18, 2022
grafanabot pushed a commit that referenced this pull request Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

9 participants