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

Improve rendering of BarGauge #1051

Merged
merged 2 commits into from
May 22, 2024
Merged

Conversation

chriadam
Copy link
Contributor

@chriadam chriadam commented May 16, 2024

  • Always calculate the _nextPos on _value change, regardless of item visibility
  • If animating to _nextPos, ensure we stop any current animation and set the "from" value appropriate, to avoid "over animating".

Fixes #1003

@chriadam chriadam force-pushed the chriadam/fix-bar-gauge-animation branch 3 times, most recently from 4db3b39 to 5ab2832 Compare May 16, 2024 04:42
? orientation === Qt.Vertical
? height - (height * _value) // slide in from bottom to top
: -width + (width * _value) // slide in from left to right
: 0

on_NextPosChanged: {
on_NextPosChanged: if (visible) _updateGauge()
onVisibleChanged: if (visible) _updateGauge()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice and simple. 👌 😊

@jpetrell
Copy link
Contributor

I am still seeing some funky stuff.

Boat/Motorhome demo 1: Quantity label showing 68%, gauge 0%:
DSC_7776

Boat/Motorhome demo 2: Quantity label showing 67%, Gauge showing first ~75% in expanded mode, than after collapsing ~55%:
https://github.com/victronenergy/gui-v2/assets/2203667/74337a38-667e-4a1b-bd74-cab49ef579ca

@chriadam
Copy link
Contributor Author

Thanks, will investigate those ones

@chriadam
Copy link
Contributor Author

Setting layer.enabled: true in the source item seemed to resolve the issue for me, but more testing required.
I'm currently writing a "clipping" BarGauge implementation which doesn't use any MultiEffect, just in case...

- Always calculate the _nextPos on _value change, regardless of
  item visibility
- Only animate to _nextPos if the previous animation has
  completed, to avoid "over animating"
- Enable a layer for the source item
- Specify explicit z ordering for each element

Fixes #1003
@chriadam
Copy link
Contributor Author

Setting layer.enabled: true in the source item seemed to resolve the issue for me, but more testing required.

Nope, still broken.

@chriadam chriadam force-pushed the chriadam/fix-bar-gauge-animation branch from 5ab2832 to 7a5536d Compare May 17, 2024 08:35
@chriadam
Copy link
Contributor Author

Updated with a new clipping bar gauge. It only works for the Tank gauges, and not for other cases (e.g. overview page etc) because it requires a fully opaque "surface colour" to be specified which allows us to draw the rounded edge properly.

Needs more testing, as reproducing this issue can be a bit hit and miss, but I think it's ready for review.

@chriadam chriadam force-pushed the chriadam/fix-bar-gauge-animation branch from 7a5536d to 63ba767 Compare May 17, 2024 08:49
@jpetrell
Copy link
Contributor

The opacity mask shader effect sometimes didn't render properly on device

Great catch! 💪 There has also been discussion about gauges moving out of sync with the value labels in Bea's side panel gauge PRs. At the time Bea tried to investigate the issue, but had no luck. I thought the sync issue was related to labels showing watts and gauges being based on currents, but maybe we are talking about the same issue here? For one I vaguely remember only seeing the sync issues on GX devices.

@jpetrell
Copy link
Contributor

Could maybe refactor so BarGauge and ClippingBarGauge share the same base component, now this duplicates fair share of code?

This fixes tanks, but potentially other gauges are also affected. Ideally there was one fix that helps all the cases. Should we file a bug to Qt about broken MultiEffect?

If MultiEffect is culprit could try doing masks with just ShaderEffect to see if helps. Used to be simple in Qt 5:

layer.enabled: true
layer.samplerName: "maskSource"
layer.effect: ShaderEffect {
    property var colorSource: insides
    fragmentShader: "
        uniform lowp sampler2D colorSource;
        uniform lowp sampler2D maskSource;
        uniform lowp float qt_Opacity;
        varying highp vec2 qt_TexCoord0;
        void main() {
            gl_FragColor = texture2D(colorSource, qt_TexCoord0) * texture2D(maskSource, qt_TexCoord0).a * qt_Opacity;
        }"
}

Didn't have Qt 6 version lying around, so still need to apply small syntax changes Qt 6 brought and move the shader to a (ugh) separate qsb file.

@chriadam
Copy link
Contributor Author

I created my own OpacityMaskEffect:
opacitymaskeffect-bargauge.diff.txt

But it doesn't fix the issue - it just doesn't render, sometimes. I suspect the problem is the old Mali400 drivers, it has a few issues.

Note that the issue you noted above: "gauges moving out of sync with the value labels" is likely the "double animation" issue which I've already fixed. The MultiEffect / OpacityMaskEffect issue I'm seeing is that the bar doesn't get rendered at all (well, the foreground part of the bar) in some cases.

I will create a base type which encapsulates all of the logic and properties.
In future, once we port the entire UI over to using opaque colours, we can simply use the clipping gauge for all cases, I think.

The opacity mask shader effect sometimes didn't render properly
on device.  This commit adds a new clip-based tank gauge instead.
@chriadam chriadam force-pushed the chriadam/fix-bar-gauge-animation branch from 63ba767 to 9d395ab Compare May 20, 2024 05:19
@chriadam
Copy link
Contributor Author

Done. FYI the PR to convert over to fully opaque colours was #689 but it's quite old and I'm certain it will need to be updated now, if we want to go that way.

Copy link
Contributor

@jpetrell jpetrell left a comment

Choose a reason for hiding this comment

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

LGTM, didn't see any issues with the tanks anymore.

But it doesn't fix the issue - it just doesn't render, sometimes. I suspect the problem is the old Mali400 drivers, it has a few issues.

Thanks for testing. 🙏

"gauges moving out of sync with the value labels" is likely the "double animation" issue which I've already fixed

Cool! There is still some other issue lurking somewhere as still seeing gauge updating differently to value on GX device:
https://github.com/victronenergy/gui-v2/assets/2203667/989aeb5b-b9fc-4b59-bcef-54fe0dfb2df5

Doesn't reproduce with the mock backend, where gauge and quantity label always update in sync.

I will create a base type which encapsulates all of the logic and properties.

Looks good. 👍

Could add comment somewhere why we need to BarGauge implementations.

@chriadam
Copy link
Contributor Author

Hmm, interesting that the load graph matches the bar gauge, but the label is completely different to both of them... I will take another look tomorrow.

@jpetrell
Copy link
Contributor

FYI Customer "pwfarnell" reported with the latest 0.3.0 release: "Just checked with ~16. On the Cerbo the bars on the level display are working OK ... On VRM the bars are still missing or wrong at times." I guess we should ask him to retest after we get these improved merged.

@chriadam
Copy link
Contributor Author

I took a look into the side panel thing, and the reason is that the label shows the current, whereas the bar (and graph) shows the ratio.

e.g.

qml: XXXXXXXXXXX acLR current = 0.140000000596, maxCurrent = 2.154, ratio = 0.06499535775116064, label: 0.1
qml: XXXXXXXXXXX acLR current = 0.370000004768, maxCurrent = 2.154, ratio = 0.17177344696750232, label: 0.4
qml: XXXXXXXXXXX acLR current = 0.259999990463, maxCurrent = 2.154, ratio = 0.12070565945357474, label: 0.3
qml: XXXXXXXXXXX acLR current = 0.449999988079, maxCurrent = 2.154, ratio = 0.20891364349071495, label: 0.4
qml: XXXXXXXXXXX acLR current = 0.379999995232, maxCurrent = 2.154, ratio = 0.17641596807428042, label: 0.4
qml: XXXXXXXXXXX acLR current = 0.469999998808, maxCurrent = 2.154, ratio = 0.21819869953946147, label: 0.5
qml: XXXXXXXXXXX acLR current = 0.40000000596, maxCurrent = 2.154, ratio = 0.1857010241225627, label: 0.4
qml: XXXXXXXXXXX acLR current = 0.819999992847, maxCurrent = 2.154, ratio = 0.3806870904582173, label: 0.8
qml: XXXXXXXXXXX acLR current = 0.52999997139, maxCurrent = 2.154, ratio = 0.24605384001392758, label: 0.5
qml: XXXXXXXXXXX acLR current = 0.439999997616, maxCurrent = 2.154, ratio = 0.20427112238440112, label: 0.4
qml: XXXXXXXXXXX acLR current = 0.490000009537, maxCurrent = 2.154, ratio = 0.227483755588208, label: 0.5
qml: XXXXXXXXXXX acLR current = 0.689999997616, maxCurrent = 2.154, ratio = 0.320334260731662, label: 0.7

So they can "get out of sync" if the maxCurrent suddenly changes (due dynamic ranging? not sure).

@chriadam
Copy link
Contributor Author

Everyone happy for me to merge this one as-is?

@blammit
Copy link
Contributor

blammit commented May 21, 2024

I took a look into the side panel thing, and the reason is that the label shows the current, whereas the bar (and graph) shows the ratio.

So they can "get out of sync" if the maxCurrent suddenly changes (due dynamic ranging? not sure).

Yes that's right, the max current could change due to dynamic ranging or if the AC input gauge changes between import-only and import-export.

@chriadam chriadam merged commit 6daa4c9 into main May 22, 2024
1 check passed
@chriadam chriadam deleted the chriadam/fix-bar-gauge-animation branch May 22, 2024 04:35
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.

Tank gauges sometimes trail behind the actual remaining capacity
3 participants