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

Legend indicator: color may not follow color_thresholds #1116

Open
ildar170975 opened this issue Jul 24, 2024 · 13 comments
Open

Legend indicator: color may not follow color_thresholds #1116

ildar170975 opened this issue Jul 24, 2024 · 13 comments
Labels
bug Something isn't working released on @dev

Comments

@ildar170975
Copy link
Collaborator

ildar170975 commented Jul 24, 2024

Initial report: #1115 (comment)

With #1115 merged:
a legend indicator's color may follow color_thresholds:

type: custom:mini-graph-card
entities:
  - entity: sensor.xiaomi_cg_1_co2
    show_state: true
    state_adaptive_color: true
  - entity: sensor.xiaomi_cg_2_co2
    show_state: true
    state_adaptive_color: true
name: CO2
hours_to_show: 24
points_per_hour: 60
lower_bound: ~400
color_thresholds:
  - value: 450
    color: orange
  - value: 550
    color: green
  - value: 650
    color: magenta
color_thresholds_transition: hard
height: 400
show:
  labels: true

изображение
i.e. here a color = PINK for both entities.

With changed color_thresholds:

color_thresholds:
  - value: 450
    color: orange
  - value: 550
    color: green
  - value: 680
    color: magenta

there is different picture - an indicator is BLACK for the 2nd entity:
изображение

Same - when graphs are built for attributes.

@ildar170975 ildar170975 added the bug Something isn't working label Jul 24, 2024
@ildar170975 ildar170975 changed the title Legend indicator: color may not follow colow_thresholds Legend indicator: color may not follow color_thresholds Jul 24, 2024
@akloeckner
Copy link
Collaborator

TL;DR

We should try to substitute inState by state here:

const factor = (c2.value - inState) / (c2.value - c1.value);


I did some digging and I think:

We're sorting the thresholds in descending order here:

valuedStops.sort((a, b) => b.value - a.value);

We then look for the first threshold c1, which is less than the state, as well as the next higher threshold c2 here:

mini-graph-card/src/main.js

Lines 655 to 657 in 49ca1cd

const index = color_thresholds.findIndex(ele => ele.value < state);
const c1 = color_thresholds[index];
const c2 = color_thresholds[index - 1];

If all thresholds are below the state, there is no c2 and we land in the else-branch for degenerate cases. This seems to work well:

mini-graph-card/src/main.js

Lines 662 to 664 in 49ca1cd

intColor = index
? color_thresholds[color_thresholds.length - 1].color
: color_thresholds[0].color;

However, the "normal" branch seems to be broken. This is the case, where we actually have c1 and c2. My current guess is that the factor cannot be calculated, because inState is a string:

mini-graph-card/src/main.js

Lines 659 to 660 in 49ca1cd

const factor = (c2.value - inState) / (c2.value - c1.value);
intColor = interpolateColor(c2.color, c1.color, factor);

I also guess that this is no issue for the graph itself, because it seems to be coloured by an SVG gradient (which I am not familiar with).

renderSvgGradient(gradients) {

@akloeckner
Copy link
Collaborator

We should try to substitute inState by state

Nope, that didn't help...

@akloeckner
Copy link
Collaborator

Ok... Next guess: the interpolateColor is weird. 😉 It only works on hexadecimal colours, such as '#00ff00':

const interpolateColor = (a, b, factor) => {
const ah = +a.replace('#', '0x');
const ar = ah >> 16;
const ag = (ah >> 8) & 0xff;
const ab = ah & 0xff;
const bh = +b.replace('#', '0x');
const br = bh >> 16;
const bg = (bh >> 8) & 0xff;
const bb = bh & 0xff;
const rr = ar + factor * (br - ar);
const rg = ag + factor * (bg - ag);
const rb = ab + factor * (bb - ab);
return `#${(((1 << 24) + (rr << 16) + (rg << 8) + rb) | 0).toString(16).slice(1)}`;
};

Try converting your colours to that notation. This works well for me. Example:

color_thresholds:
  - value: 450
    color: '#ff0000'
  - value: 550
    color: '#00ff00'
  - value: 650
    color: '#0000ff'

I am not sure how to fix this. We'd need to internally convert named colours to hexadecimal notation...

Or go the lazy way used for the state: do not interpolate and use computeColor instead of intColor:

style=${entityConfig.state_adaptive_color ? `color: ${this.computeColor(state, id)};` : ''}>

computeColor(inState, i) {

@ildar170975
Copy link
Collaborator Author

convert named colours to hexadecimal notation

I wonder is there any standard function to do it?

@akloeckner
Copy link
Collaborator

I also wondered... Since 2023 we can even directly use CSS's color-mix. But that would break compatibility with lots of older devices.

Then, I found d3.interpolate, which seems to do (among many other things) exactly what we need. It would be a new dependency, I think. But maybe worth a try.

@akloeckner
Copy link
Collaborator

d3 seems to work well. See #1118 . I'll migrate computeColor, too. Maybe you can then do some checks...

@ildar170975
Copy link
Collaborator Author

@akloeckner
very good! I will test it in a week!

Copy link

github-actions bot commented Aug 7, 2024

🎉 This issue has been resolved in version 0.12.2-dev.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@ildar170975
Copy link
Collaborator Author

Fixed in 0.12.2-dev.2
Thank you, @akloeckner

@jlsjonas
Copy link
Collaborator

jlsjonas commented Aug 27, 2024

Hi @akloeckner, first of all thanks for your continued help in maintaining mini-graph-card!

I sadly enough only got freed up enough now to notice #1118 getting merged.

The original implementation is actually very efficient, despite being limited to hex.
Sadly enough adding dependencies in the graph part of the app also goes directly against one of the (unwritten?) core objectives of the project to avoid libraries for the graph (i.e. core) part of the card, with enough focus on efficiency; leading to graphs that still work well on underpowered devices.

We can certainly extend the feature set; and use a (minimal) lib to parse named colors to hex on the configuration-side (as that's definitely a value-add) if required.

(among many other things)

is sadly enough the problem here :)

/cc @ildar170975

p.s.: thanks that you didn't :)

Or go the lazy way used for the state: do not interpolate and use computeColor instead of intColor:

@jlsjonas jlsjonas reopened this Aug 27, 2024
@jlsjonas
Copy link
Collaborator

Have a look at https://stackoverflow.com/a/1573141/620141

The canvas-method might be more future-proof as it's per spec, but is quite slow (https://jsperf.app/sobipi). I'd personally consider the risk of many new css colors being added on the low side & just hardcode it.

@akloeckner
Copy link
Collaborator

😄 I also found the canvas method and decided to remember this as a fun creative way of doing things. But never to do this myself. 😝 Sorry for calling the efficient implementation "weird"...

Are you experiencing problems on slow devices? We can always re-introduce the original interpolation and just convert everything to hex internally. Or we first wait if someone complains and then possibly re-visit this.

@jlsjonas
Copy link
Collaborator

No worries, I fully understand the perception if you're not used to binary.

The slow device support is mainly an extra advantage (I have noticed issues on my slowest device with other graphs, so it's definitely something to stay aware of)

The main reason for reopening is to keep the core code of the card (graphs) dependency-free as intended by kalkih.
This would be the first exception to that, and I believe there to be valid alternatives (converting on config import/sanitize).

I'm sadly still quite short on extra time to code but if you're willing & able to contribute I'd be glad to continue assisting architecturally meanwhile (just @ me)!

In this case, we preferably revert the introduction of d3.interpolate in graph.js & use one of the SO methods when importing the config.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released on @dev
Projects
None yet
Development

No branches or pull requests

3 participants